Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limitations do not appear to have standard implementation for enforcement #30

Closed
dmikey opened this issue Aug 24, 2020 · 13 comments · Fixed by #113
Closed

limitations do not appear to have standard implementation for enforcement #30

dmikey opened this issue Aug 24, 2020 · 13 comments · Fixed by #113
Assignees

Comments

@dmikey
Copy link

dmikey commented Aug 24, 2020

Looking for a standard implementation spec for enforcing limitation standards. What can a client do to ensure that it is uniformly carrying out this list of concerns, so that every client touching these headers performs the same set of actions upstream.

Example.

Size limit reached. Should a client perform a FIFO, and append the new key=value to the header?

@dyladan
Copy link
Member

dyladan commented Aug 25, 2020

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.

@mtwo
Copy link
Contributor

mtwo commented Aug 25, 2020

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?

@dyladan
Copy link
Member

dyladan commented Aug 25, 2020

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.

@yurishkuro
Copy link
Member

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).

@yurishkuro
Copy link
Member

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.

@mtwo
Copy link
Contributor

mtwo commented Aug 26, 2020

SGTM

@michael-lewis-rft
Copy link

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.

@dyladan
Copy link
Member

dyladan commented Aug 28, 2020

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.

@yurishkuro
Copy link
Member

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.

@michael-lewis-rft
Copy link

michael-lewis-rft commented Aug 28, 2020

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.

@danielkhan
Copy link
Contributor

@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.

@kalyanaj
Copy link
Contributor

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.

@dyladan dyladan linked a pull request Sep 27, 2022 that will close this issue
@dyladan
Copy link
Member

dyladan commented Sep 27, 2022

Limits are clarified in #113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants