Skip to content

Commit

Permalink
Simplify logic by removing unneeded boolean tracking change
Browse files Browse the repository at this point in the history
  • Loading branch information
@kimorris27
kimorris27 committed Nov 22, 2023
1 parent 74f7d43 commit 409693f
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 32 deletions.
45 changes: 26 additions & 19 deletions pkg/cluster/clusterserviceprincipal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package cluster
// Licensed under the Apache License 2.0.

import (
"bytes"
"context"
"strings"
"time"
Expand Down Expand Up @@ -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: <base64 map[string]string with keys 'aadClientId' and 'aadClientSecret'>
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
}
Expand Down
83 changes: 72 additions & 11 deletions pkg/cluster/clusterserviceprincipal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,81 @@ 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()

for _, tt := range []struct {
name string
kubernetescli func() *fake.Clientset
spp api.ServicePrincipalProfile
wantResult bool
wantSecret func() *corev1.Secret
wantErrMsg string
}{
Expand All @@ -209,7 +276,6 @@ func TestServicePrincipalUpdated(t *testing.T) {
ClientID: "aadClientId",
ClientSecret: "aadClientSecretNew",
},
wantResult: false,
wantErrMsg: "",
},
{
Expand All @@ -225,7 +291,6 @@ func TestServicePrincipalUpdated(t *testing.T) {
ClientID: "aadClientId",
ClientSecret: "aadClientSecretNew",
},
wantResult: false,
wantErrMsg: "Error getting Secret",
},
{
Expand All @@ -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 {}",
},
{
Expand All @@ -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
Expand All @@ -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
Expand All @@ -284,7 +346,6 @@ func TestServicePrincipalUpdated(t *testing.T) {
ClientID: "aadClientId",
ClientSecret: "aadClientSecret",
},
wantResult: false,
},
} {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -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"]))
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/cluster/condition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit 409693f

Please sign in to comment.