From 7ee903f81c4e49b1c6a9cee68efbcb14e5744f3a Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 10:36:50 -0700 Subject: [PATCH 01/13] Add tenant ID to Identity struct --- pkg/api/openshiftcluster.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/api/openshiftcluster.go b/pkg/api/openshiftcluster.go index caa157d71b8..0effbf3f35f 100644 --- a/pkg/api/openshiftcluster.go +++ b/pkg/api/openshiftcluster.go @@ -808,4 +808,5 @@ type Identity struct { Type string `json:"type,omitempty"` UserAssignedIdentities UserAssignedIdentities `json:"userAssignedIdentities,omitempty"` IdentityURL string `json:"identityURL,omitempty" mutable:"true"` + TenantID string `json:"tenantId,omitempty" mutable:"true"` } From 401380f572769f77677fbd07394d7249e43516d9 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 11:01:17 -0700 Subject: [PATCH 02/13] fix put/patch --- pkg/frontend/openshiftcluster_putorpatch.go | 47 +++++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 5c630897853..69dbe7bfe76 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -25,7 +25,7 @@ import ( "github.com/Azure/ARO-RP/pkg/util/version" ) -var errMissingIdentityURL error = fmt.Errorf("identityURL not provided but required for workload identity cluster") +var errMissingIdentityParmeter error = fmt.Errorf("identity parameter not provided but required for workload identity cluster") func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -44,11 +44,12 @@ func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Req resourceProviderNamespace := chi.URLParam(r, "resourceProviderNamespace") identityURL := r.Header.Get("x-ms-identity-url") + identityTenantID := r.Header.Get("x-ms-home-tenant-id") apiVersion := r.URL.Query().Get(api.APIVersionKey) err := cosmosdb.RetryOnPreconditionFailed(func() error { var err error - b, err = f._putOrPatchOpenShiftCluster(ctx, log, body, correlationData, systemData, r.URL.Path, originalPath, r.Method, referer, &header, f.apis[apiVersion].OpenShiftClusterConverter, f.apis[apiVersion].OpenShiftClusterStaticValidator, subId, resourceProviderNamespace, apiVersion, identityURL) + b, err = f._putOrPatchOpenShiftCluster(ctx, log, body, correlationData, systemData, r.URL.Path, originalPath, r.Method, referer, &header, f.apis[apiVersion].OpenShiftClusterConverter, f.apis[apiVersion].OpenShiftClusterStaticValidator, subId, resourceProviderNamespace, apiVersion, identityURL, identityTenantID) return err }) @@ -56,7 +57,8 @@ func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Req reply(log, w, header, b, err) } -func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.Entry, body []byte, correlationData *api.CorrelationData, systemData *api.SystemData, path, originalPath, method, referer string, header *http.Header, converter api.OpenShiftClusterConverter, staticValidator api.OpenShiftClusterStaticValidator, subId, resourceProviderNamespace string, apiVersion string, identityURL string) ([]byte, error) { +// TODO - refactor this function to reduce the number of parameters +func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.Entry, body []byte, correlationData *api.CorrelationData, systemData *api.SystemData, path, originalPath, method, referer string, header *http.Header, converter api.OpenShiftClusterConverter, staticValidator api.OpenShiftClusterStaticValidator, subId, resourceProviderNamespace string, apiVersion string, identityURL string, identityTenantID string) ([]byte, error) { subscription, err := f.validateSubscriptionState(ctx, path, api.SubscriptionStateRegistered) if err != nil { return nil, err @@ -96,9 +98,14 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } } - err = validateIdentityUrl(doc.OpenShiftCluster, identityURL, isCreate) - if err != nil { - return nil, err + // Don't persist identity parameters in non-wimi clusters + if doc.OpenShiftCluster.Properties.ServicePrincipalProfile == nil || doc.OpenShiftCluster.Identity != nil { + if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL, isCreate); err != nil { + return nil, err + } + if err := validateIdentityTenantID(doc.OpenShiftCluster, identityTenantID, isCreate); err != nil { + return nil, err + } } doc.CorrelationData = correlationData @@ -299,20 +306,32 @@ func enrichClusterSystemData(doc *api.OpenShiftClusterDocument, systemData *api. } func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string, isCreate bool) error { - // Don't persist identity URL in non-wimi clusters - if cluster.Properties.ServicePrincipalProfile != nil || cluster.Identity == nil { - return nil + if err := validateIdentityParam(cluster, identityURL, isCreate); err != nil { + return fmt.Errorf("%w: %s", err, "identity URL") } - if identityURL == "" { + cluster.Identity.IdentityURL = identityURL + + return nil +} + +func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID string, isCreate bool) error { + if err := validateIdentityParam(cluster, identityTenantID, isCreate); err != nil { + return fmt.Errorf("%w: %s", err, "identity tenant ID") + } + + cluster.Identity.TenantID = identityTenantID + + return nil +} + +func validateIdentityParam(cluster *api.OpenShiftCluster, param string, isCreate bool) error { + if param == "" { if isCreate { - return errMissingIdentityURL + return errMissingIdentityParmeter } - return nil } - cluster.Identity.IdentityURL = identityURL - return nil } From d76314eb97962963ae5d2395ade0884a44117a47 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 11:13:42 -0700 Subject: [PATCH 03/13] Add unit tests --- pkg/frontend/openshiftcluster_putorpatch.go | 6 +- .../openshiftcluster_putorpatch_test.go | 119 +++++++++++++----- 2 files changed, 92 insertions(+), 33 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 69dbe7bfe76..d3a23189972 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -306,7 +306,7 @@ func enrichClusterSystemData(doc *api.OpenShiftClusterDocument, systemData *api. } func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string, isCreate bool) error { - if err := validateIdentityParam(cluster, identityURL, isCreate); err != nil { + if err := validateIdentityParam(identityURL, isCreate); err != nil { return fmt.Errorf("%w: %s", err, "identity URL") } @@ -316,7 +316,7 @@ func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string, isCr } func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID string, isCreate bool) error { - if err := validateIdentityParam(cluster, identityTenantID, isCreate); err != nil { + if err := validateIdentityParam(identityTenantID, isCreate); err != nil { return fmt.Errorf("%w: %s", err, "identity tenant ID") } @@ -325,7 +325,7 @@ func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID st return nil } -func validateIdentityParam(cluster *api.OpenShiftCluster, param string, isCreate bool) error { +func validateIdentityParam(param string, isCreate bool) error { if param == "" { if isCreate { return errMissingIdentityParmeter diff --git a/pkg/frontend/openshiftcluster_putorpatch_test.go b/pkg/frontend/openshiftcluster_putorpatch_test.go index 15ee5181237..3217dbdb92e 100644 --- a/pkg/frontend/openshiftcluster_putorpatch_test.go +++ b/pkg/frontend/openshiftcluster_putorpatch_test.go @@ -6,6 +6,7 @@ package frontend import ( "context" "encoding/json" + "errors" "fmt" "net/http" "reflect" @@ -3316,67 +3317,90 @@ func TestValidateIdentityUrl(t *testing.T) { wantError error }{ { - name: "identity URL is empty, is not wi/mi cluster create", + name: "identity URL is empty, is wi/mi cluster create", identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - isCreate: false, + isCreate: true, + wantError: errMissingIdentityParmeter, }, { - name: "identity URL is empty, is wi/mi cluster create", + name: "identity URL is empty, is not wi/mi cluster create", identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - isCreate: true, - wantError: errMissingIdentityURL, + isCreate: false, + wantError: nil, }, { - name: "cluster is not wi/mi, identityURL passed", - identityURL: "http://foo.bar", + name: "pass - identity URL passed on wi/mi cluster", cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, - }, + Identity: &api.Identity{}, }, + identityURL: "http://foo.bar", expected: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, + Identity: &api.Identity{ + IdentityURL: "http://foo.bar", }, }, isCreate: true, }, + } { + t.Run(tt.name, func(t *testing.T) { + err := validateIdentityUrl(tt.cluster, tt.identityURL, tt.isCreate) + if !errors.Is(err, tt.wantError) { + t.Error(cmp.Diff(err, tt.wantError)) + } + + if !reflect.DeepEqual(tt.cluster, tt.expected) { + t.Error(cmp.Diff(tt.cluster, tt.expected)) + } + }) + } +} + +func TestValidateIdentityTenantID(t *testing.T) { + for _, tt := range []struct { + name string + tenantID string + cluster *api.OpenShiftCluster + expected *api.OpenShiftCluster + isCreate bool + wantError error + }{ + { + name: "tenantID is empty, is wi/mi cluster create", + tenantID: "", + cluster: &api.OpenShiftCluster{}, + expected: &api.OpenShiftCluster{}, + isCreate: true, + wantError: errMissingIdentityParmeter, + }, { - name: "cluster is not wi/mi, identityURL not passed", - identityURL: "", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, - }, - }, - expected: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, - }, - }, - isCreate: true, + name: "tenantID is empty, is not wi/mi cluster create", + tenantID: "", + cluster: &api.OpenShiftCluster{}, + expected: &api.OpenShiftCluster{}, + isCreate: false, + wantError: nil, }, { - name: "pass - identity URL passed on wi/mi cluster", + name: "pass - tenantID passed on wi/mi cluster", cluster: &api.OpenShiftCluster{ Identity: &api.Identity{}, }, - identityURL: "http://foo.bar", + tenantID: "bogus", expected: &api.OpenShiftCluster{ Identity: &api.Identity{ - IdentityURL: "http://foo.bar", + TenantID: "bogus", }, }, isCreate: true, }, } { t.Run(tt.name, func(t *testing.T) { - err := validateIdentityUrl(tt.cluster, tt.identityURL, tt.isCreate) - if err != nil && err != tt.wantError { + err := validateIdentityTenantID(tt.cluster, tt.tenantID, tt.isCreate) + if !errors.Is(err, tt.wantError) { t.Error(cmp.Diff(err, tt.wantError)) } @@ -3386,3 +3410,38 @@ func TestValidateIdentityUrl(t *testing.T) { }) } } + +func TestValidateIdentityParam(t *testing.T) { + for _, tt := range []struct { + name string + param string + isCreate bool + wantError error + }{ + { + name: "param is empty, is wi/mi cluster create", + param: "", + isCreate: true, + wantError: errMissingIdentityParmeter, + }, + { + name: "param is empty, is not wi/mi cluster create", + param: "", + isCreate: false, + wantError: nil, + }, + { + name: "pass - param passed on wi/mi cluster", + param: "bogus", + isCreate: true, + wantError: nil, + }, + } { + t.Run(tt.name, func(t *testing.T) { + err := validateIdentityParam(tt.param, tt.isCreate) + if !errors.Is(err, tt.wantError) { + t.Error(cmp.Diff(err, tt.wantError)) + } + }) + } +} From a0a5d22a0a05c1577b4eb3690139bb6ff4999e1e Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 11:31:36 -0700 Subject: [PATCH 04/13] Further refactor the code --- pkg/frontend/openshiftcluster_putorpatch.go | 34 ++++----- .../openshiftcluster_putorpatch_test.go | 69 ++----------------- 2 files changed, 19 insertions(+), 84 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index d3a23189972..605bc7faaa6 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -100,11 +100,13 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. // Don't persist identity parameters in non-wimi clusters if doc.OpenShiftCluster.Properties.ServicePrincipalProfile == nil || doc.OpenShiftCluster.Identity != nil { - if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL, isCreate); err != nil { - return nil, err - } - if err := validateIdentityTenantID(doc.OpenShiftCluster, identityTenantID, isCreate); err != nil { - return nil, err + if isCreate { + if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL); err != nil { + return nil, err + } + if err := validateIdentityTenantID(doc.OpenShiftCluster, identityTenantID); err != nil { + return nil, err + } } } @@ -305,9 +307,9 @@ func enrichClusterSystemData(doc *api.OpenShiftClusterDocument, systemData *api. } } -func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string, isCreate bool) error { - if err := validateIdentityParam(identityURL, isCreate); err != nil { - return fmt.Errorf("%w: %s", err, "identity URL") +func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string) error { + if identityURL == "" { + return fmt.Errorf("%w: %s", errMissingIdentityParmeter, "identity URL") } cluster.Identity.IdentityURL = identityURL @@ -315,9 +317,9 @@ func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string, isCr return nil } -func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID string, isCreate bool) error { - if err := validateIdentityParam(identityTenantID, isCreate); err != nil { - return fmt.Errorf("%w: %s", err, "identity tenant ID") +func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID string) error { + if identityTenantID == "" { + return fmt.Errorf("%w: %s", errMissingIdentityParmeter, "identity tenant ID") } cluster.Identity.TenantID = identityTenantID @@ -325,16 +327,6 @@ func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID st return nil } -func validateIdentityParam(param string, isCreate bool) error { - if param == "" { - if isCreate { - return errMissingIdentityParmeter - } - } - - return nil -} - func (f *frontend) ValidateNewCluster(ctx context.Context, subscription *api.SubscriptionDocument, cluster *api.OpenShiftCluster, staticValidator api.OpenShiftClusterStaticValidator, ext interface{}, path string) error { err := staticValidator.Static(ext, nil, f.env.Location(), f.env.Domain(), f.env.FeatureIsSet(env.FeatureRequireD2sV3Workers), path) if err != nil { diff --git a/pkg/frontend/openshiftcluster_putorpatch_test.go b/pkg/frontend/openshiftcluster_putorpatch_test.go index 3217dbdb92e..5e26e7f26c5 100644 --- a/pkg/frontend/openshiftcluster_putorpatch_test.go +++ b/pkg/frontend/openshiftcluster_putorpatch_test.go @@ -3313,27 +3313,17 @@ func TestValidateIdentityUrl(t *testing.T) { identityURL string cluster *api.OpenShiftCluster expected *api.OpenShiftCluster - isCreate bool wantError error }{ { - name: "identity URL is empty, is wi/mi cluster create", + name: "identity URL is empty", identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - isCreate: true, wantError: errMissingIdentityParmeter, }, { - name: "identity URL is empty, is not wi/mi cluster create", - identityURL: "", - cluster: &api.OpenShiftCluster{}, - expected: &api.OpenShiftCluster{}, - isCreate: false, - wantError: nil, - }, - { - name: "pass - identity URL passed on wi/mi cluster", + name: "pass - identity URL passed", cluster: &api.OpenShiftCluster{ Identity: &api.Identity{}, }, @@ -3343,11 +3333,10 @@ func TestValidateIdentityUrl(t *testing.T) { IdentityURL: "http://foo.bar", }, }, - isCreate: true, }, } { t.Run(tt.name, func(t *testing.T) { - err := validateIdentityUrl(tt.cluster, tt.identityURL, tt.isCreate) + err := validateIdentityUrl(tt.cluster, tt.identityURL) if !errors.Is(err, tt.wantError) { t.Error(cmp.Diff(err, tt.wantError)) } @@ -3365,27 +3354,17 @@ func TestValidateIdentityTenantID(t *testing.T) { tenantID string cluster *api.OpenShiftCluster expected *api.OpenShiftCluster - isCreate bool wantError error }{ { - name: "tenantID is empty, is wi/mi cluster create", + name: "tenantID is empty", tenantID: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - isCreate: true, wantError: errMissingIdentityParmeter, }, { - name: "tenantID is empty, is not wi/mi cluster create", - tenantID: "", - cluster: &api.OpenShiftCluster{}, - expected: &api.OpenShiftCluster{}, - isCreate: false, - wantError: nil, - }, - { - name: "pass - tenantID passed on wi/mi cluster", + name: "pass - tenantID passed", cluster: &api.OpenShiftCluster{ Identity: &api.Identity{}, }, @@ -3395,11 +3374,10 @@ func TestValidateIdentityTenantID(t *testing.T) { TenantID: "bogus", }, }, - isCreate: true, }, } { t.Run(tt.name, func(t *testing.T) { - err := validateIdentityTenantID(tt.cluster, tt.tenantID, tt.isCreate) + err := validateIdentityTenantID(tt.cluster, tt.tenantID) if !errors.Is(err, tt.wantError) { t.Error(cmp.Diff(err, tt.wantError)) } @@ -3410,38 +3388,3 @@ func TestValidateIdentityTenantID(t *testing.T) { }) } } - -func TestValidateIdentityParam(t *testing.T) { - for _, tt := range []struct { - name string - param string - isCreate bool - wantError error - }{ - { - name: "param is empty, is wi/mi cluster create", - param: "", - isCreate: true, - wantError: errMissingIdentityParmeter, - }, - { - name: "param is empty, is not wi/mi cluster create", - param: "", - isCreate: false, - wantError: nil, - }, - { - name: "pass - param passed on wi/mi cluster", - param: "bogus", - isCreate: true, - wantError: nil, - }, - } { - t.Run(tt.name, func(t *testing.T) { - err := validateIdentityParam(tt.param, tt.isCreate) - if !errors.Is(err, tt.wantError) { - t.Error(cmp.Diff(err, tt.wantError)) - } - }) - } -} From 56ed7ae2bbee53075f3487429080fa7c733730c0 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 14:27:58 -0700 Subject: [PATCH 05/13] fix error naming --- pkg/frontend/openshiftcluster_putorpatch.go | 6 +++--- pkg/frontend/openshiftcluster_putorpatch_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 605bc7faaa6..0291df73bec 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -25,7 +25,7 @@ import ( "github.com/Azure/ARO-RP/pkg/util/version" ) -var errMissingIdentityParmeter error = fmt.Errorf("identity parameter not provided but required for workload identity cluster") +var errMissingIdentityParameter error = fmt.Errorf("identity parameter not provided but required for workload identity cluster") func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -309,7 +309,7 @@ func enrichClusterSystemData(doc *api.OpenShiftClusterDocument, systemData *api. func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string) error { if identityURL == "" { - return fmt.Errorf("%w: %s", errMissingIdentityParmeter, "identity URL") + return fmt.Errorf("%w: %s", errMissingIdentityParameter, "identity URL") } cluster.Identity.IdentityURL = identityURL @@ -319,7 +319,7 @@ func validateIdentityUrl(cluster *api.OpenShiftCluster, identityURL string) erro func validateIdentityTenantID(cluster *api.OpenShiftCluster, identityTenantID string) error { if identityTenantID == "" { - return fmt.Errorf("%w: %s", errMissingIdentityParmeter, "identity tenant ID") + return fmt.Errorf("%w: %s", errMissingIdentityParameter, "identity tenant ID") } cluster.Identity.TenantID = identityTenantID diff --git a/pkg/frontend/openshiftcluster_putorpatch_test.go b/pkg/frontend/openshiftcluster_putorpatch_test.go index 5e26e7f26c5..acc6eafbacc 100644 --- a/pkg/frontend/openshiftcluster_putorpatch_test.go +++ b/pkg/frontend/openshiftcluster_putorpatch_test.go @@ -3320,7 +3320,7 @@ func TestValidateIdentityUrl(t *testing.T) { identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - wantError: errMissingIdentityParmeter, + wantError: errMissingIdentityParameter, }, { name: "pass - identity URL passed", @@ -3361,7 +3361,7 @@ func TestValidateIdentityTenantID(t *testing.T) { tenantID: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, - wantError: errMissingIdentityParmeter, + wantError: errMissingIdentityParameter, }, { name: "pass - tenantID passed", From 681810f483c005534e88836b2a39ffeb4d6c2e78 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 15:08:57 -0700 Subject: [PATCH 06/13] Add util for checking miwi cluster --- pkg/api/util/identity/identity.go | 11 ++++++ pkg/api/util/identity/identity_test.go | 52 ++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 pkg/api/util/identity/identity.go create mode 100644 pkg/api/util/identity/identity_test.go diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go new file mode 100644 index 00000000000..a3d30d9e59f --- /dev/null +++ b/pkg/api/util/identity/identity.go @@ -0,0 +1,11 @@ +package identity + +import "github.com/Azure/ARO-RP/pkg/api" + +func IsManagedIdentityCluster(cluster *api.OpenShiftCluster) bool { + if cluster.Properties.ServicePrincipalProfile == nil && cluster.Properties.PlatformWorkloadIdentityProfile != nil { + return true + } + + return false +} diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go new file mode 100644 index 00000000000..68ebf15d20b --- /dev/null +++ b/pkg/api/util/identity/identity_test.go @@ -0,0 +1,52 @@ +package identity + +import ( + "testing" + + "github.com/Azure/ARO-RP/pkg/api" +) + +func TestIsManagedIdentityCluster(t *testing.T) { + tests := []struct { + name string + cluster *api.OpenShiftCluster + expected bool + }{ + { + name: "Managed Identity Cluster", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: nil, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + }, + expected: true, + }, + { + name: "Non-Managed Identity Cluster", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: &api.ServicePrincipalProfile{}, + PlatformWorkloadIdentityProfile: nil, + }, + }, + expected: false, + }, + { + name: "Nil Properties", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsManagedIdentityCluster(tt.cluster) + if result != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, result) + } + }) + } +} From 2c53a3157b250426da1558734927bc32a5eeb373 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 15:26:22 -0700 Subject: [PATCH 07/13] Revert "Add util for checking miwi cluster" This reverts commit 681810f483c005534e88836b2a39ffeb4d6c2e78. --- pkg/api/util/identity/identity.go | 11 ------ pkg/api/util/identity/identity_test.go | 52 -------------------------- 2 files changed, 63 deletions(-) delete mode 100644 pkg/api/util/identity/identity.go delete mode 100644 pkg/api/util/identity/identity_test.go diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go deleted file mode 100644 index a3d30d9e59f..00000000000 --- a/pkg/api/util/identity/identity.go +++ /dev/null @@ -1,11 +0,0 @@ -package identity - -import "github.com/Azure/ARO-RP/pkg/api" - -func IsManagedIdentityCluster(cluster *api.OpenShiftCluster) bool { - if cluster.Properties.ServicePrincipalProfile == nil && cluster.Properties.PlatformWorkloadIdentityProfile != nil { - return true - } - - return false -} diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go deleted file mode 100644 index 68ebf15d20b..00000000000 --- a/pkg/api/util/identity/identity_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package identity - -import ( - "testing" - - "github.com/Azure/ARO-RP/pkg/api" -) - -func TestIsManagedIdentityCluster(t *testing.T) { - tests := []struct { - name string - cluster *api.OpenShiftCluster - expected bool - }{ - { - name: "Managed Identity Cluster", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: nil, - PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, - }, - }, - expected: true, - }, - { - name: "Non-Managed Identity Cluster", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, - PlatformWorkloadIdentityProfile: nil, - }, - }, - expected: false, - }, - { - name: "Nil Properties", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{}, - }, - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsManagedIdentityCluster(tt.cluster) - if result != tt.expected { - t.Errorf("expected %v, got %v", tt.expected, result) - } - }) - } -} From 524b396b728578380254061668f653a51a567476 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 15:52:46 -0700 Subject: [PATCH 08/13] Add function to test if miwi enabled --- pkg/api/util/identity/identity.go | 13 ++++ pkg/api/util/identity/identity_test.go | 69 +++++++++++++++++++++ pkg/frontend/openshiftcluster_putorpatch.go | 4 +- 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 pkg/api/util/identity/identity.go create mode 100644 pkg/api/util/identity/identity_test.go diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go new file mode 100644 index 00000000000..79ec76bf24d --- /dev/null +++ b/pkg/api/util/identity/identity.go @@ -0,0 +1,13 @@ +package identity + +import ( + "github.com/Azure/ARO-RP/pkg/api" +) + +func IsManagedWorkloadIdentityEnabled(cluster *api.OpenShiftCluster) bool { + if cluster.Properties.ServicePrincipalProfile == nil && cluster.Properties.PlatformWorkloadIdentityProfile != nil && cluster.Identity != nil { + return true + } + + return false +} diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go new file mode 100644 index 00000000000..735d940c48d --- /dev/null +++ b/pkg/api/util/identity/identity_test.go @@ -0,0 +1,69 @@ +package identity + +import ( + "testing" + + "github.com/Azure/ARO-RP/pkg/api" +) + +func TestIsManagedWorkloadIdentityEnabled(t *testing.T) { + tests := []struct { + name string + cluster *api.OpenShiftCluster + expected bool + }{ + { + name: "Workload Identity Enabled", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: nil, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + Identity: &api.Identity{}, + }, + expected: true, + }, + { + name: "Service Principal Profile not nil", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: &api.ServicePrincipalProfile{}, + PlatformWorkloadIdentityProfile: nil, + }, + Identity: nil, + }, + expected: false, + }, + { + name: "PlatformWorkloadIdentityProfile is nil", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: nil, + PlatformWorkloadIdentityProfile: nil, + }, + Identity: &api.Identity{}, + }, + expected: false, + }, + { + name: "Identity is nil", + cluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: nil, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + Identity: nil, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsManagedWorkloadIdentityEnabled(tt.cluster) + if result != tt.expected { + t.Errorf("expected %t, got %t", tt.expected, result) + } + }) + } +} diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 0291df73bec..61f76109460 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -18,6 +18,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/api/admin" + "github.com/Azure/ARO-RP/pkg/api/util/identity" "github.com/Azure/ARO-RP/pkg/database/cosmosdb" "github.com/Azure/ARO-RP/pkg/env" "github.com/Azure/ARO-RP/pkg/frontend/middleware" @@ -99,7 +100,8 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } // Don't persist identity parameters in non-wimi clusters - if doc.OpenShiftCluster.Properties.ServicePrincipalProfile == nil || doc.OpenShiftCluster.Identity != nil { + if identity.IsManagedWorkloadIdentityEnabled(doc.OpenShiftCluster) { + // We don't support changing the cluster MSI, so only need to validate/apply on create if isCreate { if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL); err != nil { return nil, err From 0cdcd00c93f0f71d430150842f1378ce08354c90 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Fri, 28 Jun 2024 15:56:48 -0700 Subject: [PATCH 09/13] Add license --- pkg/api/util/identity/identity.go | 3 +++ pkg/api/util/identity/identity_test.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go index 79ec76bf24d..93cdc605caf 100644 --- a/pkg/api/util/identity/identity.go +++ b/pkg/api/util/identity/identity.go @@ -1,5 +1,8 @@ package identity +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + import ( "github.com/Azure/ARO-RP/pkg/api" ) diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go index 735d940c48d..ae062f95455 100644 --- a/pkg/api/util/identity/identity_test.go +++ b/pkg/api/util/identity/identity_test.go @@ -1,5 +1,8 @@ package identity +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + import ( "testing" From 0c840e3977a672c4935f43efbbaed9a7ab3caf00 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Mon, 1 Jul 2024 07:51:16 -0700 Subject: [PATCH 10/13] Revert "Add license" This reverts commit 0cdcd00c93f0f71d430150842f1378ce08354c90. --- pkg/api/util/identity/identity.go | 3 --- pkg/api/util/identity/identity_test.go | 3 --- 2 files changed, 6 deletions(-) diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go index 93cdc605caf..79ec76bf24d 100644 --- a/pkg/api/util/identity/identity.go +++ b/pkg/api/util/identity/identity.go @@ -1,8 +1,5 @@ package identity -// Copyright (c) Microsoft Corporation. -// Licensed under the Apache License 2.0. - import ( "github.com/Azure/ARO-RP/pkg/api" ) diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go index ae062f95455..735d940c48d 100644 --- a/pkg/api/util/identity/identity_test.go +++ b/pkg/api/util/identity/identity_test.go @@ -1,8 +1,5 @@ package identity -// Copyright (c) Microsoft Corporation. -// Licensed under the Apache License 2.0. - import ( "testing" From ecbc28d7d0b979ebdbada345e966e1958186a860 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Mon, 1 Jul 2024 07:51:27 -0700 Subject: [PATCH 11/13] Revert "Add function to test if miwi enabled" This reverts commit 524b396b728578380254061668f653a51a567476. --- pkg/api/util/identity/identity.go | 13 ---- pkg/api/util/identity/identity_test.go | 69 --------------------- pkg/frontend/openshiftcluster_putorpatch.go | 4 +- 3 files changed, 1 insertion(+), 85 deletions(-) delete mode 100644 pkg/api/util/identity/identity.go delete mode 100644 pkg/api/util/identity/identity_test.go diff --git a/pkg/api/util/identity/identity.go b/pkg/api/util/identity/identity.go deleted file mode 100644 index 79ec76bf24d..00000000000 --- a/pkg/api/util/identity/identity.go +++ /dev/null @@ -1,13 +0,0 @@ -package identity - -import ( - "github.com/Azure/ARO-RP/pkg/api" -) - -func IsManagedWorkloadIdentityEnabled(cluster *api.OpenShiftCluster) bool { - if cluster.Properties.ServicePrincipalProfile == nil && cluster.Properties.PlatformWorkloadIdentityProfile != nil && cluster.Identity != nil { - return true - } - - return false -} diff --git a/pkg/api/util/identity/identity_test.go b/pkg/api/util/identity/identity_test.go deleted file mode 100644 index 735d940c48d..00000000000 --- a/pkg/api/util/identity/identity_test.go +++ /dev/null @@ -1,69 +0,0 @@ -package identity - -import ( - "testing" - - "github.com/Azure/ARO-RP/pkg/api" -) - -func TestIsManagedWorkloadIdentityEnabled(t *testing.T) { - tests := []struct { - name string - cluster *api.OpenShiftCluster - expected bool - }{ - { - name: "Workload Identity Enabled", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: nil, - PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, - }, - Identity: &api.Identity{}, - }, - expected: true, - }, - { - name: "Service Principal Profile not nil", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: &api.ServicePrincipalProfile{}, - PlatformWorkloadIdentityProfile: nil, - }, - Identity: nil, - }, - expected: false, - }, - { - name: "PlatformWorkloadIdentityProfile is nil", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: nil, - PlatformWorkloadIdentityProfile: nil, - }, - Identity: &api.Identity{}, - }, - expected: false, - }, - { - name: "Identity is nil", - cluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: nil, - PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, - }, - Identity: nil, - }, - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsManagedWorkloadIdentityEnabled(tt.cluster) - if result != tt.expected { - t.Errorf("expected %t, got %t", tt.expected, result) - } - }) - } -} diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 61f76109460..0291df73bec 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -18,7 +18,6 @@ import ( "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/api/admin" - "github.com/Azure/ARO-RP/pkg/api/util/identity" "github.com/Azure/ARO-RP/pkg/database/cosmosdb" "github.com/Azure/ARO-RP/pkg/env" "github.com/Azure/ARO-RP/pkg/frontend/middleware" @@ -100,8 +99,7 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } // Don't persist identity parameters in non-wimi clusters - if identity.IsManagedWorkloadIdentityEnabled(doc.OpenShiftCluster) { - // We don't support changing the cluster MSI, so only need to validate/apply on create + if doc.OpenShiftCluster.Properties.ServicePrincipalProfile == nil || doc.OpenShiftCluster.Identity != nil { if isCreate { if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL); err != nil { return nil, err From 8fa599f5548382094f9778c7221b385fbb03f741 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Mon, 1 Jul 2024 07:53:04 -0700 Subject: [PATCH 12/13] Add todo for shared function --- pkg/frontend/openshiftcluster_putorpatch.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 0291df73bec..d7ce41a7dd2 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -98,7 +98,8 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } } - // Don't persist identity parameters in non-wimi clusters + // Don't persist identity parameters in non managed/workload identity clusters + // 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 isCreate { if err := validateIdentityUrl(doc.OpenShiftCluster, identityURL); err != nil { From 786e0cf65146c31f91420c942274d537d554daa7 Mon Sep 17 00:00:00 2001 From: Nicolas Ontiveros Date: Tue, 2 Jul 2024 07:57:39 -0700 Subject: [PATCH 13/13] Fix unit tests --- pkg/frontend/openshiftcluster_putorpatch.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index d7ce41a7dd2..ae12dcdf114 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -98,10 +98,11 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } } - // Don't persist identity parameters in non managed/workload identity clusters - // 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 isCreate { + 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 err := validateIdentityUrl(doc.OpenShiftCluster, identityURL); err != nil { return nil, err }