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

slawande2 · 2024-07-11T18:54:24Z

Which issue this PR addresses:

What this PR does / why we need it:

Throughout the RP codebase and installer wrapper, we are using if platformworkloadidentityprofile != nil or ServicePrincipalProfile to indicate whether or not a cluster is workload identity or not. This PR includes a shared utility function to check if a function is a Workload Identity or Service Principal cluster, which can be imported in all parts of the codebase.

Test plan for issue:

Passed all the unit test cases.

mociarain · 2024-07-12T06:47:01Z

/azp run ci

azure-pipelines · 2024-07-12T06:47:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mociarain

Nice

pkg/util/iswimi/iswimi.go

kimorris27

LGTM broadly speaking! I second Ayato's suggestions, and I have one other small change request to make sure some terminology usage is clear.

pkg/util/iswimi/iswimi.go

pkg/util/iswimi/iswimi_test.go

cadenmarchese

great work here!

pkg/util/iswimi/iswimi.go

pkg/cluster/deploybaseresources.go

pkg/frontend/openshiftcluster_putorpatch.go

pkg/operator/deploy/deploy.go

pkg/util/wimi/iswimi.go

pkg/util/wimi/iswimi_test.go

pkg/cluster/delete.go

pkg/frontend/openshiftcluster_putorpatch.go

kimorris27

Thanks for being flexible in the review process! LGTM as long as E2E passes.

kimorris27 · 2024-07-15T20:39:02Z

/azp run ci, e2e

azure-pipelines · 2024-07-15T20:39:15Z

Azure Pipelines successfully started running 2 pipeline(s).

pkg/util/wimi/iswimi.go

SudoBrendan

LGTM - feel free to action or dismiss my nit.

SudoBrendan · 2024-07-17T15:52:53Z

pkg/util/wimi/iswimi.go

+	if oc.Properties.PlatformWorkloadIdentityProfile == nil || oc.Properties.ServicePrincipalProfile != nil {
+		return false
+	}
+	return true


suuuuuper nit on style - please feel free to tell me to go away :) - if you discover you are using an if block to both return true and return false, you can return the boolean expression of the if itself.

For example, this if block and two returns is equivalent to:

return !(oc.Properties.PlatformWorkloadIdentityProfile == nil || oc.Properties.ServicePrincipalProfile != nil)

SudoBrendan · 2024-07-17T16:04:32Z

/azp run e2e

azure-pipelines · 2024-07-17T16:04:43Z

Azure Pipelines successfully started running 1 pipeline(s).

kimorris27 · 2024-07-17T20:28:48Z

/azp run e2e

azure-pipelines · 2024-07-17T20:28:58Z

Azure Pipelines successfully started running 1 pipeline(s).

kimorris27

Nice work here.

I hadn't thought about it during my own reviews, but I think Ayato's suggestion here makes sense if you feel inclined to make it before we merge. I don't consider it blocking though.

kimorris27 · 2024-07-18T20:14:11Z

/azp run ci, e2e

azure-pipelines · 2024-07-18T20:14:24Z

Azure Pipelines successfully started running 2 pipeline(s).

rajdeepc2792 · 2024-07-23T16:32:30Z

pkg/util/wimi/iswimi_test.go

+	"github.com/Azure/ARO-RP/pkg/api"
+)
+
+func TestIswimi(t *testing.T) {


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) {

AldoFusterTurpin

@slawande2 Thank you for this. LGTM! I just left an optional suggestion about moving the business logic closer to the struct where the data lives in to avoid having an anemic domain. No blocker at all, just a FYI 🙂

AldoFusterTurpin · 2024-07-23T16:50:52Z

pkg/cluster/delete.go

@@ -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) {


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 🙂

AldoFusterTurpin · 2024-07-23T16:51:01Z

pkg/cluster/deploybaseresources.go

@@ -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) {


Same as before.

AldoFusterTurpin · 2024-07-23T16:51:11Z

pkg/frontend/openshiftcluster_putorpatch.go

@@ -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) {


Same as before.

AldoFusterTurpin · 2024-07-23T16:51:22Z

pkg/operator/deploy/deploy.go

@@ -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) {


Same as before.

AldoFusterTurpin · 2024-07-23T16:53:12Z

pkg/util/wimi/iswimi.go

+// 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
+}


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 🙂

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.

AldoFusterTurpin · 2024-07-23T16:53:56Z

pkg/util/wimi/iswimi_test.go

Nice to see some unit tests 🙂

cadenmarchese · 2024-07-23T19:53:00Z

@AldoFusterTurpin thanks a lot for the suggestion! We are going to implement it in a new PR.

AldoFusterTurpin · 2024-07-25T07:35:46Z

@AldoFusterTurpin thanks a lot for the suggestion! We are going to implement it in a new PR.

Great! I see that PR is now open: #3721

Thanks!

shared utility function to check if a cluster is workload identity

e794516

slawande2 added the chainsaw Pull requests or issues owned by Team Chainsaw label Jul 11, 2024

adding license statement

03f22f0

mociarain previously approved these changes Jul 12, 2024

View reviewed changes

bitoku reviewed Jul 12, 2024

View reviewed changes

pkg/util/iswimi/iswimi.go Outdated Show resolved Hide resolved

pkg/util/iswimi/iswimi.go Outdated Show resolved Hide resolved

kimorris27 requested changes Jul 12, 2024

View reviewed changes

pkg/util/iswimi/iswimi.go Outdated Show resolved Hide resolved

pkg/util/iswimi/iswimi_test.go Outdated Show resolved Hide resolved

cadenmarchese reviewed Jul 12, 2024

View reviewed changes

pkg/util/iswimi/iswimi.go Outdated Show resolved Hide resolved

pkg/util/iswimi/iswimi.go Outdated Show resolved Hide resolved

apply suggestions from code review

1c37593

cadenmarchese reviewed Jul 15, 2024

View reviewed changes

apply suggestions from code review

5c31409

kimorris27 reviewed Jul 15, 2024

View reviewed changes

pkg/frontend/openshiftcluster_putorpatch.go Outdated Show resolved Hide resolved

remove TODO comment

a4c6582

kimorris27 reviewed Jul 15, 2024

View reviewed changes

bitoku reviewed Jul 17, 2024

View reviewed changes

pkg/util/wimi/iswimi.go Outdated Show resolved Hide resolved

SudoBrendan previously approved these changes Jul 17, 2024

View reviewed changes

slawande2 requested a review from kimorris27 July 17, 2024 16:42

kimorris27 previously approved these changes Jul 18, 2024

View reviewed changes

slawande2 dismissed stale reviews from kimorris27 and SudoBrendan via d73aa38 July 18, 2024 20:11

apply code review suggestions

d73aa38

bitoku approved these changes Jul 19, 2024

View reviewed changes

kimorris27 approved these changes Jul 19, 2024

View reviewed changes

cadenmarchese approved these changes Jul 22, 2024

View reviewed changes

rajdeepc2792 reviewed Jul 23, 2024

View reviewed changes

AldoFusterTurpin approved these changes Jul 23, 2024

View reviewed changes

cadenmarchese merged commit 53691de into master Jul 23, 2024
21 checks passed

SudoBrendan deleted the slawande/iswimi branch July 24, 2024 15:51

slawande2 mentioned this pull request Jul 24, 2024

move iswimi function to openshiftcluster.go #3721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

slawande2 commented Jul 11, 2024 •

edited

Loading

mociarain commented Jul 12, 2024

azure-pipelines bot commented Jul 12, 2024

mociarain left a comment

kimorris27 left a comment

cadenmarchese left a comment

kimorris27 left a comment

kimorris27 commented Jul 15, 2024

azure-pipelines bot commented Jul 15, 2024

SudoBrendan left a comment

SudoBrendan Jul 17, 2024

SudoBrendan commented Jul 17, 2024

azure-pipelines bot commented Jul 17, 2024

kimorris27 commented Jul 17, 2024

azure-pipelines bot commented Jul 17, 2024

kimorris27 left a comment

kimorris27 commented Jul 18, 2024

azure-pipelines bot commented Jul 18, 2024

rajdeepc2792 Jul 23, 2024

AldoFusterTurpin left a comment

AldoFusterTurpin Jul 23, 2024 •

edited

Loading

AldoFusterTurpin Jul 23, 2024

AldoFusterTurpin Jul 23, 2024

AldoFusterTurpin Jul 23, 2024

AldoFusterTurpin Jul 23, 2024

AldoFusterTurpin Jul 23, 2024

AldoFusterTurpin Jul 23, 2024

cadenmarchese commented Jul 23, 2024

AldoFusterTurpin commented Jul 25, 2024 •

edited

Loading

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

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

Conversation

slawande2 commented Jul 11, 2024 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

mociarain commented Jul 12, 2024

azure-pipelines bot commented Jul 12, 2024

mociarain left a comment

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

cadenmarchese left a comment

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

kimorris27 commented Jul 15, 2024

azure-pipelines bot commented Jul 15, 2024

SudoBrendan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBrendan commented Jul 17, 2024

azure-pipelines bot commented Jul 17, 2024

kimorris27 commented Jul 17, 2024

azure-pipelines bot commented Jul 17, 2024

kimorris27 left a comment

Choose a reason for hiding this comment

kimorris27 commented Jul 18, 2024

azure-pipelines bot commented Jul 18, 2024

Choose a reason for hiding this comment

AldoFusterTurpin left a comment

Choose a reason for hiding this comment

AldoFusterTurpin Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadenmarchese commented Jul 23, 2024

AldoFusterTurpin commented Jul 25, 2024 • edited Loading

slawande2 commented Jul 11, 2024 •

edited

Loading

AldoFusterTurpin Jul 23, 2024 •

edited

Loading

AldoFusterTurpin commented Jul 25, 2024 •

edited

Loading