[Secrets] Support using key maps for secrets #1195
Conversation
|
Reminding myself - add a test that creates a schedule with name that is an invalid secret key |
| return key.startswith(self.internal_secrets_key_prefix) | ||
|
|
||
| def _is_key_map_secret_key(self, key: str) -> bool: | ||
| return key.startswith(self.key_map_secrets_key_prefix) |
There was a problem hiding this comment.
| return key.startswith(self.key_map_secrets_key_prefix) | |
| return key.startswith(self.key_map_secrets_key_prefix) and validate_secret_key_regex(key, raise_on_failure=False) |
A key-map secret must be stored as a 1st level secret (otherwise you'll get issues with key-map pointing at key-map which obviously won't work).
There was a problem hiding this comment.
I already the validate the key map secret key regex on creation (in _validate_and_enrich_secrets_to_store in line 349) as a proof, the test you asked to verify that was working
I don't see a reason validating the key every time I'm calling this function which is just there to check whether we're dealing with a key map secret key
There was a problem hiding this comment.
If the client is used a key name generated by generate_schedule_key_map_secret_key then of course there's no issue. But, the other methods actually get a key-map name, which can be anything. For example, I can call get_secret and pass a name that is not valid for the key-map. In this case the flow will eventually fail in _resolve_secret_key as it tries to list_secrets on the invalid key name. It's not terrible, but it is easier to debug if there's a specific check for the key-map name that puts out an indicative error on that.
I don't mind too much keeping it as-is, but it would have been clearer that way.
| return key.startswith(self.internal_secrets_key_prefix) | ||
|
|
||
| def _is_key_map_secret_key(self, key: str) -> bool: | ||
| return key.startswith(self.key_map_secrets_key_prefix) |
There was a problem hiding this comment.
If the client is used a key name generated by generate_schedule_key_map_secret_key then of course there's no issue. But, the other methods actually get a key-map name, which can be anything. For example, I can call get_secret and pass a name that is not valid for the key-map. In this case the flow will eventually fail in _resolve_secret_key as it tries to list_secrets on the invalid key name. It's not terrible, but it is easier to debug if there's a specific check for the key-map name that puts out an indicative error on that.
I don't mind too much keeping it as-is, but it would have been clearer that way.
|
Sounds good, I will add it in a followup PR (already have one in progress, which I'm afraid gonna conflict with this one, so want to merge it) |
We started saving each schedule credentials in project secrets, so far we used the schedule key to generate the secret key, but secret keys in k8s have restriction which schedule names might not X to.
Therefore I added a mechanism to save a key map in a value of another secret key, that is simply a json dump of a map between the schedule names to generated uuids that are used as the actual secret keys.
Note that we're using the key map only when needed - for schedule names that are invalid secret keys
2 other solutions brought up that we decided not to choose: