Skip to content

Commit 52aef23

Browse files
tjamettjamet
committed
Do reconcile when ConfigSecret changes
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.
1 parent e6f642a commit 52aef23

File tree

2 files changed

+192
-8
lines changed

2 files changed

+192
-8
lines changed

‎internal/controller/genericprovider_controller.go

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ import (
2323
"errors"
2424
"fmt"
2525

26+
v1 "k8s.io/api/core/v1"
2627
apierrors "k8s.io/apimachinery/pkg/api/errors"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2729
kerrors "k8s.io/apimachinery/pkg/util/errors"
2830
"k8s.io/client-go/rest"
2931
operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
@@ -46,7 +48,8 @@ type GenericProviderReconciler struct {
4648
}
4749

4850
const (
49-
appliedSpecHashAnnotation = "operator.cluster.x-k8s.io/applied-spec-hash"
51+
appliedSpecHashAnnotation = "operator.cluster.x-k8s.io/applied-spec-hash"
52+
appliedConfigHashAnnotation = "operator.cluster.x-k8s.io/applied-config-hash"
5053
)
5154

5255
func (r *GenericProviderReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
@@ -107,9 +110,19 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
107110
return ctrl.Result{}, err
108111
}
109112

110-
if r.Provider.GetAnnotations()[appliedSpecHashAnnotation] == specHash {
111-
log.Info("No changes detected, skipping further steps")
113+
upTodate := r.Provider.GetAnnotations()[appliedSpecHashAnnotation] == specHash
114+
115+
configHash, err := r.getProviderConfigSecretHash(ctx)
116+
if err != nil {
117+
return ctrl.Result{}, err
118+
}
112119

120+
if r.Provider.GetAnnotations()[appliedConfigHashAnnotation] != configHash {
121+
upTodate = false
122+
}
123+
124+
if upTodate {
125+
log.Info("No changes detected, skipping further steps")
113126
return ctrl.Result{}, nil
114127
}
115128

@@ -129,8 +142,19 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
129142
}
130143

131144
annotations[appliedSpecHashAnnotation] = specHash
145+
146+
configHash, err := r.getProviderConfigSecretHash(ctx)
147+
if err != nil {
148+
return ctrl.Result{}, err
149+
}
150+
if configHash != "" {
151+
annotations[appliedConfigHashAnnotation] = configHash
152+
} else {
153+
delete(annotations, appliedConfigHashAnnotation)
154+
}
132155
} else {
133156
annotations[appliedSpecHashAnnotation] = ""
157+
delete(annotations, appliedConfigHashAnnotation)
134158
}
135159

136160
r.Provider.SetAnnotations(annotations)
@@ -218,6 +242,30 @@ func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provide
218242
return res, nil
219243
}
220244

245+
func (r *GenericProviderReconciler) getProviderConfigSecretHash(ctx context.Context) (string, error) {
246+
if r.Provider.GetSpec().ConfigSecret != nil {
247+
secret := &v1.Secret{
248+
ObjectMeta: metav1.ObjectMeta{
249+
Namespace: r.Provider.GetSpec().ConfigSecret.Namespace,
250+
Name: r.Provider.GetSpec().ConfigSecret.Name,
251+
},
252+
}
253+
if secret.Namespace == "" {
254+
secret.Namespace = r.Provider.GetNamespace()
255+
}
256+
err := r.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret)
257+
if err != nil {
258+
return "", err
259+
}
260+
configHash, err := calculateHash(secret.Data)
261+
if err != nil {
262+
return "", err
263+
}
264+
return configHash, nil
265+
}
266+
return "", nil
267+
}
268+
221269
func calculateHash(object interface{}) (string, error) {
222270
jsonData, err := json.Marshal(object)
223271
if err != nil {

‎internal/controller/genericprovider_controller_test.go

Lines changed: 141 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,142 @@ releaseSeries:
424424
}
425425
}
426426

427+
func TestProviderConfigSecretChanges(t *testing.T) {
428+
testCases := []struct {
429+
name string
430+
cmData map[string][]byte
431+
updatedCMData map[string][]byte
432+
expectSameHash bool
433+
}{
434+
{
435+
name: "With the same configmap data, the hash annotation doesn't change",
436+
cmData: map[string][]byte{
437+
"some-key": []byte("some data"),
438+
"another-key": []byte("another data"),
439+
},
440+
updatedCMData: map[string][]byte{
441+
"another-key": []byte("another data"),
442+
"some-key": []byte("some data"),
443+
},
444+
expectSameHash: true,
445+
},
446+
{
447+
name: "With the same configmap data, the hash annotation doesn't change",
448+
cmData: map[string][]byte{
449+
"some-key": []byte("some data"),
450+
"another-key": []byte("another data"),
451+
},
452+
updatedCMData: map[string][]byte{
453+
"another-key": []byte("another data"),
454+
"some-key": []byte("some updated data"),
455+
},
456+
expectSameHash: false,
457+
},
458+
}
459+
460+
for _, tc := range testCases {
461+
t.Run(tc.name, func(t *testing.T) {
462+
g := NewWithT(t)
463+
464+
dataHash, err := calculateHash(tc.cmData)
465+
g.Expect(err).ToNot(HaveOccurred())
466+
467+
updatedDataHash, err := calculateHash(tc.updatedCMData)
468+
g.Expect(err).ToNot(HaveOccurred())
469+
470+
if tc.expectSameHash {
471+
g.Expect(updatedDataHash).To(Equal(dataHash))
472+
} else {
473+
g.Expect(updatedDataHash).ToNot(Equal(dataHash))
474+
}
475+
476+
cmSecretName := "test-config"
477+
478+
provider := &operatorv1.CoreProvider{
479+
ObjectMeta: metav1.ObjectMeta{
480+
Name: "cluster-api",
481+
},
482+
Spec: operatorv1.CoreProviderSpec{
483+
ProviderSpec: operatorv1.ProviderSpec{
484+
Version: testCurrentVersion,
485+
FetchConfig: &operatorv1.FetchConfiguration{
486+
Selector: &metav1.LabelSelector{
487+
MatchLabels: map[string]string{
488+
"test": "dummy-config",
489+
},
490+
},
491+
},
492+
ConfigSecret: &operatorv1.SecretReference{
493+
Name: cmSecretName,
494+
},
495+
},
496+
},
497+
}
498+
499+
providerNamespace, err := env.CreateNamespace(ctx, "test-provider")
500+
t.Log("Ensure namespace exists", providerNamespace.Name)
501+
g.Expect(err).ToNot(HaveOccurred())
502+
503+
configNamespace, err := env.CreateNamespace(ctx, "test-provider-config-changes")
504+
t.Log("Ensure namespace exists", configNamespace.Name)
505+
g.Expect(err).ToNot(HaveOccurred())
506+
507+
g.Expect(env.CreateAndWait(ctx, dummyConfigMap(providerNamespace.Name, testCurrentVersion))).To(Succeed())
508+
509+
provider.Namespace = providerNamespace.Name
510+
provider.Spec.ConfigSecret.Namespace = configNamespace.Name
511+
512+
secret := &corev1.Secret{
513+
ObjectMeta: metav1.ObjectMeta{
514+
Namespace: configNamespace.Name,
515+
Name: cmSecretName,
516+
},
517+
Data: tc.cmData,
518+
}
519+
520+
g.Expect(env.CreateAndWait(ctx, secret.DeepCopy())).To(Succeed())
521+
522+
t.Log("creating test provider", provider.GetName())
523+
g.Expect(env.CreateAndWait(ctx, provider.DeepCopy())).To(Succeed())
524+
525+
g.Eventually(generateExpectedResultChecker(provider, appliedConfigHashAnnotation, dataHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
526+
527+
g.Eventually(func() error {
528+
if err := env.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret); err != nil {
529+
return err
530+
}
531+
532+
// Change provider config data
533+
secret.Data = tc.updatedCMData
534+
535+
return env.Client.Update(ctx, secret)
536+
}).Should(Succeed())
537+
538+
g.Eventually(func() error {
539+
if err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
540+
return err
541+
}
542+
543+
// Set a label to ensure that provider was changed
544+
labels := provider.GetLabels()
545+
if labels == nil {
546+
labels = map[string]string{}
547+
}
548+
labels["my-label"] = "some-value"
549+
provider.SetLabels(labels)
550+
provider.SetManagedFields(nil)
551+
552+
return env.Client.Update(ctx, provider)
553+
}).Should(Succeed())
554+
555+
g.Eventually(generateExpectedResultChecker(provider, appliedConfigHashAnnotation, updatedDataHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
556+
557+
// Clean up
558+
g.Expect(env.Cleanup(ctx, provider, secret, providerNamespace, configNamespace)).To(Succeed())
559+
})
560+
}
561+
}
562+
427563
func TestProviderSpecChanges(t *testing.T) {
428564
testCases := []struct {
429565
name string
@@ -540,7 +676,7 @@ func TestProviderSpecChanges(t *testing.T) {
540676
t.Log("creating test provider", provider.GetName())
541677
g.Expect(env.CreateAndWait(ctx, provider.DeepCopy())).To(Succeed())
542678

543-
g.Eventually(generateExpectedResultChecker(provider, specHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
679+
g.Eventually(generateExpectedResultChecker(provider, appliedSpecHashAnnotation, specHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
544680

545681
g.Eventually(func() error {
546682
if err := env.Client.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
@@ -563,9 +699,9 @@ func TestProviderSpecChanges(t *testing.T) {
563699
}).Should(Succeed())
564700

565701
if !tc.expectError {
566-
g.Eventually(generateExpectedResultChecker(provider, updatedSpecHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
702+
g.Eventually(generateExpectedResultChecker(provider, appliedSpecHashAnnotation, updatedSpecHash, corev1.ConditionTrue), timeout).Should(BeEquivalentTo(true))
567703
} else {
568-
g.Eventually(generateExpectedResultChecker(provider, "", corev1.ConditionFalse), timeout).Should(BeEquivalentTo(true))
704+
g.Eventually(generateExpectedResultChecker(provider, appliedSpecHashAnnotation, "", corev1.ConditionFalse), timeout).Should(BeEquivalentTo(true))
569705
}
570706

571707
// Clean up
@@ -574,14 +710,14 @@ func TestProviderSpecChanges(t *testing.T) {
574710
}
575711
}
576712

577-
func generateExpectedResultChecker(provider genericprovider.GenericProvider, specHash string, condStatus corev1.ConditionStatus) func() bool {
713+
func generateExpectedResultChecker(provider genericprovider.GenericProvider, hashKey, specHash string, condStatus corev1.ConditionStatus) func() bool {
578714
return func() bool {
579715
if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
580716
return false
581717
}
582718

583719
// In case of error we don't want the spec annotation to be updated
584-
if provider.GetAnnotations()[appliedSpecHashAnnotation] != specHash {
720+
if provider.GetAnnotations()[hashKey] != specHash {
585721
return false
586722
}
587723

0 commit comments

Comments
 (0)