Use cluster doc ID as cluster MSI KV secret name (#4075)

* Use cluster doc ID for cluster MSI secret name * To determine whether the error was a 404, parse as an autorest error rather than as an azuresdk error I found that I needed to do this while deleting a preexisting dev cluster after making the changes from the previous commit
Azure · Jan 31, 2025 · bf12b9d · bf12b9d
1 parent 00c1837
commit bf12b9d
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 99 deletions.
diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go
@@ -36,12 +36,9 @@ var (
 // vault. It does not concern itself with whether an existing certificate is valid
 // or not; that can be left to the certificate refresher component.
 func (m *manager) ensureClusterMsiCertificate(ctx context.Context) error {
-	secretName, err := m.clusterMsiSecretName()
-	if err != nil {
-		return err
-	}
+	secretName := m.clusterMsiSecretName()
 
-	_, err = m.clusterMsiKeyVaultStore.GetSecret(ctx, secretName)
+	_, err := m.clusterMsiKeyVaultStore.GetSecret(ctx, secretName)
 	if err == nil {
 		return nil
 	} else if azcoreErr, ok := err.(*azcore.ResponseError); !ok || azcoreErr.StatusCode != http.StatusNotFound {
@@ -107,10 +104,7 @@ func (m *manager) ensureClusterMsiCertificate(ctx context.Context) error {
 // initializeClusterMsiClients intializes any Azure clients that use the cluster
 // MSI certificate.
 func (m *manager) initializeClusterMsiClients(ctx context.Context) error {
-	secretName, err := m.clusterMsiSecretName()
-	if err != nil {
-		return err
-	}
+	secretName := m.clusterMsiSecretName()
 
 	kvSecretResponse, err := m.clusterMsiKeyVaultStore.GetSecret(ctx, secretName)
 	if err != nil {
@@ -172,13 +166,8 @@ func (m *manager) initializeClusterMsiClients(ctx context.Context) error {
 
 // clusterMsiSecretName returns the name to store the cluster MSI certificate under in
 // the cluster MSI key vault.
-func (m *manager) clusterMsiSecretName() (string, error) {
-	clusterMsi, err := m.doc.OpenShiftCluster.ClusterMsiResourceId()
-	if err != nil {
-		return "", err
-	}
-
-	return fmt.Sprintf("%s-%s", m.doc.ID, clusterMsi.Name), nil
+func (m *manager) clusterMsiSecretName() string {
+	return m.doc.ID
 }
 
 func (m *manager) clusterIdentityIDs(ctx context.Context) error {

diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go
@@ -34,7 +34,7 @@ func TestEnsureClusterMsiCertificate(t *testing.T) {
 	clusterRGName := "aro-cluster"
 	miName := "aro-cluster-msi"
 	miResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName)
-	secretName := fmt.Sprintf("%s-%s", mockGuid, miName)
+	secretName := mockGuid
 
 	secretNotFoundError := &azcore.ResponseError{
 		StatusCode: 404,
@@ -218,64 +218,6 @@ func TestEnsureClusterMsiCertificate(t *testing.T) {
 	}
 }
 
-func TestClusterMsiSecretName(t *testing.T) {
-	mockGuid := "00000000-0000-0000-0000-000000000000"
-	clusterRGName := "aro-cluster"
-	miName := "aro-cluster-msi"
-	miResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName)
-
-	tests := []struct {
-		name       string
-		doc        *api.OpenShiftClusterDocument
-		wantResult string
-		wantErr    string
-	}{
-		{
-			name: "error - invalid resource ID (theoretically not possible, but still)",
-			doc: &api.OpenShiftClusterDocument{
-				OpenShiftCluster: &api.OpenShiftCluster{
-					Identity: &api.ManagedServiceIdentity{
-						UserAssignedIdentities: map[string]api.UserAssignedIdentity{
-							"Hi hello I'm not a valid resource ID": {},
-						},
-					},
-				},
-			},
-			wantErr: "invalid resource ID: resource id 'Hi hello I'm not a valid resource ID' must start with '/'",
-		},
-		{
-			name: "success",
-			doc: &api.OpenShiftClusterDocument{
-				ID: mockGuid,
-				OpenShiftCluster: &api.OpenShiftCluster{
-					Identity: &api.ManagedServiceIdentity{
-						UserAssignedIdentities: map[string]api.UserAssignedIdentity{
-							miResourceId: {},
-						},
-					},
-				},
-			},
-			wantResult: fmt.Sprintf("%s-%s", mockGuid, miName),
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			m := manager{
-				log: logrus.NewEntry(logrus.StandardLogger()),
-				doc: tt.doc,
-			}
-
-			result, err := m.clusterMsiSecretName()
-			utilerror.AssertErrorMessage(t, err, tt.wantErr)
-
-			if result != tt.wantResult {
-				t.Error(result)
-			}
-		})
-	}
-}
-
 func TestClusterIdentityIDs(t *testing.T) {
 	ctx := context.Background()
 

diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go
@@ -364,12 +364,9 @@ func (m *manager) deleteClusterMsiCertificate(ctx context.Context) error {
 		return nil
 	}
 
-	secretName, err := m.clusterMsiSecretName()
-	if err != nil {
-		return err
-	}
+	secretName := m.clusterMsiSecretName()
 
-	if _, err := m.clusterMsiKeyVaultStore.DeleteSecret(ctx, secretName); err != nil && !azuresdkerrors.IsNotFoundError(err) {
+	if _, err := m.clusterMsiKeyVaultStore.DeleteSecret(ctx, secretName); err != nil && !azureerrors.IsNotFoundError(err) {
 		return err
 	}
 

diff --git a/pkg/cluster/delete_test.go b/pkg/cluster/delete_test.go
@@ -372,6 +372,7 @@ func TestDisconnectSecurityGroup(t *testing.T) {
 func TestDeleteClusterMsiCertificate(t *testing.T) {
 	ctx := context.Background()
 	mockGuid := "00000000-0000-0000-0000-000000000000"
+	secretName := mockGuid
 	clusterRGName := "aro-cluster"
 	miName := "aro-cluster-msi"
 	miResourceId := fmt.Sprintf("/subscriptions/%s/resourcegroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName)
@@ -409,23 +410,6 @@ func TestDeleteClusterMsiCertificate(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "error - error getting cluster MSI secret name (this theoretically won't happen, but...)",
-			doc: &api.OpenShiftClusterDocument{
-				ID: mockGuid,
-				OpenShiftCluster: &api.OpenShiftCluster{
-					Identity: &api.ManagedServiceIdentity{
-						UserAssignedIdentities: map[string]api.UserAssignedIdentity{
-							"not a valid MI resource ID": {
-								ClientID:    mockGuid,
-								PrincipalID: mockGuid,
-							},
-						},
-					},
-				},
-			},
-			wantErr: "invalid resource ID: resource id 'not a valid MI resource ID' must start with '/'",
-		},
 		{
 			name: "error - error deleting cluster MSI certificate from key vault",
 			doc: &api.OpenShiftClusterDocument{
@@ -442,7 +426,7 @@ func TestDeleteClusterMsiCertificate(t *testing.T) {
 				},
 			},
 			mocks: func(kvclient *mock_keyvault.MockManager) {
-				kvclient.EXPECT().DeleteSecret(gomock.Any(), fmt.Sprintf("%s-%s", mockGuid, miName)).Times(1).Return(keyvault.DeletedSecretBundle{}, fmt.Errorf("error in DeleteSecret"))
+				kvclient.EXPECT().DeleteSecret(gomock.Any(), secretName).Times(1).Return(keyvault.DeletedSecretBundle{}, fmt.Errorf("error in DeleteSecret"))
 			},
 			wantErr: "error in DeleteSecret",
 		},
@@ -462,7 +446,7 @@ func TestDeleteClusterMsiCertificate(t *testing.T) {
 				},
 			},
 			mocks: func(kvclient *mock_keyvault.MockManager) {
-				kvclient.EXPECT().DeleteSecret(gomock.Any(), fmt.Sprintf("%s-%s", mockGuid, miName)).Times(1).Return(keyvault.DeletedSecretBundle{}, nil)
+				kvclient.EXPECT().DeleteSecret(gomock.Any(), secretName).Times(1).Return(keyvault.DeletedSecretBundle{}, nil)
 			},
 		},
 	}