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?
| if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { | ||
| if oldAPIKeyValue != newAPIKeyValue { | ||
| a.appendK8sSecretBasedIdentity(new) | ||
| delete(a.secrets, oldAPIKeyValue) |
There was a problem hiding this comment.
We should flag this and the appendK8sSecretBasedIdentity function as non-thread safe. Today the API Key reconciler won't process two workloads at a time, but still long-term risky.
| 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?
| @@ -1,5 +1,4 @@ | |||
| //go:build !ignore_autogenerated | |||
| // +build !ignore_autogenerated | |||
There was a problem hiding this comment.
I feel like different versions of controller-gen/Golang in people's dev envs keep fighting to set this one way or the other.
Indeed with go v1.22.2 and controller-gen v0.15.0 I too get this change, on main:
❯ go version
go version go1.22.2 darwin/arm64
❯ bin/controller-gen --version
Version: v0.15.0
❯ rm -rf ./bin/*
zsh: sure you want to delete all 2 files in …/github.com/kuadrant/authorino/./bin [yn]? y
❯ make generate manifests
go mod tidy
go mod vendor
go: creating new go.mod: module tmp
Downloading sigs.k8s.io/controller-tools/cmd/[email protected]
[…]
❯ git diff
diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
index c70bb9df..3fe74ccf 100644
--- a/api/v1beta1/zz_generated.deepcopy.go
+++ b/api/v1beta1/zz_generated.deepcopy.go
@@ -1,5 +1,4 @@
//go:build !ignore_autogenerated
-// +build !ignore_autogenerated
/*
Copyright 2020 Red Hat, Inc.
@@ -123,7 +122,8 @@ func (in *AuthConfigSpec) DeepCopyInto(out *AuthConfigSpec) {
if val == nil {
(*out)[key] = nil
} else {
- in, out := &val, &outVal
+ inVal := (*in)[key]
+ in, out := &inVal, &outVal
*out = make(JSONPatternExpressions, len(*in))
copy(*out, *in)
}
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
index 3647917e..29171143 100644
--- a/api/v1beta2/zz_generated.deepcopy.go
+++ b/api/v1beta2/zz_generated.deepcopy.go
@@ -1,5 +1,4 @@
//go:build !ignore_autogenerated
-// +build !ignore_autogenerated
/*
Copyright 2020 Red Hat, Inc.
@@ -137,7 +136,8 @@ func (in *AuthConfigSpec) DeepCopyInto(out *AuthConfigSpec) {
if val == nil {
(*out)[key] = nil
} else {
- in, out := &val, &outVal
+ inVal := (*in)[key]
+ in, out := &inVal, &outVal
*out = make(PatternExpressions, len(*in))
copy(*out, *in)
}There was a problem hiding this comment.
Hmm, not sure about this one, this still give this diff on go1.21 locally. This seems to be more related controller-gen v0.15.0 instead requiring go1.22. So people's authorino/bin/controller-gen might be outdated if it's already been downloaded before since I believe it won't be redownloaded by the makefile command if it's already present 🤔
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