fix: Use GVK instead of GR to check if the given resource is enforced type

Author: hrk091

internal/hncconfig/reconciler.go

@@ -146,6 +146,7 @@ func (r *Reconciler) ensureEnforcedTypes(inst *api.HNCConfiguration) error {
 // to retry but only set conditions since the configuration may be incorrect.
 func (r *Reconciler) reconcileConfigTypes(inst *api.HNCConfiguration) {
 	// Get valid settings in the spec.resources of the `config` singleton.
+	gvkChecker := newGVKChecker(r.resourceMapper)
 	for _, rsc := range inst.Spec.Resources {
 		gr := schema.GroupResource{Group: rsc.Group, Resource: rsc.Resource}
 		// Look if the resource exists in the API server.
@@ -164,7 +165,7 @@ func (r *Reconciler) reconcileConfigTypes(inst *api.HNCConfiguration) {
 			log := r.Log.WithValues("resource", gr, "appliedMode", rsc.Mode)
 			msg := ""
 			// Set a different message if the type is enforced by HNC.
-			if api.IsEnforcedType(rsc) {
+			if gvkChecker.isEnforced(gvk) {
 				msg = fmt.Sprintf("The sync mode for %q is enforced by HNC as %q and cannot be overridden", gr, api.Propagate)
 				log.Info("The sync mode for this resource is enforced by HNC and cannot be overridden")
 			} else {
@@ -541,3 +542,32 @@ func reasonForGVKError(err error) string {
 	}
 	return reason
 }
+
+// gvkChecker checks if a GVK is an enforced type.
+// It is needed to check duplicated types in ResourceSpec using GVK instead of GR,
+// since the user-given GRs might include some ambiguity.
+// (e.g. singular/plural, empty group is handled as corev1).
+type gvkChecker struct {
+	enforcedGVKs []schema.GroupVersionKind
+}
+
+func newGVKChecker(mapper resourceMapper) *gvkChecker {
+	var enforcedGVKs []schema.GroupVersionKind
+	for _, enforcedType := range api.EnforcedTypes {
+		enforcedGVK, _ := mapper.NamespacedKindFor(schema.GroupResource{
+			Group:    enforcedType.Group,
+			Resource: enforcedType.Resource,
+		})
+		enforcedGVKs = append(enforcedGVKs, enforcedGVK)
+	}
+	return &gvkChecker{enforcedGVKs}
+}
+
+func (c *gvkChecker) isEnforced(gvk schema.GroupVersionKind) bool {
+	for _, enforcedGVK := range c.enforcedGVKs {
+		if gvk == enforcedGVK {
+			return true
+		}
+	}
+	return false
+}

internal/hncconfig/validator.go

@@ -91,14 +91,10 @@ func (v *Validator) handle(inst *api.HNCConfiguration) admission.Response {
 
 func (v *Validator) validateTypes(inst *api.HNCConfiguration, ts gvkSet) admission.Response {
 	allErrs := field.ErrorList{}
+	gvkChecker := newGVKChecker(v.mapper)
 	for i, r := range inst.Spec.Resources {
 		gr := schema.GroupResource{Group: r.Group, Resource: r.Resource}
 		fldPath := field.NewPath("spec", "resources").Index(i)
-		// Validate the type configured is not an HNC enforced type.
-		if api.IsEnforcedType(r) {
-			fldErr := field.Invalid(fldPath, gr, "always uses the 'Propagate' mode and cannot be configured")
-			allErrs = append(allErrs, fldErr)
-		}
 
 		// Validate the resource exists and is namespaced. And convert GR to GVK.
 		// We use GVK because we will need to checkForest() later to avoid source
@@ -109,6 +105,12 @@ func (v *Validator) validateTypes(inst *api.HNCConfiguration, ts gvkSet) admissi
 			allErrs = append(allErrs, fldErr)
 		}
 
+		// Validate the type configured is not an HNC enforced type.
+		if gvkChecker.isEnforced(gvk) {
+			fldErr := field.Invalid(fldPath, gr, "always uses the 'Propagate' mode and cannot be configured")
+			allErrs = append(allErrs, fldErr)
+		}
+
 		// Validate if the configuration of a type already exists. Each type should
 		// only have one configuration.
 		if _, exists := ts[gvk]; exists {

internal/hncconfig/validator_test.go

@@ -24,6 +24,10 @@ var (
 	gr2gvk = map[schema.GroupResource]schema.GroupVersionKind{
 		{Group: api.RBACGroup, Resource: api.RoleResource}:        {Group: api.RBACGroup, Version: "v1", Kind: api.RoleKind},
 		{Group: api.RBACGroup, Resource: api.RoleBindingResource}: {Group: api.RBACGroup, Version: "v1", Kind: api.RoleBindingKind},
+		{Group: api.RBACGroup, Resource: "role"}:                  {Group: api.RBACGroup, Version: "v1", Kind: api.RoleKind},
+		{Group: api.RBACGroup, Resource: "rolebinding"}:           {Group: api.RBACGroup, Version: "v1", Kind: api.RoleBindingKind},
+		{Resource: api.RoleResource}:                              {Group: api.RBACGroup, Version: "v1", Kind: api.RoleKind},
+		{Resource: api.RoleBindingResource}:                       {Group: api.RBACGroup, Version: "v1", Kind: api.RoleBindingKind},
 		{Group: "", Resource: "secrets"}:                          {Group: "", Version: "v1", Kind: "Secret"},
 		{Group: "", Resource: "resourcequotas"}:                   {Group: "", Version: "v1", Kind: "ResourceQuota"},
 	}
@@ -87,6 +91,22 @@ func TestRBACTypes(t *testing.T) {
 			},
 			allow: false,
 		},
+		{
+			name: "Configure redundant enforced resources in a singular manner",
+			configs: []api.ResourceSpec{
+				{Group: api.RBACGroup, Resource: "role", Mode: api.AllowPropagate},
+				{Group: api.RBACGroup, Resource: "rolebinding", Mode: api.AllowPropagate},
+			},
+			allow: false,
+		},
+		{
+			name: "Configure redundant enforced resources without specifying group",
+			configs: []api.ResourceSpec{
+				{Resource: api.RoleResource, Mode: api.AllowPropagate},
+				{Resource: api.RoleBindingResource, Mode: api.AllowPropagate},
+			},
+			allow: false,
+		},
 	}
 
 	for _, tc := range tests {