Replies: 9 comments 3 replies
-
|
This would help validate the "Human Readable" requirement that already exists as well. As for why that's important, cover up the right two columns and tell me what the difference is here:
Technically all human readable, but not human distinguishable. |
Beta Was this translation helpful? Give feedback.
-
|
I would guess this won't affect most users. One could imagine that if we're trying to restrict this for one reason or another, that even So while this is a fine change, just to devils advocate a bit, this reminds me a bit of the Zero one infinity rule.
We log other headers, and this is a problem with a variety of things, e.g. User-Agent, other MDC things, and maybe should be solved more at the logging framework level if it's an issue? You can suggest this as validation, but practically we don't enforce it that hard.
Your restriction isn't nearly restrictive enough to protect it for metric labels. It's far more of a cardinality problem than a character problem. Like any app doing this has to limit it to like 3 choices for safe label usage anyway. |
Beta Was this translation helpful? Give feedback.
-
|
I could get behind limiting to |
Beta Was this translation helpful? Give feedback.
-
|
"[a-zA-Z0-9/:.-_()[]]+" is the updated RegEx I'm proposing to accommodate some well reasoned suggested naming needs (Largely JIRA, Git, Version and URN) . The current tiny subset of available EC values can Not drive the restrictiveness of this Header value. |
Beta Was this translation helpful? Give feedback.
-
|
Right, from an HTTP Header perspective. Then, if we moved forward with, it would be prudent to add a MUST clause (or some kind of an aside) that applications must interpret it as lower case since it will be used/set that way in non-header scenarios. In the cases where it's stored from the Header, it's gotta be stored that way. |
Beta Was this translation helpful? Give feedback.
-
|
As new use cases come up, I think that's when we consider loosening restrictions. |
Beta Was this translation helpful? Give feedback.
-
|
I'm good with the set Nate Dogg provided, it seems reasonable to me, and achieves the goal I have of preventing control characters and other difficult characters to sanitize from being a valid header. This obviously doesn't mean no sanitization is needed on the application side but it will be a lot easier to handle than how the header is defined today. I don't think we should plan on loosening restrictions. It would be better to have a stable standard declared now that doesn't change with different people/projects. If we loosen restrictions that could mean dev time across a number of services is needed. Are there any hard objections to I'll make sure to update the shared model with the decided upon regex as Travis called out on the PR. |
Beta Was this translation helpful? Give feedback.
-
|
I'm more inclined to say we should keep this a-z0-9-_ initially, and loosen the restriction as we move forward and the requirements for to expand become concrete. Right now it's to support "prod" and "preprod" (and maybe "integration"). Opening it up as much as above seems like there is some standard or expectation that a broader audience is not aware of or necessarily aligned on. We can start down that track... but I'm not sure we're ready or will have the time at this moment if there's not a driving priority. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for guiding this discussion from the architecture side @travisgosselin! It's hard to agree on standards, but I believe it's good to start somewhere. With that, I agree on your approach you've outlined and will make the changes to align with @acchristensen's proposed regex. It will be nice to have some guidance right NOW for teams, with the announced expectation for expansion in the future. This allows teams to plan for a reasonable expansion, but have a more defined structure to work with for now. I'll make the changes in all the needed places within the next few days, referencing this conversation. We can close this discussion out. Thank you to all who participated! |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Summary of Issue
The existing 100-character limit allowed by the Sps-Execution-Context header is already generous and gives us essentially unlimited, human-readable tags using only the characters
A-Z. Since this is a user defined value being sent with the request we need to handle it safely across the stack:Allowing “any dynamic string” feels at odds with common engineering practices for identifiers that are:
If we keep the spec broad, we’ll have to sanitize/escape on every log site, which adds boilerplate and still won’t be perfectly consistent across services.
Concrete Proposal
I've created a PR looking to limit the character set allowed by the Sps-Execution-Context header, found here: #106, and corresponding
sps-api-designchange here.Specifically, narrow character set allowed to:
[A-Za-z0-9_-]This still allows human readable values to be passed, with the benefit that it will be easy to sanitize consistently across the org.
My ask to you:
Please respond with:
We want to identify how backwards compatible this change is as we are making this more restrictive. I am open to other characters being allowed if needed, especially if already used in production code today. However I would be hesitant to allow anything that would be harder to sanitize.
Beta Was this translation helpful? Give feedback.
All reactions