Restart on configuration change #200
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bouskaJ The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bouskaJ
requested review from
cooktheryan
and removed request for
lance and
Gregory-Pereira
|
did a quick test with this by modifying the OIDC configuration. It changed the configmap and notified fulcio to redeploy |
|
Code wise nothing jumped out at me as incorrect. I did testing with two deployments and modified OIDC configuration no panics and logs were clean. LGTM from a usage test |
|
/LGTM |
| return g.Failed(fmt.Errorf("could not set controller reference for Secret: %w", err)) | ||
| } | ||
| if updated, err = g.Ensure(ctx, secret); err != nil { | ||
| // ensure that only new key is exposed | ||
| if err = g.Client.DeleteAllOf(ctx, &v1.Secret{}, client.InNamespace(instance.Namespace), client.MatchingLabels(constants.LabelsFor(ComponentName, DeploymentName, instance.Name)), client.HasLabels{FulcioCALabel}); err != nil { |
There was a problem hiding this comment.
I think that old keys should be keept in cluster. The reason is that these CA key could be used to sign entry into transaction log, so it still have to be linked to CTlog configuration rootCertificates
There was a problem hiding this comment.
consider this PR as the baseline for the key rotation. I am sure that we will need something more sophisticated for key rotation support.
| if instance.Status.ServerConfigRef == nil { | ||
| return true | ||
| } | ||
| existing, err := kubernetes.GetConfigMap(ctx, i.Client, instance.Namespace, instance.Status.ServerConfigRef.Name) |
There was a problem hiding this comment.
Maybe better solution to sove this could be usage of single status field which will contains latest hash of spec content which has been reconciled. For example:
kind: Fulcio
spec:
config:
aaa:
IssuerURL: aaa
status:
lastReconciled: b9c66b80ae026e9768222122e8063dd1b66f36ce03fdf2625b198e5e6d2669fa
if hash changed than execute reconciliation loop. Benefit will be more generic detection of changes in spec. Currently changes in monitoring or external access spec will not be reconciled.
Other benefit is to remove kubernetes api requests from CanHandle method
There was a problem hiding this comment.
-
The deployment is loading configuration from the status field. It's not possible to use hardcoded name because the CM is immutable and needs to be recated with every config change.
-
I use similar approach as the CTLog uses. Currently, the config contains keys, the key MUST be password protected so in case of unprotected key, the key is encrypted with generated password. That means that you will never get the same hash for the very same config (the key contains salt so the hash differs even with the same password).
I am not against using the hash but I think, we have bigger rocks...
There was a problem hiding this comment.
Sorry, I should add information that hash is for Operator's objects and not for config maps/secrets.
No description provided.