Skip to content

Commit

Permalink
feedback/comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@cadenmarchese
cadenmarchese committed May 16, 2024
1 parent e8fdae8 commit ceb8cac
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 52 deletions.
24 changes: 10 additions & 14 deletions pkg/frontend/openshiftcluster_putorpatch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down
53 changes: 15 additions & 38 deletions pkg/frontend/openshiftcluster_putorpatch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand All @@ -3347,11 +3344,10 @@ func TestPersistIdentityUrl(t *testing.T) {
},
},
isCreate: true,

Check failure on line 3346 in pkg/frontend/openshiftcluster_putorpatch_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

File is not `gofmt`-ed with `-s` (gofmt)
wantError: "",
},
{
name: "cluster is not wi/mi, identityURL not passed",
identityURL: nil,
identityURL: "",
cluster: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
ServicePrincipalProfile: &api.ServicePrincipalProfile{},
Expand All @@ -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))
}

Expand Down

0 comments on commit ceb8cac

Please sign in to comment.