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.

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

[ARO-1885] Implement OperatorFlagsMergeStrategy #3911

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 8 additions & 7 deletions pkg/api/admin/openshiftcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@ type OpenShiftClusterList struct {

// OpenShiftCluster represents an Azure Red Hat OpenShift cluster.
type OpenShiftCluster struct {
ID string `json:"id,omitempty" mutable:"case"`
Name string `json:"name,omitempty" mutable:"case"`
Type string `json:"type,omitempty" mutable:"case"`
Location string `json:"location,omitempty"`
Tags map[string]string `json:"tags,omitempty"`
Properties OpenShiftClusterProperties `json:"properties,omitempty"`
Identity *Identity `json:"identity,omitempty"`
ID string `json:"id,omitempty" mutable:"case"`
Name string `json:"name,omitempty" mutable:"case"`
Type string `json:"type,omitempty" mutable:"case"`
Location string `json:"location,omitempty"`
Tags map[string]string `json:"tags,omitempty"`
Properties OpenShiftClusterProperties `json:"properties,omitempty"`
Identity *Identity `json:"identity,omitempty"`
OperatorFlagsMergeStrategy string `json:"operatorFlagsMergeStrategy,omitempty" mutable:"true"`
}

// OpenShiftClusterProperties represents an OpenShift cluster's properties.
Expand Down
55 changes: 55 additions & 0 deletions pkg/api/admin/operatorflags.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package admin

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

import (
"encoding/json"
"net/http"

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

const (
OperatorFlagsMergeStrategyMerge string = "merge"
OperatorFlagsMergeStrategyReset string = "reset"
)

// When a cluster is edited via the PATCH Cluster Geneva Action (aka an Admin Update)
// the flags given are treated according to the provided Update Strategy,
// provided in operatorFlagsMergeStrategy

// merge (default): The provided cluster flags are laid on top of the cluster’s existing flags.
// reset: The provided cluster flags are laid on top of the default cluster flags,
// essentially ‘resetting’ the flags if no new flags are provided.
func OperatorFlagsMergeStrategy(oc *api.OpenShiftCluster, body []byte) error {
payload := &OpenShiftCluster{}

err := json.Unmarshal(body, &payload)
if err != nil {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidRequestContent, "", "The request content was invalid and could not be deserialized: %q.", err)
}

// if it's empty, use the default of merge, which is performed by
// deserialising the body JSON later
if payload.OperatorFlagsMergeStrategy == "" {
return nil
}

// return error if OperatorFlagsMergeStrategy is not merge or reset, default is merge
if payload.OperatorFlagsMergeStrategy != OperatorFlagsMergeStrategyMerge &&
payload.OperatorFlagsMergeStrategy != OperatorFlagsMergeStrategyReset {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "", "invalid operatorFlagsMergeStrategy '%s', can only be 'merge' or 'reset'", payload.OperatorFlagsMergeStrategy)
}

// return nil, if OperatorFlagsMergeStrategy is merge and payload has not operatorFlags
// return operatorFlags of payload, if OperatorFlagsMergeStrategy is merge and payload has operatorFlags
// return defaultOperatorFlags, if OperatorFlagsMergeStrategy is reset and payload has not operatorFlags
// return defaultOperatorFlags + operatorFlags of payload, if OperatorFlagsMergeStrategy is reset and payload has operatorFlags
if payload.OperatorFlagsMergeStrategy == OperatorFlagsMergeStrategyReset {
oc.Properties.OperatorFlags = operator.DefaultOperatorFlags()
}

return nil
}
60 changes: 60 additions & 0 deletions pkg/api/admin/operatorflags_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package admin

import (
"testing"

"github.com/Azure/ARO-RP/pkg/api"
utilerror "github.com/Azure/ARO-RP/test/util/error"
)

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

func TestOperatorFlagsMergeStrategy(t *testing.T) {
tests := []struct {
name string
oc *api.OpenShiftCluster
wantOc *api.OpenShiftCluster
Copy link
Collaborator

@rhamitarora rhamitarora Oct 22, 2024

Choose a reason for hiding this comment

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

The variable wantOc is declared, but its usage is not shown anywhere.

body []byte
wantErr string
}{
{
name: "invalid_json",
oc: nil,
body: []byte(`{{}`),
wantErr: `400: InvalidRequestContent: : The request content was invalid and could not be deserialized: "invalid character '{' looking for beginning of object key string".`,
},
{
name: "OperatorFlagsMergeStrategy_is_not_merge_or_reset",
oc: nil,
body: []byte(`{"operatorFlagsMergeStrategy": "xyz"}`),
wantErr: `400: InvalidParameter: : invalid operatorFlagsMergeStrategy 'xyz', can only be 'merge' or 'reset'`,
},
{
name: "OperatorFlagsMergeStrategy_payload_is_empty",
oc: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
OperatorFlags: api.OperatorFlags{"aro.feature1.enabled": "false"},
},
},
body: []byte(`{"operatorflagsmergestrategy":"merge"}`),
wantErr: "",
},
{
name: "OperatorFlagsMergeStrategy_reset",
oc: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
OperatorFlags: api.OperatorFlags{"aro.feature1.enabled": "false"},
},
},
body: []byte(`{"operatorflagsmergestrategy":"reset"}`),
wantErr: "",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := OperatorFlagsMergeStrategy(tt.oc, tt.body)
utilerror.AssertErrorMessage(t, err, tt.wantErr)
})
}
}
10 changes: 10 additions & 0 deletions pkg/frontend/openshiftcluster_putorpatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,16 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.
// provide single field json to be updated in the database.
// Patch should be used for updating individual fields of the document.
case http.MethodPatch:
if putOrPatchClusterParameters.apiVersion == admin.APIVersion {
// OperatorFlagsMergeStrategy==reset will place the default flags in
// the external object and then merge in the body's flags when the
// request is unmarshaled below.
err = admin.OperatorFlagsMergeStrategy(doc.OpenShiftCluster, putOrPatchClusterParameters.body)
if err != nil {
// OperatorFlagsMergeStrategy returns CloudErrors
return nil, err
}
}
ext = putOrPatchClusterParameters.converter.ToExternal(doc.OpenShiftCluster)
}

Expand Down
Loading
Loading