From 409693f420db119037d0dc73b4b3d2a020102e29 Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Wed, 22 Nov 2023 15:30:39 -0600 Subject: [PATCH] Simplify logic by removing unneeded boolean tracking change --- pkg/cluster/clusterserviceprincipal.go | 45 ++++++----- pkg/cluster/clusterserviceprincipal_test.go | 83 ++++++++++++++++++--- pkg/cluster/condition.go | 4 +- 3 files changed, 100 insertions(+), 32 deletions(-) diff --git a/pkg/cluster/clusterserviceprincipal.go b/pkg/cluster/clusterserviceprincipal.go index dd767889b57..bd26991567b 100644 --- a/pkg/cluster/clusterserviceprincipal.go +++ b/pkg/cluster/clusterserviceprincipal.go @@ -4,6 +4,7 @@ package cluster // Licensed under the Apache License 2.0. import ( + "bytes" "context" "strings" "time" @@ -78,59 +79,65 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context) return nil } +// cloudConfigSecretFromChanges takes in the kube-system/azure-cloud-provider Secret and a map +// containing cloud-config data. If the cloud-config data in cf is different from what's currently +// in the Secret, cloudConfigSecretFromChanges updates and returns the Secret. Otherwise, it returns nil. +func cloudConfigSecretFromChanges(secret *corev1.Secret, cf map[string]interface{}) (*corev1.Secret, error) { + data, err := yaml.Marshal(cf) + if err != nil { + return nil, err + } + + if !bytes.Equal(secret.Data["cloud-config"], data) { + secret.Data["cloud-config"] = data + return secret, nil + } + + return nil, nil +} + // servicePrincipalUpdated checks whether the CSP has been updated by comparing the cluster doc's // ServicePrincipalProfile to the contents of the kube-system/azure-cloud-provider Secret. If the CSP -// has changed, it returns true along with a new corev1.Secret to use to update the Secret to match +// has changed, it returns a new corev1.Secret to use to update the Secret to match // what's in the cluster doc. -func (m *manager) servicePrincipalUpdated(ctx context.Context) (bool, *corev1.Secret, error) { - var changed bool +func (m *manager) servicePrincipalUpdated(ctx context.Context) (*corev1.Secret, error) { spp := m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile //data: // cloud-config: secret, err := m.kubernetescli.CoreV1().Secrets("kube-system").Get(ctx, "azure-cloud-provider", metav1.GetOptions{}) if err != nil { if kerrors.IsNotFound(err) { // we are not in control if secret is not present - return false, nil, nil + return nil, nil } - return false, nil, err + return nil, err } var cf map[string]interface{} if secret != nil && secret.Data != nil { err = yaml.Unmarshal(secret.Data["cloud-config"], &cf) if err != nil { - return false, nil, err + return nil, err } if val, ok := cf["aadClientId"].(string); ok { if val != spp.ClientID { cf["aadClientId"] = spp.ClientID - changed = true } } if val, ok := cf["aadClientSecret"].(string); ok { if val != string(spp.ClientSecret) { cf["aadClientSecret"] = spp.ClientSecret - changed = true } } } - if changed { - data, err := yaml.Marshal(cf) - if err != nil { - return false, nil, err - } - secret.Data["cloud-config"] = data - return true, secret, nil - } - - return false, nil, nil + return cloudConfigSecretFromChanges(secret, cf) } func (m *manager) updateAROSecret(ctx context.Context) error { var changed bool err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - changed, secret, err := m.servicePrincipalUpdated(ctx) + secret, err := m.servicePrincipalUpdated(ctx) + changed = secret != nil if err != nil { return err } diff --git a/pkg/cluster/clusterserviceprincipal_test.go b/pkg/cluster/clusterserviceprincipal_test.go index d0948b3cb45..f160ab67d30 100644 --- a/pkg/cluster/clusterserviceprincipal_test.go +++ b/pkg/cluster/clusterserviceprincipal_test.go @@ -189,6 +189,74 @@ func getFakeAROSecret(clientID, secret string) corev1.Secret { } } +func TestCloudConfigSecretFromChanges(t *testing.T) { + for _, tt := range []struct { + name string + secretIn func() *corev1.Secret + cf map[string]interface{} + wantSecret func() *corev1.Secret + wantErrMsg string + }{ + { + name: "New CSP (client ID and client secret both changed)", + secretIn: func() *corev1.Secret { + secret := getFakeAROSecret("aadClientId", "aadClientSecret") + return &secret + }, + cf: map[string]interface{}{ + "aadClientId": "aadClientIdNew", + "aadClientSecret": "aadClientSecretNew", + }, + wantSecret: func() *corev1.Secret { + secret := getFakeAROSecret("aadClientIdNew", "aadClientSecretNew") + return &secret + }, + }, + { + name: "Updated secret (client ID stayed the same, client secret changed)", + secretIn: func() *corev1.Secret { + secret := getFakeAROSecret("aadClientId", "aadClientSecret") + return &secret + }, + cf: map[string]interface{}{ + "aadClientId": "aadClientId", + "aadClientSecret": "aadClientSecretNew", + }, + wantSecret: func() *corev1.Secret { + secret := getFakeAROSecret("aadClientId", "aadClientSecretNew") + return &secret + }, + }, + { + name: "No errors, nothing changed", + secretIn: func() *corev1.Secret { + secret := getFakeAROSecret("aadClientId", "aadClientSecret") + return &secret + }, + cf: map[string]interface{}{ + "aadClientId": "aadClientId", + "aadClientSecret": "aadClientSecret", + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + secret, err := cloudConfigSecretFromChanges(tt.secretIn(), tt.cf) + if tt.wantSecret != nil { + wantSecret := tt.wantSecret() + if secret == nil { + t.Errorf("Did not return a Secret, but expected the following Secret data: %v", string(wantSecret.Data["cloud-config"])) + } + if !reflect.DeepEqual(secret, wantSecret) { + t.Errorf("\n%+v \n!= \n%+v", string(secret.Data["cloud-config"]), string(wantSecret.Data["cloud-config"])) + } + } else if tt.wantSecret == nil && secret != nil { + t.Errorf("Should not have returned a Secret") + } + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + }) + } +} + func TestServicePrincipalUpdated(t *testing.T) { ctx := context.Background() @@ -196,7 +264,6 @@ func TestServicePrincipalUpdated(t *testing.T) { name string kubernetescli func() *fake.Clientset spp api.ServicePrincipalProfile - wantResult bool wantSecret func() *corev1.Secret wantErrMsg string }{ @@ -209,7 +276,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientId", ClientSecret: "aadClientSecretNew", }, - wantResult: false, wantErrMsg: "", }, { @@ -225,7 +291,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientId", ClientSecret: "aadClientSecretNew", }, - wantResult: false, wantErrMsg: "Error getting Secret", }, { @@ -239,7 +304,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientId", ClientSecret: "aadClientSecretNew", }, - wantResult: false, wantErrMsg: "error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type map[string]interface {}", }, { @@ -252,7 +316,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientIdNew", ClientSecret: "aadClientSecretNew", }, - wantResult: true, wantSecret: func() *corev1.Secret { secret := getFakeAROSecret("aadClientIdNew", "aadClientSecretNew") return &secret @@ -268,7 +331,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientId", ClientSecret: "aadClientSecretNew", }, - wantResult: true, wantSecret: func() *corev1.Secret { secret := getFakeAROSecret("aadClientId", "aadClientSecretNew") return &secret @@ -284,7 +346,6 @@ func TestServicePrincipalUpdated(t *testing.T) { ClientID: "aadClientId", ClientSecret: "aadClientSecret", }, - wantResult: false, }, } { t.Run(tt.name, func(t *testing.T) { @@ -299,12 +360,12 @@ func TestServicePrincipalUpdated(t *testing.T) { }, } - result, secret, err := m.servicePrincipalUpdated(ctx) - if result != tt.wantResult { - t.Errorf("Result was %v, wanted %v", result, tt.wantResult) - } + secret, err := m.servicePrincipalUpdated(ctx) if tt.wantSecret != nil { wantSecret := tt.wantSecret() + if secret == nil { + t.Errorf("Did not return a Secret, but expected the following Secret data: %v", string(wantSecret.Data["cloud-config"])) + } if !reflect.DeepEqual(secret, wantSecret) { t.Errorf("\n%+v \n!= \n%+v", string(secret.Data["cloud-config"]), string(wantSecret.Data["cloud-config"])) } diff --git a/pkg/cluster/condition.go b/pkg/cluster/condition.go index beb3c8bcf18..a617403a283 100644 --- a/pkg/cluster/condition.go +++ b/pkg/cluster/condition.go @@ -104,10 +104,10 @@ func isOperatorAvailable(operator *configv1.ClusterOperator) bool { // and old value to compare to. func (m *manager) aroCredentialsRequestReconciled(ctx context.Context) (bool, error) { // If the CSP hasn't been updated, the CredentialsRequest does not need to be reconciled. - spChanged, _, err := m.servicePrincipalUpdated(ctx) + secret, err := m.servicePrincipalUpdated(ctx) if err != nil { return false, err - } else if !spChanged { + } else if secret == nil { return true, nil }