-
Notifications
You must be signed in to change notification settings - Fork 522
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
Allow more characters in API Key names #511
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew! That was a journey! Nice work on this big change. Let's get it in and keep moving!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
workspaces/api/apiserver/src/datastore/deserialization/pairs.rs
Outdated
Show resolved
Hide resolved
This change allows for more characters to appear in our key names. Specifically, "." and "/" are valid in Kubernetes labels and taints, but were disallowed by the existing API system. The size of this change reflects some of the complexity of keys: * Their names are used commonly in discussions, documentation, user data, and code. We want the names to be convenient to reference. * Their naming has security considerations. The character set of key names might not line up with the allowed character set in the data store implementation, for example dots and slashes in the filesystem. * They're used everywhere in API-related code, so while we add functionality and security, we can't make them hard to use. Here are the changes at 30,000 feet: * Special characters in key names are percent-encoded ("URL-encoded") when used as filenames on the filesystem. For example, "%2E" for "." * Key names are "TOML-quoted" elsewhere. This fits usage in user data, and is relatively intuitive. For example: settings.kubernetes.node-labels."group.name" This key name leads to "group.name" being a single segment instead of two, and therefore "group.name" being the name of a Kubernetes label. * Key name segments in API requests and responses don't need to be quoted because they're already separated structurally, for example: {"settings": {"kubernetes": {"node-labels": {"group.name": "value"}}}} * No existing key names or file paths have to change, because keys without special characters are still valid in both schemes. Here are the changes at 10,000 feet: * Key names are treated as a series of segments, rather than a single string. This allows for more precise specification and testing of names. * Key names are parsed and encoded from segments, rather than checking them with a regex. * Key name stripping / appending is done more carefully, by segment. * Internal functions (for example in the data store) take Keys rather than strings, where feasible, to improve specificity. * Some, like to_pairs_with_prefix, continue to take strings, when it makes a simple use case more friendly. * migrations continue to use strings so they don't have to link to the data store, and to make writing them simpler. * Keys no longer Deref to string, AsRef<str>, etc., to reduce the possibility of a mixup resulting in mistaken/double quoting. Other implementation notes: * `get_prefix` had the `strip_prefix` parameter removed because it was no longer used, and would have had to change to taking segments/Key anyway.
This push addresses @bcressey's concerns. Updated and new unit tests pass, and the manual testing above repeats correctly. His request for additional testing exposed two bugs that are also fixed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
This change allows for more characters to appear in our key names.
Specifically, "." and "/" are valid in Kubernetes labels and taints, but were
disallowed by the existing API system.
The size of this change reflects some of the complexity of keys:
code. We want the names to be convenient to reference.
might not line up with the allowed character set in the data store
implementation, for example dots and slashes in the filesystem.
and security, we can't make them hard to use.
Here are the changes at 30,000 feet:
as filenames on the filesystem. For example, "%2E" for "."
relatively intuitive. For example:
settings.kubernetes.node-labels."group.name"
This key name leads to "group.name" being a single segment instead of two,
and therefore "group.name" being the name of a Kubernetes label.
because they're already separated structurally, for example:
{"settings": {"kubernetes": {"node-labels": {"group.name": "value"}}}}
special characters are still valid in both schemes.
Here are the changes at 10,000 feet:
This allows for more precise specification and testing of names.
with a regex.
strings, where feasible, to improve specificity.
case more friendly.
store, and to make writing them simpler.
of a mixup resulting in mistaken/double quoting.
Other implementation notes:
get_prefix
had thestrip_prefix
parameter removed because it was nolonger used, and would have had to change to taking segments/Key anyway.
Fixes #455.
Testing done:
Existing and new unit tests pass, for the whole workspace.
I added a node label with slashes and dots to my user data:
The instance launched OK and all services were running:
The setting shows up as you'd expect - no escaping needed in JSON format.
It won't connect to Kubernetes because spaces aren't valid in label values :)
Fix that:
Then the instance joins just fine, and you can see the label at the end here:
A busybox pod ran fine.
Here's how you can update settings; same JSON format:
POSTing the changes returns a list of the changed settings, e.g. for thar-be-settings. We can see the dotted-key form here, which requires escaping of the inner quotes:
The new setting shows up fine, next to the one from (the earlier) user data: