feat(Grafana): Update Grafana Deployment when referenced Secret or Configmap contents change#2525
Conversation
…er rolling restart on change
theSuess
left a comment
There was a problem hiding this comment.
Thanks for all the work you put into this! I took an initial look at this and will chat with the other maintainers about this in our sync today!
| func ReferencedSecretsAndConfigMaps(cr *v1beta1.Grafana) ([]string, []string) { | ||
| secretSet := make(map[string]struct{}) | ||
| configMapSet := make(map[string]struct{}) | ||
|
|
||
| addSecret := func(name string) { | ||
| if name != "" { | ||
| secretSet[name] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| addConfigMap := func(name string) { | ||
| if name != "" { | ||
| configMapSet[name] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| collectDeploymentRefs(cr, addSecret, addConfigMap) | ||
| collectExternalRefs(cr, addSecret) | ||
| collectClientRefs(cr, addSecret) | ||
|
|
||
| secretNames := make([]string, 0, len(secretSet)) | ||
| for name := range secretSet { | ||
| secretNames = append(secretNames, name) | ||
| } | ||
|
|
||
| configMapNames := make([]string, 0, len(configMapSet)) | ||
| for name := range configMapSet { | ||
| configMapNames = append(configMapNames, name) | ||
| } | ||
|
|
||
| sort.Strings(secretNames) | ||
| sort.Strings(configMapNames) | ||
|
|
||
| return secretNames, configMapNames | ||
| } |
There was a problem hiding this comment.
This could be a struct function of the v1beta1.Grafana type directly
| return secretNames, configMapNames | ||
| } | ||
|
|
||
| func collectDeploymentRefs(cr *v1beta1.Grafana, addSecret, addConfigMap func(string)) { |
There was a problem hiding this comment.
I think these functions would be cleaner when returning arrays directly rather than relying on passed down closures
| // Grafana CR's deployment spec and external config, hashes them, and stores the result in | ||
| // vars.SecretsHash. The deployment reconciler then injects this as a SECRETS_HASH env var, | ||
| // so any secret rotation causes a pod template change and triggers a rolling restart. | ||
| func (r *SecretsReconciler) Reconcile(ctx context.Context, cr *v1beta1.Grafana, vars *v1beta1.OperatorReconcileVars, scheme *runtime.Scheme) (v1beta1.OperatorStageStatus, error) { |
There was a problem hiding this comment.
I think we can have this be part of the deployment reconciler
@Baarsgaard / @weisdd what do you think?
There was a problem hiding this comment.
Talked about this in the weekly sync, please refactor this to be part of the deployment reconciler
| { | ||
| // helps to restart Grafana when referenced secrets or configmaps are rotated | ||
| Name: "SECRETS_HASH", | ||
| Value: vars.SecretsHash, | ||
| }, |
There was a problem hiding this comment.
Since this is not actually used, it's better to have this be an annotation on the pod instead. This will also trigger a rollout without polluting the environment variables
…edSecretsAndConfigMaps on Grafana and used GetEnvVarConfigMapSource from tk8s
…ncedSecretsAndConfigMaps and deployment reconciler cheksum annotation process
|
Hey @theSuess I have updated based on ur comments. Lemme know if any more changes required. thanks :) |
theSuess
left a comment
There was a problem hiding this comment.
Sorry for the late review
Looks good from my end, just have a few style suggestions
api/v1beta1/grafana_types.go
Outdated
| // Refs are collected from: (1) deployment pod template — container Env ValueFrom (SecretKeyRef/ | ||
| // ConfigMapKeyRef) and EnvFrom (SecretRef/ConfigMapRef), plus volume Secret and ConfigMap; | ||
| // (2) external — AdminUser, AdminPassword, APIKey, TLS cert Secret if set; (3) client TLS cert | ||
| // Secret if set. The deployment reconciler uses this to compute a hash of those resources' | ||
| // ResourceVersions and sets the checksum/secrets pod template annotation. |
There was a problem hiding this comment.
nit: we can probably format this better so that each point has a linebreak
api/v1beta1/grafana_types.go
Outdated
| return secretNames, configMapNames | ||
| } | ||
|
|
||
| func (in *Grafana) collectDeploymentRefs() (secrets, configMaps []string) { |
There was a problem hiding this comment.
Naming this deploymentRefs() would be more idiomatic go
api/v1beta1/grafana_types.go
Outdated
| return secrets, configMaps | ||
| } | ||
|
|
||
| func collectContainerEnvRefs(c corev1.Container) (secrets, configMaps []string) { |
There was a problem hiding this comment.
| func collectContainerEnvRefs(c corev1.Container) (secrets, configMaps []string) { | |
| func containerEnvRefs(c corev1.Container) (secrets, configMaps []string) { |
api/v1beta1/grafana_types.go
Outdated
| return secrets, configMaps | ||
| } | ||
|
|
||
| func (in *Grafana) collectExternalRefs() []string { |
There was a problem hiding this comment.
Since external instances don't need restarting (and credentials are updated immediately), we don't need to index these
api/v1beta1/grafana_types_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| func TestGrafana_ReferencedSecretsAndConfigMaps(t *testing.T) { |
There was a problem hiding this comment.
Since many test cases only test against the container env vars, can you extract these to their own test and run against collectContainerEnvRefs directly?
I think this would reduce the overhead a bit.
Another alternative would be to create a top level "blank" &Grafana{} on top and then set the required fields using struct accessors like basic.Spec.Deployment.Spec.Template.Spec.Containers = []corev1.Container{...} which would drastically cut down on } } } } } waterfalls 😅
|
Hey @theSuess, |
theSuess
left a comment
There was a problem hiding this comment.
One last minor thing and then this is good to go!
| func (in *Grafana) collectClientRefs() []string { | ||
| if in.Spec.Client == nil || in.Spec.Client.TLS == nil || in.Spec.Client.TLS.CertSecretRef == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return []string{in.Spec.Client.TLS.CertSecretRef.Name} | ||
| } |
There was a problem hiding this comment.
Client secrets are not used in the deployment so this is not needed
Description
This PR adds automatic rolling restarts for Grafana pods when Secrets or ConfigMaps referenced in a Grafana CR are changed. Previously, the operator had no mechanism to react to secret changes, leaving Grafana pods running with stale credentials until manually restarted.
Related Issue
Fixes #2484
What Changed
Added a new SecretsReconciler to the grafana controller:
References are collected from:
Tests
Deployed this grafana
Deployed this secret
Changed the secret to check if the grafana pod restarts

Pod restarted
Unit Tests