feat: configurable API Key K8s secret key name #488
Conversation
Signed-off-by: KevFan <[email protected]>
There was a problem hiding this comment.
Thank you so much for taking the time to address this issue, @KevFan!
I've left a few comments. Nothing that invalidates the change, but mainly to double check on the design we want for the feature and some implementation detail.
One general wondering I have though is whether we want to allow this change into v1beta2 or v1beta3 (addressing #480 first). Although the keySelectors field not being backwards compatible with v1beta1 is not much of a concern (since #483 was merged), it feels like breaking a promise (internal agreement maybe?) that all changes to the API now would go into v1beta3.
Do you have an opinion on this, @alexsnaps?
| apiKey := &APIKey{ | ||
| AuthCredentials: authCred, | ||
| Name: name, | ||
| LabelSelectors: labelSelectors, | ||
| Namespace: namespace, | ||
| KeySelectors: append(keySelectors, apiKeySelector), |
There was a problem hiding this comment.
Minor suggestion:
| KeySelectors: append(keySelectors, apiKeySelector), | |
| KeySelectors: append(keySelectors, defaultAPIKeySelector), |
There was a problem hiding this comment.
Sounds good 👍
On a related note, what's your thoughts on instead of doing this append, use the kubebuilder default annotation in the API struct instead? The append works but this default value is hidden from the CRD, but not sure is this want we want in general
// +kubebuilder:default:={"api_key"}
| logger.V(1).Info("api key unchanged") | ||
| for _, key := range a.KeySelectors { | ||
| newAPIKeyValue := string(new.Data[key]) | ||
| for oldAPIKeyValue, current := range a.secrets { |
There was a problem hiding this comment.
I wonder if the struct shouldn't store 2 maps – api key value → k8s secret and k8s secret namespaced name → api key value. This would make this function as well as appendK8sSecretBasedIdentity both slightly more efficient. On the other hand, updating both maps might be an atomic operation.
WDYT?
| for oldAPIKeyValue, current := range a.secrets { | ||
| if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { | ||
| if oldAPIKeyValue != newAPIKeyValue { | ||
| a.appendK8sSecretBasedIdentity(new) |
There was a problem hiding this comment.
In the issue I said:
Authorino would try in order when reading the secret value of the API key (stopping when the first valid key name is found within the Kubernetes Secret), this change would also make it easier to implement key rotation […]
But I guess the part of making it easier to implement key rotation is not really true, is it? In order to properly support rotation, all keys in a Secret must be accepted, otherwise at the moment a new valid key is added to the secret, the old one is automatically deleted and stop being accepted.
Should we add to the APIKey.secrets map all API key values whose key match any of the APIKey.KeySelectors?
So... @guicassolato, you'd bump the version with this change, is that it? I'm growing concerned I won't get the CEL stuff done by the EOW tho... So the question becomes more of when would |
Description
Closes: #360