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

Jump to bottom

shared utility function to check if a cluster is workload identity #3683

Merged
merged 6 commits into from
Jul 23, 2024
Merged

shared utility function to check if a cluster is workload identity #3683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e794516
shared utility function to check if a cluster is workload identity
Jul 11, 2024
03f22f0
adding license statement
Jul 11, 2024
1c37593
apply suggestions from code review
Jul 12, 2024
5c31409
apply suggestions from code review
Jul 15, 2024
a4c6582
remove TODO comment
Jul 15, 2024
d73aa38
apply code review suggestions
Jul 18, 2024
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
3 changes: 2 additions & 1 deletion pkg/cluster/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/Azure/ARO-RP/pkg/util/oidcbuilder"
"github.com/Azure/ARO-RP/pkg/util/rbac"
"github.com/Azure/ARO-RP/pkg/util/stringutils"
utilwimi "github.com/Azure/ARO-RP/pkg/util/wimi"
)

// deleteNic deletes the network interface resource by first fetching the resource using the interface
Expand Down Expand Up @@ -443,7 +444,7 @@ func (m *manager) Delete(ctx context.Context) error {
return err
}

if m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile == nil && m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile != nil {
if utilwimi.IsWimi(m.doc.OpenShiftCluster) {
Copy link

@AldoFusterTurpin AldoFusterTurpin Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just a small comment: If we move the definition of the IsWimi function to the OpenShift cluster, we obtain a richer domain where functions are closer where the data is used (to avoid having an anemic Domain Model, a concept of DDD that here could be interesting).
If we do that (check next comments for details) this line would look like this:

if m.doc.OpenShiftCluster.IsWimi() {

which is pretty neat as clearly states that the fact that a cluster is Wimi or not just depend on the cluster and reads a bit better 🙂

m.log.Printf("deleting OIDC configuration")
blobContainerURL := oidcbuilder.GenerateBlobContainerURL(m.env)
azBlobClient, err := m.rpBlob.GetAZBlobClient(blobContainerURL, &azblob.ClientOptions{})
Expand Down
3 changes: 2 additions & 1 deletion pkg/cluster/deploybaseresources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/Azure/ARO-RP/pkg/util/oidcbuilder"
"github.com/Azure/ARO-RP/pkg/util/pointerutils"
"github.com/Azure/ARO-RP/pkg/util/stringutils"
utilwimi "github.com/Azure/ARO-RP/pkg/util/wimi"
)

var nsgNotReadyErrorRegex = regexp.MustCompile("Resource.*networkSecurityGroups.*referenced by resource.*not found")
Expand All @@ -39,7 +40,7 @@ func (m *manager) createDNS(ctx context.Context) error {
}

func (m *manager) createOIDC(ctx context.Context) error {
if m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile != nil || m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile == nil {
if !utilwimi.IsWimi(m.doc.OpenShiftCluster) {
Copy link

Choose a reason for hiding this comment

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

Same as before.

return nil
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/frontend/openshiftcluster_putorpatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/Azure/ARO-RP/pkg/frontend/middleware"
"github.com/Azure/ARO-RP/pkg/operator"
"github.com/Azure/ARO-RP/pkg/util/version"
utilwimi "github.com/Azure/ARO-RP/pkg/util/wimi"
)

var errMissingIdentityParameter error = fmt.Errorf("identity parameter not provided but required for workload identity cluster")
Expand Down Expand Up @@ -135,8 +136,7 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.
if isCreate {
// Persist identity URL and tenant ID only for managed/workload identity cluster create
// We don't support updating cluster managed identity after cluster creation
// TODO - use a common function to check if the cluster is a managed/workload identity cluster
if !(doc.OpenShiftCluster.Properties.ServicePrincipalProfile != nil || doc.OpenShiftCluster.Identity == nil) {
if utilwimi.IsWimi(doc.OpenShiftCluster) {
Copy link

Choose a reason for hiding this comment

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

Same as before.

if err := validateIdentityUrl(doc.OpenShiftCluster, putOrPatchClusterParameters.identityURL); err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/deploy/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/Azure/ARO-RP/pkg/util/pullsecret"
"github.com/Azure/ARO-RP/pkg/util/ready"
"github.com/Azure/ARO-RP/pkg/util/restconfig"
utilwimi "github.com/Azure/ARO-RP/pkg/util/wimi"
)

//go:embed staticresources
Expand Down Expand Up @@ -468,8 +469,7 @@ func (o *operator) RenewMDSDCertificate(ctx context.Context) error {
}

func (o *operator) EnsureUpgradeAnnotation(ctx context.Context) error {
if o.oc.Properties.PlatformWorkloadIdentityProfile == nil ||
o.oc.Properties.ServicePrincipalProfile != nil {
if !utilwimi.IsWimi(o.oc) {
Copy link

Choose a reason for hiding this comment

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

Same as before.

return nil
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/util/wimi/iswimi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package wimi

// Copyright (c) Microsoft Corporation.
// Licensed under the Apache License 2.0.

import "github.com/Azure/ARO-RP/pkg/api"

// IsWimi checks whether a cluster is a Workload Identity cluster or Service Principal cluster
func IsWimi(oc *api.OpenShiftCluster) bool {
if oc.Properties.PlatformWorkloadIdentityProfile != nil && oc.Properties.ServicePrincipalProfile == nil {
return true
}
return false
}
Comment on lines +8 to +14
Copy link

Choose a reason for hiding this comment

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

Following my first comment in the PR, if we want to have a richer domain and avoid an anemic domain model, we can move this function definition to pkg/api/openshiftcluster.go and have on line 29 something like:

// IsWimi checks whether a cluster is a Workload Identity cluster or Service Principal cluster
func (oc *OpenShiftCluster) IsWimi() bool {
	if oc.Properties.PlatformWorkloadIdentityProfile != nil && oc.Properties.ServicePrincipalProfile == nil {
		return true
	}
	return false
}

which moves the business logic closer to where the data lives in 🙂

Copy link

Choose a reason for hiding this comment

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

One last minor comment about it, we can simplify the function if we change it to one single return where the true condition should be met

// IsWimi checks whether a cluster is a Workload Identity cluster or Service Principal cluster
func (oc *OpenShiftCluster) IsWimi() bool {
	return oc.Properties.PlatformWorkloadIdentityProfile != nil && oc.Properties.ServicePrincipalProfile == nil
}

Just mentioning it in case you find it easier to read.

49 changes: 49 additions & 0 deletions pkg/util/wimi/iswimi_test.go
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Nice to see some unit tests 🙂

Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package wimi

// Copyright (c) Microsoft Corporation.
// Licensed under the Apache License 2.0.

import (
"fmt"
"testing"

"github.com/Azure/ARO-RP/pkg/api"
)

func TestIswimi(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suuuper NIT:- Since the function is named isWimi, this can be changed to TestIsWimi.
Please feel free to dismiss it or handle it accordingly. ```suggestion
func TestIsWimi(t *testing.T) {

tests := []*struct {
name string
oc api.OpenShiftCluster
want bool
}{
{
name: "Cluster is Workload Identity",
oc: api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{},
ServicePrincipalProfile: nil,
},
},
want: true,
},
{
name: "Cluster is Service Principal",
oc: api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
PlatformWorkloadIdentityProfile: nil,
ServicePrincipalProfile: &api.ServicePrincipalProfile{},
},
},
want: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := IsWimi(&test.oc)
if got != test.want {
t.Error(fmt.Errorf("got != want: %v != %v", got, test.want))
}
})
}
}
Loading