Giter VIP home page Giter VIP logo

Comments (13)

yurishkuro avatar yurishkuro commented on August 20, 2024 4

I agree that at the spec level it's very hard to come up with a universal truncation strategy. It should be left to the applications and the SDKs implementing the baggage API. The SDKs can be shipped with a number of available strategies, such as

  • Drop random (or largest) keys until the full baggage fits into limits, or
  • Do not allow adding new keys if the new baggage will exceed the limits

But this spec should not dictate the algorithm, just the limits. So it is a MUST that applications cannot produce header that violates the limits, but neither SHOULD nor MUST about how they do it (perhaps MAY with the above strategies as examples).

from baggage.

danielkhan avatar danielkhan commented on August 20, 2024 2

@michaeltlewis in our meeting on 2020-09-08 we agreed that while we don't aim to specify all corner cases, we will provide non-normative recommendation on how to handle them.

from baggage.

michael-lewis-rft avatar michael-lewis-rft commented on August 20, 2024 1

The standard should be clear about how systems should propagate the baggage header and in particular how ordering and duplicate keys should be handled. That said, can we not leave the interpretation of duplicates of particular keys, and of order for that matter, to the consumers of this data?
In other words, the choices of how to parse such items (eg "{ key1: [a,c], key2: b }" etc) is not necessarily something that has to be defined by the standard.

If some systems (e.g. in OpenTracing and OpenTelemetry) can't handle duplicates then they are free to deduplicate by whatever means, but other systems will benefit from having duplicates transported to them in a consistent (ie order preserved) manner.

I note that https://w3c.github.io/baggage/#list currently requires that duplicates are preserved, although discouraged - exactly as @dyladan suggested. This seems fine to me - I would just like to see order preserved (during propagation) as well.

from baggage.

kalyanaj avatar kalyanaj commented on August 20, 2024 1

The spec now defines minimum limits rather than maximum limits. So, implementations must propagate at least the values for the defined minimum limits. @dyladan will be adding some additional information here.

from baggage.

dyladan avatar dyladan commented on August 20, 2024 1

Limits are clarified in #113

from baggage.

dyladan avatar dyladan commented on August 20, 2024

I think at this point, those mechanisms are still too much of an open question to have concrete answers. Eventually, these will be specified and we will have a test suite you can run against your implementation to ensure it is compliant. For an example of such a test suite you can look at the one for Trace Context. I know this isn't a great answer but I hope I didn't completely miss the mark here.

from baggage.

mtwo avatar mtwo commented on August 20, 2024

This is a great question, and we discussed it on the weekly call today. Let's discuss the following here in this issue and then make a decision:

  • Should the list of keys be ordered?
  • Should the keys be modifiable?
  • Can specific keys be deleted? Or should the entire header be dropped if a deletion is necessary?
  • If the list is full, should old keys be removed (assumes an ordered list)? Or should it not be possible to add new keys?
  • .Net's implementation doesn’t allow the modification or deletion of specific keys, does this seem reasonable?

I like the .Net implementation for the following reasons:

  • The solution is simple
  • If I set a baggage key on an edge service, I expect it to be propagated and there's no way to warn me if it gets dropped
  • If I need to delete baggage keys on a call that leaves a security boundary, I can drop the entire header

Thoughts?

from baggage.

dyladan avatar dyladan commented on August 20, 2024

My initial thoughts are that the header should just be an "unordered bag" kv pairs, and that if you are making a request to an untrusted service, you should drop the whole header if you are worried about security sensitive keys. This implies that you cannot drop old keys to insert a new one when the bag is full, since there is no way of knowing which keys are oldest or newest to determine drop order.

That said, i think these requirements should be SHOULD and not MUST, and allow for application developers to specify their own rules if they have a need for it. In some applications, for instance, the app may know which keys are "low value" and can be dropped in order to make room for "high value" keys.

Regarding modification of keys, I'm not sure I see the benefit of disallowing key modification. This seems like a decision best left up to individual application developers.

If I set a baggage key on an edge service, I expect it to be propagated and there's no way to warn me if it gets dropped [or modified]

True that there is no way to warn the edge service that the key was dropped or modified, but this is also true of dropping the whole header. Presumably, if a tier n service overwrites a key set in tier 0, it had a good reason for doing so. My understanding of the baggage mechanism is that it is meant to be used within an application, and not meant to be arbitrarily propagated through third party services the way Trace Context is, so you don't have to trust a third party to propagate your key unmodified. As the application developer, you have control over your own keys and if they are mutated/dropped.

from baggage.

yurishkuro avatar yurishkuro commented on August 20, 2024

Also +1 for just unordered bag of kv pairs. If we support modifiers like baggage: k1=v1;mod1;mod2, k2=v2 then those could be used to impose additional priorities if the applications require it.

Also, I do not see why unsetting a key should be prohibited. After all, this might be the desired behavior of the specific cross-cutting concern, e.g. propagating a fault injection or a breakpoint to a specific place in the architecture, but not further.

from baggage.

mtwo avatar mtwo commented on August 20, 2024

SGTM

from baggage.

michael-lewis-rft avatar michael-lewis-rft commented on August 20, 2024

When describing this as an "unordered list", are we implying that order is not significant?
May I lobby for field ordering to be explicitly preserved?
Since we're explicitly allowing duplicate keys (which is great when you need to add list items efficiently), then order can have significant meaning. Also note that the spec already references RFC7230§3.2.2 for combining fields and that explicitly requires that order is maintained.

from baggage.

dyladan avatar dyladan commented on August 20, 2024

When describing this as an "unordered list", are we implying that order is not significant?

Yes

May I lobby for field ordering to be explicitly preserved?

Also yes :)

Since we're explicitly allowing duplicate keys (which is great when you need to add list items efficiently), then order can have significant meaning.

If this is the case, we should specify what it means to have a duplicate key. Given header key1=a,key2=b,key1=c, should that parse to:

  • { key1: a, key2: b }
  • { key1: [a,c], key2: b }
    • If this one, is the order of a and c significant (list) or insignificant (set)?
  • { key1: c, key2: b }
  • or some other meaning?

If we are going to allow duplicates, I would probably lean toward { key1: a, key2: b }. If you're looking for a particular key, you can scan from left to right and return on the first match. If you need to insert a key in a performance-sensitive context, you can prepend it to the string without parsing the full string.

Another option is to disallow or discourage duplicate keys. A sender SHOULD NOT send duplicate keys, possibly with an exception that they MAY if in a performance-sensitive context, and a receiver MUST expect that duplicate keys are a possibility. This makes key order significant only from a parsing perspective, but leaves the door open to edge cases like what @michaeltlewis described.

Also note that the spec already references RFC7230§3.2.2 for combining fields and that explicitly requires that order is maintained.

That section describes concatenating two arbitrary HTTP headers together. It specifies to preserve order because when combining arbitrary headers you cannot know if the order of the header you are combining is important or not, so the default assumption is that it is important. It doesn't necessitate that the headers being combined are order-important though.

from baggage.

yurishkuro avatar yurishkuro commented on August 20, 2024

I would argue against allowing duplicate keys. This is not just a matter of the header syntax, but also of the many existing "baggage" APIs (e.g. in OpenTracing and OpenTelemetry), which expect strict maps, not multi-maps.

We'd still need to decide if header with dup keys must be treated as invalid or transformed into a strict map with either first-wins or last-wins deduplication. I suspect that existing implementation of baggage usually implement last-wins, since it's the simplest to implement by just populating a map structure as you read KVs from the header.

from baggage.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.