From ceb8cac2bb6dda08dbece892a91d1e55c0da6f97 Mon Sep 17 00:00:00 2001 From: cadenmarchese Date: Thu, 16 May 2024 17:10:21 -0400 Subject: [PATCH] feedback/comments --- pkg/frontend/openshiftcluster_putorpatch.go | 24 ++++----- .../openshiftcluster_putorpatch_test.go | 53 ++++++------------- 2 files changed, 25 insertions(+), 52 deletions(-) diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go index 7374239807d..c35f7255ee8 100644 --- a/pkg/frontend/openshiftcluster_putorpatch.go +++ b/pkg/frontend/openshiftcluster_putorpatch.go @@ -25,13 +25,14 @@ import ( "github.com/Azure/ARO-RP/pkg/util/version" ) +var errMissingIdentityURL error = fmt.Errorf("identityURL not provided but required for workload identity cluster") + func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Request) { ctx := r.Context() log := ctx.Value(middleware.ContextKeyLog).(*logrus.Entry) var header http.Header var b []byte - var identityURL []string body := r.Context().Value(middleware.ContextKeyBody).([]byte) correlationData := r.Context().Value(middleware.ContextKeyCorrelationData).(*api.CorrelationData) @@ -42,10 +43,7 @@ func (f *frontend) putOrPatchOpenShiftCluster(w http.ResponseWriter, r *http.Req subId := chi.URLParam(r, "subscriptionId") resourceProviderNamespace := chi.URLParam(r, "resourceProviderNamespace") - identityUrlHeader := "x-ms-identity-url" - if r.Header.Get(identityUrlHeader) != "" { - identityURL = r.Header.Values(identityUrlHeader) - } + identityURL := r.Header.Get("x-ms-identity-url") apiVersion := r.URL.Query().Get(api.APIVersionKey) err := cosmosdb.RetryOnPreconditionFailed(func() error { @@ -58,7 +56,7 @@ 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) { +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) { subscription, err := f.validateSubscriptionState(ctx, path, api.SubscriptionStateRegistered) if err != nil { return nil, err @@ -98,7 +96,7 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus. } } - err = persistIdentityUrl(doc.OpenShiftCluster, identityURL, isCreate) + err = validateIdentityUrl(doc.OpenShiftCluster, identityURL, isCreate) if err != nil { return nil, err } @@ -300,25 +298,23 @@ func enrichClusterSystemData(doc *api.OpenShiftClusterDocument, systemData *api. } } -func persistIdentityUrl(cluster *api.OpenShiftCluster, identityURL []string, isCreate bool) error { +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 identityURL == nil { + if identityURL == "" { if isCreate { - return fmt.Errorf("identityURL not provided but required for workload identity cluster") + return errMissingIdentityURL } return nil } - if len(identityURL) != 1 { - return fmt.Errorf("identityURL cannot contain multiple values: %v", identityURL) + if cluster.Identity != nil { + cluster.Identity.IdentityURL = identityURL } - cluster.Identity.IdentityURL = identityURL[0] - return nil } diff --git a/pkg/frontend/openshiftcluster_putorpatch_test.go b/pkg/frontend/openshiftcluster_putorpatch_test.go index a6865dcf713..9b8306950a5 100644 --- a/pkg/frontend/openshiftcluster_putorpatch_test.go +++ b/pkg/frontend/openshiftcluster_putorpatch_test.go @@ -3306,36 +3306,33 @@ func TestEnrichClusterSystemData(t *testing.T) { } } -func TestPersistIdentityUrl(t *testing.T) { +func TestValidateIdentityUrl(t *testing.T) { for _, tt := range []struct { name string - identityURL []string + identityURL string cluster *api.OpenShiftCluster expected *api.OpenShiftCluster isCreate bool - wantError string + wantError error }{ { - name: "identity URL is nil, is not wi/mi cluster create", - identityURL: nil, + name: "identity URL is empty, is not wi/mi cluster create", + identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, isCreate: false, - wantError: "", }, { - name: "identity URL is nil, is wi/mi cluster create", - identityURL: nil, + name: "identity URL is empty, is wi/mi cluster create", + identityURL: "", cluster: &api.OpenShiftCluster{}, expected: &api.OpenShiftCluster{}, isCreate: true, - wantError: "identityURL not provided but required for workload identity cluster", + wantError: errMissingIdentityURL, }, { - name: "cluster is not wi/mi, identityURL passed", - identityURL: []string{ - "http://foo.bar", - }, + name: "cluster is not wi/mi, identityURL passed", + identityURL: "http://foo.bar", cluster: &api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ ServicePrincipalProfile: &api.ServicePrincipalProfile{}, @@ -3347,11 +3344,10 @@ func TestPersistIdentityUrl(t *testing.T) { }, }, isCreate: true, - wantError: "", }, { name: "cluster is not wi/mi, identityURL not passed", - identityURL: nil, + identityURL: "", cluster: &api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ ServicePrincipalProfile: &api.ServicePrincipalProfile{}, @@ -3363,43 +3359,24 @@ func TestPersistIdentityUrl(t *testing.T) { }, }, isCreate: true, - wantError: "", - }, - { - name: "fail - identity URL has more than one value", - identityURL: []string{ - "http://foo.bar", - "http://foo.microsoftonline.com", - }, - cluster: &api.OpenShiftCluster{ - Identity: &api.Identity{}, - }, - expected: &api.OpenShiftCluster{ - Identity: &api.Identity{}, - }, - isCreate: true, - wantError: "identityURL cannot contain multiple values: [http://foo.bar http://foo.microsoftonline.com]", }, { - name: "pass - identity URL has one value", + name: "pass - identity URL passed on wi/mi cluster", cluster: &api.OpenShiftCluster{ Identity: &api.Identity{}, }, - identityURL: []string{ - "http://foo.bar", - }, + identityURL: "http://foo.bar", expected: &api.OpenShiftCluster{ Identity: &api.Identity{ IdentityURL: "http://foo.bar", }, }, isCreate: true, - wantError: "", }, } { t.Run(tt.name, func(t *testing.T) { - err := persistIdentityUrl(tt.cluster, tt.identityURL, tt.isCreate) - if err != nil && err.Error() != tt.wantError { + err := validateIdentityUrl(tt.cluster, tt.identityURL, tt.isCreate) + if err != nil && err != tt.wantError { t.Error(cmp.Diff(err, tt.wantError)) }