Skip to content

Commit

Permalink
Fixing magic strings on operator flags (Azure#3327)
Browse files Browse the repository at this point in the history 
* replacing usages of magic strings with flags from the subpackage

* removing the //todo comment regarding the magic strings

* replacing magic strings with operator constants

* move DefaultOperatorFlags to operator package, inject when needed
  • Loading branch information
@azoppiserpa @ventifus
azoppiserpa authored and ventifus committed Feb 7, 2024
1 parent eff0e44 commit 0801fd9
Show file tree
Hide file tree
Showing 66 changed files with 353 additions and 336 deletions.
50 changes: 2 additions & 48 deletions pkg/api/defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ package api
// when interacting with newer api versions. This together with
// database migration will make sure we have right values in the cluster documents
// when moving between old and new versions
func SetDefaults(doc *OpenShiftClusterDocument) {
func SetDefaults(doc *OpenShiftClusterDocument, defaultOperatorFlags func() map[string]string) {
if doc.OpenShiftCluster != nil {
// EncryptionAtHost was introduced in 2021-09-01-preview.
// It can't be changed post cluster creation
Expand Down Expand Up @@ -40,7 +40,7 @@ func SetDefaults(doc *OpenShiftClusterDocument) {

// If there's no operator flags, set the default ones
if doc.OpenShiftCluster.Properties.OperatorFlags == nil {
doc.OpenShiftCluster.Properties.OperatorFlags = DefaultOperatorFlags()
doc.OpenShiftCluster.Properties.OperatorFlags = OperatorFlags(defaultOperatorFlags())
}

// If there's no OutboundType, set default one
Expand All @@ -63,49 +63,3 @@ func SetDefaults(doc *OpenShiftClusterDocument) {
}
}
}

// shorthand
const flagTrue string = "true"
const flagFalse string = "false"

// DefaultOperatorFlags returns flags for new clusters
// and ones that have not been AdminUpdated.
func DefaultOperatorFlags() OperatorFlags {
// TODO: Get rid of magic strings.
// We already have constants for all of the below strings.
// For example `controllerEnabled` in `github.com/Azure/ARO-RP/pkg/operator/controllers/machine`.
// But if we import packages with constants here we will have a cyclic import issue because controllers
// import this package. We should probably move this somewhere else.
// Maybe into a subpackage like `github.com/Azure/ARO-RP/pkg/api/defaults`?
return OperatorFlags{
"aro.alertwebhook.enabled": flagTrue,
"aro.azuresubnets.enabled": flagTrue,
"aro.azuresubnets.nsg.managed": flagTrue,
"aro.azuresubnets.serviceendpoint.managed": flagTrue,
"aro.banner.enabled": flagFalse,
"aro.checker.enabled": flagTrue,
"aro.dnsmasq.enabled": flagTrue,
"aro.restartdnsmasq.enabled": flagFalse,
"aro.genevalogging.enabled": flagTrue,
"aro.imageconfig.enabled": flagTrue,
"aro.ingress.enabled": flagTrue,
"aro.machine.enabled": flagTrue,
"aro.machineset.enabled": flagTrue,
"aro.machinehealthcheck.enabled": flagTrue,
"aro.machinehealthcheck.managed": flagTrue,
"aro.monitoring.enabled": flagTrue,
"aro.nodedrainer.enabled": flagTrue,
"aro.pullsecret.enabled": flagTrue,
"aro.pullsecret.managed": flagTrue,
"aro.rbac.enabled": flagTrue,
"aro.routefix.enabled": flagTrue,
"aro.storageaccounts.enabled": flagTrue,
"aro.workaround.enabled": flagTrue,
"aro.autosizednodes.enabled": flagTrue,
"rh.srep.muo.enabled": flagTrue,
"rh.srep.muo.managed": flagTrue,
"aro.guardrails.enabled": flagFalse,
"aro.guardrails.deploy.managed": flagFalse,
"aro.cloudproviderconfig.enabled": flagTrue,
}
}
4 changes: 2 additions & 2 deletions pkg/api/defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func validOpenShiftClusterDocument() *OpenShiftClusterDocument {
ClusterProfile: ClusterProfile{
FipsValidatedModules: FipsValidatedModulesDisabled,
},
OperatorFlags: DefaultOperatorFlags(),
OperatorFlags: OperatorFlags{"testflag": "testvalue"},
},
},
}
Expand Down Expand Up @@ -136,7 +136,7 @@ func TestSetDefaults(t *testing.T) {
tt.input(doc)
}

SetDefaults(doc)
SetDefaults(doc, func() map[string]string { return map[string]string{"testflag": "testvalue"} })

if !reflect.DeepEqual(&doc, &want) {
t.Error(fmt.Errorf("\n%+v\n !=\n%+v", doc, want)) // can't use cmp due to cycle imports
Expand Down
5 changes: 3 additions & 2 deletions pkg/cluster/defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (
"context"

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

// ensureDefaults will ensure cluster documents has all default values
// for new api versions
func (m *manager) ensureDefaults(ctx context.Context) error {
var err error
m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
api.SetDefaults(doc)
api.SetDefaults(doc, operator.DefaultOperatorFlags)
return nil
})
if err != nil {
Expand All @@ -31,7 +32,7 @@ func (m *manager) ensurePreconfiguredNSG(ctx context.Context) error {
var err error
m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
flags := doc.OpenShiftCluster.Properties.OperatorFlags
flags["aro.azuresubnets.nsg.managed"] = "false"
flags[operator.AzureSubnetsNsgManaged] = operator.FlagFalse
return nil
})
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/frontend/openshiftcluster_putorpatch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/Azure/ARO-RP/pkg/database/cosmosdb"
"github.com/Azure/ARO-RP/pkg/env"
"github.com/Azure/ARO-RP/pkg/frontend/middleware"
"github.com/Azure/ARO-RP/pkg/operator"
"github.com/Azure/ARO-RP/pkg/util/feature"
"github.com/Azure/ARO-RP/pkg/util/version"
)
Expand Down Expand Up @@ -207,7 +208,7 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.
}

// SetDefaults will set defaults on cluster document
api.SetDefaults(doc)
api.SetDefaults(doc, operator.DefaultOperatorFlags)

doc.AsyncOperationID, err = f.newAsyncOperation(ctx, subId, resourceProviderNamespace, doc)
if err != nil {
Expand Down
31 changes: 16 additions & 15 deletions pkg/frontend/openshiftcluster_putorpatch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
v20200430 "github.com/Azure/ARO-RP/pkg/api/v20200430"
v20220401 "github.com/Azure/ARO-RP/pkg/api/v20220401"
"github.com/Azure/ARO-RP/pkg/metrics/noop"
"github.com/Azure/ARO-RP/pkg/operator"
"github.com/Azure/ARO-RP/pkg/util/bucket"
"github.com/Azure/ARO-RP/pkg/util/cmp"
mock_frontend "github.com/Azure/ARO-RP/pkg/util/mocks/frontend"
Expand Down Expand Up @@ -319,7 +320,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: api.MasterProfile{
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
MaintenanceState: api.MaintenanceStateUnplanned,
},
},
Expand Down Expand Up @@ -349,7 +350,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
ClusterProfile: admin.ClusterProfile{
FipsValidatedModules: admin.FipsValidatedModulesDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
MaintenanceState: admin.MaintenanceStateUnplanned,
},
},
Expand Down Expand Up @@ -414,7 +415,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: api.MasterProfile{
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
MaintenanceState: api.MaintenanceStateUnplanned,
},
},
Expand Down Expand Up @@ -444,7 +445,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
MaintenanceState: admin.MaintenanceStateUnplanned,
},
},
Expand Down Expand Up @@ -691,7 +692,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
MaintenanceState: api.MaintenanceStatePending,
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
},
},
})
Expand Down Expand Up @@ -721,7 +722,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
},
},
},
Expand Down Expand Up @@ -789,7 +790,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
MaintenanceState: api.MaintenanceStatePending,
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
},
},
})
Expand Down Expand Up @@ -819,7 +820,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
},
},
},
Expand Down Expand Up @@ -885,7 +886,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: api.MasterProfile{
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
MaintenanceState: api.MaintenanceStatePlanned,
},
},
Expand Down Expand Up @@ -915,7 +916,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
MaintenanceState: admin.MaintenanceStatePlanned,
},
},
Expand Down Expand Up @@ -984,7 +985,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
MaintenanceState: api.MaintenanceStateNone,
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
},
},
})
Expand Down Expand Up @@ -1014,7 +1015,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
},
},
},
Expand Down Expand Up @@ -1082,7 +1083,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
EncryptionAtHost: api.EncryptionAtHostDisabled,
},
MaintenanceState: api.MaintenanceStateNone,
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
},
},
})
Expand Down Expand Up @@ -1112,7 +1113,7 @@ func TestPutOrPatchOpenShiftClusterAdminAPI(t *testing.T) {
MasterProfile: admin.MasterProfile{
EncryptionAtHost: admin.EncryptionAtHostDisabled,
},
OperatorFlags: admin.OperatorFlags(api.DefaultOperatorFlags()),
OperatorFlags: admin.OperatorFlags(operator.DefaultOperatorFlags()),
},
},
},
Expand Down Expand Up @@ -1393,7 +1394,7 @@ func TestPutOrPatchOpenShiftCluster(t *testing.T) {
FeatureProfile: api.FeatureProfile{
GatewayEnabled: true,
},
OperatorFlags: api.DefaultOperatorFlags(),
OperatorFlags: operator.DefaultOperatorFlags(),
},
},
})
Expand Down
4 changes: 2 additions & 2 deletions pkg/monitor/cluster/clusterflagsandbanner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/operator"
arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
)

Expand All @@ -27,7 +27,7 @@ func (mon *Monitor) emitOperatorFlagsAndSupportBanner(ctx context.Context) error

for _, cluster := range clusters.Items {
if cluster.Spec.OperatorFlags != nil {
defaultFlags := api.DefaultOperatorFlags()
defaultFlags := operator.DefaultOperatorFlags()
nonStandardOperatorFlagDims := make(map[string]string, len(defaultFlags))

//check if the current set flags matches the default ones
Expand Down
38 changes: 19 additions & 19 deletions pkg/monitor/cluster/clusterflagsandbanner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/operator"
arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake"
)
Expand Down Expand Up @@ -45,15 +45,15 @@ func (e *fakeMetricsEmitter) EmitFloat(topic string, value float64, dims map[str

func generateDefaultFlags() arov1alpha1.OperatorFlags {
df := make(arov1alpha1.OperatorFlags)
for k, v := range api.DefaultOperatorFlags() {
for k, v := range operator.DefaultOperatorFlags() {
df[k] = v
}
return df
}

func generateNonStandardFlags(nonDefualtFlagNames []string) arov1alpha1.OperatorFlags {
nsf := make(arov1alpha1.OperatorFlags)
for k, v := range api.DefaultOperatorFlags() {
for k, v := range operator.DefaultOperatorFlags() {
nsf[k] = v
}
for _, n := range nonDefualtFlagNames {
Expand All @@ -68,7 +68,7 @@ func generateNonStandardFlags(nonDefualtFlagNames []string) arov1alpha1.Operator

func generateFlagsWithMissingEntries(missingFlagNames []string) arov1alpha1.OperatorFlags {
mf := make(arov1alpha1.OperatorFlags)
for k, v := range api.DefaultOperatorFlags() {
for k, v := range operator.DefaultOperatorFlags() {
mf[k] = v
}
for _, n := range missingFlagNames {
Expand Down Expand Up @@ -117,32 +117,32 @@ func TestEmitOperatorFlagsAndSupportBanner(t *testing.T) {
},
{
name: "cluster with non-standard operator flags",
operatorFlags: generateNonStandardFlags([]string{"aro.imageconfig.enabled", "aro.dnsmasq.enabled", "aro.genevalogging.enabled", "aro.autosizednodes.enabled"}),
operatorFlags: generateNonStandardFlags([]string{operator.ImageConfigEnabled, operator.DnsmasqEnabled, operator.GenevaLoggingEnabled, operator.AutosizedNodesEnabled}),
clusterBanner: arov1alpha1.Banner{
Content: "",
},
expectFlagsMetricsValue: 1,
expectFlagsMetricsDims: map[string]string{
"aro.imageconfig.enabled": "false",
"aro.dnsmasq.enabled": "false",
"aro.genevalogging.enabled": "false",
"aro.autosizednodes.enabled": "false",
operator.ImageConfigEnabled: operator.FlagFalse,
operator.DnsmasqEnabled: operator.FlagFalse,
operator.GenevaLoggingEnabled: operator.FlagFalse,
operator.AutosizedNodesEnabled: operator.FlagFalse,
},
expectBannerMetricsValue: 0,
expectBannerMetricsDims: nil,
},
{
name: "cluster with missing operator flags",
operatorFlags: generateFlagsWithMissingEntries([]string{"aro.imageconfig.enabled", "aro.dnsmasq.enabled", "aro.genevalogging.enabled", "aro.autosizednodes.enabled"}),
operatorFlags: generateFlagsWithMissingEntries([]string{operator.ImageConfigEnabled, operator.DnsmasqEnabled, operator.GenevaLoggingEnabled, operator.AutosizedNodesEnabled}),
clusterBanner: arov1alpha1.Banner{
Content: "",
},
expectFlagsMetricsValue: 1,
expectFlagsMetricsDims: map[string]string{
"aro.imageconfig.enabled": "DNE",
"aro.dnsmasq.enabled": "DNE",
"aro.genevalogging.enabled": "DNE",
"aro.autosizednodes.enabled": "DNE",
operator.ImageConfigEnabled: "DNE",
operator.DnsmasqEnabled: "DNE",
operator.GenevaLoggingEnabled: "DNE",
operator.AutosizedNodesEnabled: "DNE",
},
expectBannerMetricsValue: 0,
expectBannerMetricsDims: nil,
Expand All @@ -160,16 +160,16 @@ func TestEmitOperatorFlagsAndSupportBanner(t *testing.T) {
},
{
name: "cluster with non-standard operator flags and activated support banner",
operatorFlags: generateNonStandardFlags([]string{"aro.imageconfig.enabled", "aro.dnsmasq.enabled", "aro.genevalogging.enabled", "aro.autosizednodes.enabled"}),
operatorFlags: generateNonStandardFlags([]string{operator.ImageConfigEnabled, operator.DnsmasqEnabled, operator.GenevaLoggingEnabled, operator.AutosizedNodesEnabled}),
clusterBanner: arov1alpha1.Banner{
Content: arov1alpha1.BannerContactSupport,
},
expectFlagsMetricsValue: 1,
expectFlagsMetricsDims: map[string]string{
"aro.imageconfig.enabled": "false",
"aro.dnsmasq.enabled": "false",
"aro.genevalogging.enabled": "false",
"aro.autosizednodes.enabled": "false",
operator.ImageConfigEnabled: operator.FlagFalse,
operator.DnsmasqEnabled: operator.FlagFalse,
operator.GenevaLoggingEnabled: operator.FlagFalse,
operator.AutosizedNodesEnabled: operator.FlagFalse,
},
expectBannerMetricsValue: 1,
expectBannerMetricsDims: map[string]string{"msg": "contact support"},
Expand Down
Loading

0 comments on commit 0801fd9

Please sign in to comment.