Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MIWI Cluster update flow to add new openshift operator identity #4037

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix MIWI Cluster update flow to add new openshift operator identity #4037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3065fae
ARO-13916 fix new operator identity addition flow for miwi cluster up…
rajdeepc2792 Jan 3, 2025
d4ec450
ARO-13916 add the federateIdentityCredentials step back to update
rajdeepc2792 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions pkg/cluster/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,28 +220,25 @@ func (m *manager) Update(ctx context.Context) error {
steps.Action(m.fixupClusterMsiTenantID),
steps.Action(m.ensureClusterMsiCertificate),
steps.Action(m.initializeClusterMsiClients),
)
}

s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources))

if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
s = append(s,
steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs),
steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs),
steps.Action(m.federateIdentityCredentials),
rajdeepc2792 marked this conversation as resolved.
Show resolved Hide resolved
)
} else {
s = append(s,
// Since ServicePrincipalProfile is now a pointer and our converters re-build the struct,
// our update path needs to enrich the doc with SPObjectID since it was overwritten by our API on put/patch.
steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixupClusterSPObjectID),

// CSP credentials rotation flow steps
steps.Action(m.createOrUpdateClusterServicePrincipalRBAC),
)
}

s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be moved down here? I'm thinking that now we could end up with situations where a cx passes a valid MI resource ID, and we either persist its object/client IDs to the cluster doc (or error out with lack of permission trying to do so) before we make it to dynamic validation, which would give the cx a more helpful error message.


if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
s = append(s, steps.Action(m.federateIdentityCredentials))
} else {
s = append(s, steps.Action(m.createOrUpdateClusterServicePrincipalRBAC)) // CSP credentials rotation flow steps
}

s = append(s,
steps.Action(m.initializeKubernetesClients),
steps.Action(m.initializeOperatorDeployer), // depends on kube clients
Expand Down
30 changes: 22 additions & 8 deletions pkg/cluster/workloadidentityresources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (m *manager) generateWorkloadIdentityResources() (map[string]kruntime.Objec
}

resources := map[string]kruntime.Object{}
if platformWorkloadIdentitySecrets, err := m.generatePlatformWorkloadIdentitySecrets(); err != nil {
if platformWorkloadIdentitySecrets, _, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces(); err != nil {
return nil, err
} else {
for _, secret := range platformWorkloadIdentitySecrets {
Expand All @@ -60,30 +60,34 @@ func (m *manager) generateWorkloadIdentityResources() (map[string]kruntime.Objec
}

func (m *manager) deployPlatformWorkloadIdentitySecrets(ctx context.Context) error {
secrets, err := m.generatePlatformWorkloadIdentitySecrets()
secrets, namespaces, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces()
if err != nil {
return err
}
resources := make([]kruntime.Object, len(secrets))
for i, secret := range secrets {
resources[i] = secret
resources := []kruntime.Object{}
for _, namespace := range namespaces {
resources = append(resources, namespace)
}
for _, secret := range secrets {
resources = append(resources, secret)
}

return m.ch.Ensure(ctx, resources...)
}

func (m *manager) generatePlatformWorkloadIdentitySecrets() ([]*corev1.Secret, error) {
func (m *manager) generatePlatformWorkloadIdentitySecretsAndNamespaces() ([]*corev1.Secret, []*corev1.Namespace, error) {
subscriptionId := m.subscriptionDoc.ID
tenantId := m.subscriptionDoc.Subscription.Properties.TenantID
region := m.doc.OpenShiftCluster.Location

roles := m.platformWorkloadIdentityRolesByVersion.GetPlatformWorkloadIdentityRolesByRoleName()

secrets := []*corev1.Secret{}
namespaces := []*corev1.Namespace{}
for name, identity := range m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
role, exists := roles[name]
if !exists {
return nil, platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, roles)
return nil, nil, platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, roles)
}
// Skip creating a secret for the ARO Operator. This will be
// generated by the ARO Operator deployer instead
Expand All @@ -110,9 +114,19 @@ func (m *manager) generatePlatformWorkloadIdentitySecrets() ([]*corev1.Secret, e
"azure_federated_token_file": azureFederatedTokenFileLocation,
},
})

namespaces = append(namespaces, &corev1.Namespace{
TypeMeta: metav1.TypeMeta{
APIVersion: corev1.SchemeGroupVersion.Identifier(),
Kind: "Namespace",
},
ObjectMeta: metav1.ObjectMeta{
Name: role.SecretLocation.Namespace,
},
})
}

return secrets, nil
return secrets, namespaces, nil
}

func (m *manager) generateCloudCredentialOperatorSecret() (*corev1.Secret, error) {
Expand Down
67 changes: 51 additions & 16 deletions pkg/cluster/workloadidentityresources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,17 +317,19 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
location := "eastus"

for _, tt := range []struct {
name string
identities map[string]api.PlatformWorkloadIdentity
roles []api.PlatformWorkloadIdentityRole
want []*corev1.Secret
wantErr string
name string
identities map[string]api.PlatformWorkloadIdentity
roles []api.PlatformWorkloadIdentityRole
wantSecrets []*corev1.Secret
wantNamespaces []*corev1.Namespace
wantErr string
}{
{
name: "no identities, no secrets",
identities: map[string]api.PlatformWorkloadIdentity{},
roles: []api.PlatformWorkloadIdentityRole{},
want: []*corev1.Secret{},
name: "no identities, no secrets",
identities: map[string]api.PlatformWorkloadIdentity{},
roles: []api.PlatformWorkloadIdentityRole{},
wantSecrets: []*corev1.Secret{},
wantNamespaces: []*corev1.Namespace{},
},
{
name: "converts cluster PWIs if a role definition is present",
Expand Down Expand Up @@ -355,7 +357,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
},
},
},
want: []*corev1.Secret{
wantSecrets: []*corev1.Secret{
{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Expand Down Expand Up @@ -393,6 +395,26 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
},
},
},
wantNamespaces: []*corev1.Namespace{
{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Namespace",
},
ObjectMeta: metav1.ObjectMeta{
Name: "openshift-foo",
},
},
{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Namespace",
},
ObjectMeta: metav1.ObjectMeta{
Name: "openshift-bar",
},
},
},
},
{
name: "error - identities with unexpected operator name present",
Expand All @@ -401,9 +423,10 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
ClientID: "00f00f00-0f00-0f00-0f00-f00f00f00f00",
},
},
roles: []api.PlatformWorkloadIdentityRole{},
want: []*corev1.Secret{},
wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
roles: []api.PlatformWorkloadIdentityRole{},
wantSecrets: []*corev1.Secret{},
wantNamespaces: []*corev1.Namespace{},
wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
},
{
name: "skips ARO operator identity",
Expand Down Expand Up @@ -431,7 +454,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
},
},
},
want: []*corev1.Secret{
wantSecrets: []*corev1.Secret{
{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Expand All @@ -451,6 +474,17 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
},
},
},
wantNamespaces: []*corev1.Namespace{
{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Namespace",
},
ObjectMeta: metav1.ObjectMeta{
Name: "openshift-foo",
},
},
},
},
} {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -484,10 +518,11 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
controller, tt.roles,
),
}
got, err := m.generatePlatformWorkloadIdentitySecrets()
gotSecrets, gotNamespaces, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces()

utilerror.AssertErrorMessage(t, err, tt.wantErr)
assert.ElementsMatch(t, got, tt.want)
assert.ElementsMatch(t, gotSecrets, tt.wantSecrets)
assert.ElementsMatch(t, gotNamespaces, tt.wantNamespaces)
})
}
}
Expand Down
Loading