Use LabelSelector directly in namespaceSelector

- Replace `matchLabels`/`matchExpressions` with `LabelSelector` for simplicity - Clean up variable declarations throughout Signed-off-by: Dale Haiducek <[email protected]>
open-cluster-management-io · Oct 30, 2024 · 90571b0 · 90571b0
1 parent 48d42aa
commit 90571b0
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 140 deletions.
diff --git a/api/v1/configurationpolicy_types.go b/api/v1/configurationpolicy_types.go
@@ -70,35 +70,24 @@ type Severity string
 type PruneObjectBehavior string
 
 type Target struct {
+	*metav1.LabelSelector `json:",inline"`
+
 	// Include is an array of filepath expressions to include objects by name.
 	Include []NonEmptyString `json:"include,omitempty"`
 
 	// Exclude is an array of filepath expressions to exclude objects by name.
 	Exclude []NonEmptyString `json:"exclude,omitempty"`
-
-	// MatchLabels is a map of {key,value} pairs matching objects by label.
-	MatchLabels *map[string]string `json:"matchLabels,omitempty"`
-
-	// MatchExpressions is an array of label selector requirements matching objects by label.
-	MatchExpressions *[]metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty"`
 }
 
 // Define String() so that the LabelSelector is dereferenced in the logs
 func (t Target) String() string {
 	fmtSelectorStr := "{include:%s,exclude:%s,matchLabels:%+v,matchExpressions:%+v}"
-	if t.MatchLabels == nil && t.MatchExpressions == nil {
-		return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, nil, nil)
-	}
-
-	if t.MatchLabels == nil {
-		return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, nil, *t.MatchExpressions)
-	}
 
-	if t.MatchExpressions == nil {
-		return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, *t.MatchLabels, nil)
+	if t.LabelSelector == nil {
+		return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, nil, nil)
 	}
 
-	return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, *t.MatchLabels, *t.MatchExpressions)
+	return fmt.Sprintf(fmtSelectorStr, t.Include, t.Exclude, t.MatchLabels, t.MatchExpressions)
 }
 
 // EvaluationInterval configures the minimum elapsed time before a configuration policy is
@@ -292,7 +281,8 @@ type ConfigurationPolicySpec struct {
 	// `spec["object-templates"]`. All selector rules are combined. If 'include' is not provided but
 	// `matchLabels` and/or `matchExpressions` are, `include` will behave as if `['*']` were given. If
 	// `matchExpressions` and `matchLabels` are both not provided, `include` must be provided to
-	// retrieve namespaces.
+	// retrieve namespaces. If there is a namespace defined in the `objectDefinition`, the
+	// `namespaceSelector` is ignored.
 	NamespaceSelector Target `json:"namespaceSelector,omitempty"`
 
 	// The `object-templates` is an array of object configurations for the configuration policy to

diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go
diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
@@ -55,24 +55,11 @@ import (
 )
 
 const (
-	ControllerName             string = "configuration-policy-controller"
-	CRDName                    string = "configurationpolicies.policy.open-cluster-management.io"
-	pruneObjectFinalizer       string = "policy.open-cluster-management.io/delete-related-objects"
-	disableTemplatesAnnotation string = "policy.open-cluster-management.io/disable-templates"
-)
-
-var log = ctrl.Log.WithName(ControllerName)
-
-// PlcChan a channel used to pass policies ready for update
-var PlcChan chan *policyv1.ConfigurationPolicy
-
-var (
-	eventNormal  = "Normal"
-	eventWarning = "Warning"
-	eventFmtStr  = "policy: %s/%s"
-)
+	ControllerName             = "configuration-policy-controller"
+	CRDName                    = "configurationpolicies.policy.open-cluster-management.io"
+	pruneObjectFinalizer       = "policy.open-cluster-management.io/delete-related-objects"
+	disableTemplatesAnnotation = "policy.open-cluster-management.io/disable-templates"
 
-const (
 	reasonWantFoundExists    = "Resource found as expected"
 	reasonWantFoundCreated   = "K8s creation success"
 	reasonUpdateSuccess      = "K8s update success"
@@ -85,11 +72,19 @@ const (
 	reasonFoundNotApplicable = "Resource found but will not be handled in mustnothave mode"
 )
 
-var ErrPolicyInvalid = errors.New("the Policy is invalid")
+var (
+	log = ctrl.Log.WithName(ControllerName)
+
+	eventNormal  = "Normal"
+	eventWarning = "Warning"
+	eventFmtStr  = "policy: %s/%s"
 
-// commonSprigFuncMap includes only the sprig functions that are available in the
-// stolostron/go-template-utils library.
-var commonSprigFuncMap template.FuncMap
+	ErrPolicyInvalid = errors.New("the Policy is invalid")
+
+	// commonSprigFuncMap includes only the sprig functions that are available in the
+	// stolostron/go-template-utils library.
+	commonSprigFuncMap template.FuncMap
+)
 
 func init() {
 	commonSprigFuncMap = template.FuncMap{}
@@ -444,8 +439,7 @@ func (r *ConfigurationPolicyReconciler) shouldEvaluatePolicy(
 		return true, 0
 	}
 
-	usesSelector := policy.Spec.NamespaceSelector.MatchLabels != nil ||
-		policy.Spec.NamespaceSelector.MatchExpressions != nil ||
+	usesSelector := policy.Spec.NamespaceSelector.LabelSelector != nil ||
 		len(policy.Spec.NamespaceSelector.Include) != 0
 
 	if usesSelector && r.SelectorReconciler.HasUpdate(policy.Namespace, policy.Name) {
@@ -1269,8 +1263,8 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObject(
 		}
 	}
 
-	// strings.TrimSpace() is needed here because a multi-line value will have '\n' in it. This is kept for
-	// backwards compatibility.
+	// strings.TrimSpace() is needed here because a multi-line value will have
+	// '\n' in it. This is kept for backwards compatibility.
 	desiredObj.SetName(strings.TrimSpace(desiredObj.GetName()))
 	desiredObj.SetNamespace(strings.TrimSpace(desiredObj.GetNamespace()))
 	desiredObj.SetKind(strings.TrimSpace(desiredObj.GetKind()))
@@ -1289,11 +1283,16 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObject(
 		}
 	}
 
-	if scopedGVR.Namespaced && desiredObj.GetNamespace() == "" {
-		selectedNamespaces, err := r.SelectorReconciler.Get(plc.Namespace, plc.Name, plc.Spec.NamespaceSelector)
+	// Fetch and filter namespaces using provided namespaceSelector
+	desiredNs := desiredObj.GetNamespace()
+
+	if scopedGVR.Namespaced && desiredNs == "" {
+		nsSelector := plc.Spec.NamespaceSelector
+
+		selectedNamespaces, err := r.SelectorReconciler.Get(plc.Namespace, plc.Name, nsSelector)
 		if err != nil {
 			log.Error(err, "Failed to select the namespaces",
-				"namespaceSelector", fmt.Sprintf("%+v", plc.Spec.NamespaceSelector))
+				"namespaceSelector", nsSelector.String())
 
 			msg := fmt.Sprintf("Error filtering namespaces with provided namespaceSelector: %v", err)
 
@@ -1308,12 +1307,12 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObject(
 		}
 
 		if len(selectedNamespaces) == 0 {
-			relevantNamespaces = []string{desiredObj.GetNamespace()}
+			relevantNamespaces = []string{desiredNs}
 		} else {
 			relevantNamespaces = selectedNamespaces
 		}
 	} else {
-		relevantNamespaces = []string{desiredObj.GetNamespace()}
+		relevantNamespaces = []string{desiredNs}
 	}
 
 	return desiredObj, scopedGVR, relevantNamespaces, errEvent, mappingErr

diff --git a/...ustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml b/...ustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml
@@ -99,7 +99,8 @@ spec:
                   `spec["object-templates"]`. All selector rules are combined. If 'include' is not provided but
                   `matchLabels` and/or `matchExpressions` are, `include` will behave as if `['*']` were given. If
                   `matchExpressions` and `matchLabels` are both not provided, `include` must be provided to
-                  retrieve namespaces.
+                  retrieve namespaces. If there is a namespace defined in the `objectDefinition`, the
+                  `namespaceSelector` is ignored.
                 properties:
                   exclude:
                     description: Exclude is an array of filepath expressions to exclude
@@ -116,8 +117,8 @@ spec:
                       type: string
                     type: array
                   matchExpressions:
-                    description: MatchExpressions is an array of label selector requirements
-                      matching objects by label.
+                    description: matchExpressions is a list of label selector requirements.
+                      The requirements are ANDed.
                     items:
                       description: |-
                         A label selector requirement is a selector that contains values, a key, and an operator that
@@ -147,13 +148,17 @@ spec:
                       - operator
                       type: object
                     type: array
+                    x-kubernetes-list-type: atomic
                   matchLabels:
                     additionalProperties:
                       type: string
-                    description: MatchLabels is a map of {key,value} pairs matching
-                      objects by label.
+                    description: |-
+                      matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels
+                      map is equivalent to an element of matchExpressions, whose key field is "key", the
+                      operator is "In", and the values array contains only "value". The requirements are ANDed.
                     type: object
                 type: object
+                x-kubernetes-map-type: atomic
               object-templates:
                 description: |-
                   The `object-templates` is an array of object configurations for the configuration policy to

diff --git a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml
@@ -106,7 +106,8 @@ spec:
                   `spec["object-templates"]`. All selector rules are combined. If 'include' is not provided but
                   `matchLabels` and/or `matchExpressions` are, `include` will behave as if `['*']` were given. If
                   `matchExpressions` and `matchLabels` are both not provided, `include` must be provided to
-                  retrieve namespaces.
+                  retrieve namespaces. If there is a namespace defined in the `objectDefinition`, the
+                  `namespaceSelector` is ignored.
                 properties:
                   exclude:
                     description: Exclude is an array of filepath expressions to exclude
@@ -123,8 +124,8 @@ spec:
                       type: string
                     type: array
                   matchExpressions:
-                    description: MatchExpressions is an array of label selector requirements
-                      matching objects by label.
+                    description: matchExpressions is a list of label selector requirements.
+                      The requirements are ANDed.
                     items:
                       description: |-
                         A label selector requirement is a selector that contains values, a key, and an operator that
@@ -154,13 +155,17 @@ spec:
                       - operator
                       type: object
                     type: array
+                    x-kubernetes-list-type: atomic
                   matchLabels:
                     additionalProperties:
                       type: string
-                    description: MatchLabels is a map of {key,value} pairs matching
-                      objects by label.
+                    description: |-
+                      matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels
+                      map is equivalent to an element of matchExpressions, whose key field is "key", the
+                      operator is "In", and the values array contains only "value". The requirements are ANDed.
                     type: object
                 type: object
+                x-kubernetes-map-type: atomic
               object-templates:
                 description: |-
                   The `object-templates` is an array of object configurations for the configuration policy to

diff --git a/main.go b/main.go
@@ -70,11 +70,6 @@ var (
 // Policies applied by users are deployed here. Used only in non-hosted mode.
 const ocmPolicyNs = "open-cluster-management-policies"
 
-func printVersion() {
-	log.Info("Using", "OperatorVersion", version.Version, "GoVersion", runtime.Version(),
-		"GOOS", runtime.GOOS, "GOARCH", runtime.GOARCH)
-}
-
 func init() {
 	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
 	//+kubebuilder:scaffold:scheme
@@ -163,7 +158,8 @@ func main() {
 		panic("The --evaluation-concurrency option cannot be less than 1")
 	}
 
-	printVersion()
+	log.Info("Using", "OperatorVersion", version.Version, "GoVersion", runtime.Version(),
+		"GOOS", runtime.GOOS, "GOARCH", runtime.GOARCH)
 
 	// Get a config to talk to the apiserver
 	cfg, err := config.GetConfig()