-
Notifications
You must be signed in to change notification settings - Fork 66
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
✨ Handle config secret updates #565
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @tjamet! |
Hi @tjamet. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-operator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/ok-to-test |
Digging into the failing test code, I don't understand how the proposed changes can influence the status. the failing test ( I can see the test failing similarly in a dependabot PR, leaning towards instability. /retest |
cmd/main.go
Outdated
@@ -286,6 +291,24 @@ func setupReconcilers(mgr ctrl.Manager) { | |||
setupLog.Error(err, "unable to create controller", "controller", "Healthcheck") | |||
os.Exit(1) | |||
} | |||
|
|||
if watchConfigSecretChanges { |
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.
I like the idea of watching secret changes, but I don’t like that it requires a separate controller. Hash of both objects can be combined, and stored under the same annotation.
Controller can establish watch on secrets with object mapper, which will trigger reconcile for all providers where the secret is referenced.
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.
Thanks or the review.
I have updated the code to watch for secret changes and trigger a reconciliation of each Provider using them
Let me know what you think
c8563e7
to
a5a1fed
Compare
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.
Left some comments regarding CR usage, but overall looks good.
cc @furkatgofurov7 @alexander-demicev PTAL
builder := ctrl.NewControllerManagedBy(mgr). | ||
For(r.Provider) | ||
if r.WatchConfigSecretChanges { | ||
builder = builder.Watches(&corev1.Secret{}, &providerSecretMapper{ |
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.
It may be better to use handler.MapFunc
, similar to this, since using queue directly is “internal-ish” way of doing that.
builder.Watch(
source.Kind(mgr.GetCache(), &corev1.Secret{}),
handler.EnqueueRequestsFromMapFunc(secretToProviders),
)
// where
func secretToProviders(ctx context.Context, secret client.Object) []ctrl.Request
// loop over listConfigSecretUsers
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.
I addressed your comment.
Apparently, builder
does not have a Watch
function but a Watches
one which accepts a plain object as first parameter.
I though used handler.EnqueueRequestsFromMapFunc
as suggested and adapted the rest of the code
a5a1fed
to
c748db9
Compare
As a cluster operator, I want to iterate my infrastructure provisioner configuration and ensure the relevant cluster-api-providers are provisioned as configured. Currently, the GenericProvider Reconciler considers exclusively the Providers spec and ignores the data pointed by the ConfigSecret field. If one of those fields changes, the deployment of the provider is unchanged. For example, if an infrastructure is defined as ```yaml apiVersion: operator.cluster.x-k8s.io/v1alpha2 kind: InfrastructureProvider metadata: name: aws namespace: infrastructure-aws-system spec: version: v2.5.2 configSecret: name: aws-variables deployment: replicas: 1 --- apiVersion: v1 kind: Secret metadata: name: aws-variables namespace: capi-config type: Opaque stringData: AWS_B64ENCODED_CREDENTIALS: "SOME_BASE_64_CREDS" ``` It is impossible to get the latest version of `AWS_B64ENCODED_CREDENTIALS` without changing the provisioner version or the deployments or verbosity level This commit proposes to also take into consideration the content of the configuration so, any change to it leads to an adjustment of the deployment.
Currently, if a ConfigSecret is updated, it is not reflected automatically to all providers using it. Implement this behaviour, under an opt-in flag.
5d5c555
to
63f8947
Compare
As requested in the reviews, change the reconcile logic and remove the added secret reconciler. Instead, all generic reconcilers will listen to secret changes and trigger a reconciliation for each Provider using the secret
63f8947
to
e09838c
Compare
What this PR does / why we need it:
As a cluster operator, I want to iterate my infrastructure provisioner
configuration and ensure the relevant cluster-api-providers are
provisioned as configured.
Currently, the GenericProvider Reconciler considers exclusively the
Providers spec and ignores the data pointed by the ConfigSecret field.
If one of those fields changes, the deployment of the provider is
unchanged.
For example, if an infrastructure is defined as
It is impossible to get the latest version of
AWS_B64ENCODED_CREDENTIALS
without changing the provisioner version orthe deployments or verbosity level
This commit proposes to also take into consideration the content of the
configuration so, any change to it leads to an adjustment of the
deployment.
Currently, if a ConfigSecret is updated, it is not reflected automatically to all providers using it.
Add an optional reconciler to trigger the update of all providers using it.
Both combined ensures that any configuration update leads to an update of the provider deployment, with the least changes in behaviour possible