Merge pull request #351 from hrk091/use-gvk-to-check-enforced-type

Use GVK to validate resource type

Author: k8s-ci-robot

internal/hncconfig/reconciler.go

@@ -146,15 +146,26 @@ 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}
-		// If there are multiple configurations of the same type, we will follow the
+		// Look if the resource exists in the API server.
+		gvk, err := r.resourceMapper.NamespacedKindFor(gr)
+		if err != nil {
+			// Log error and write conditions, but don't early exit since the other types can still be reconciled.
+			r.Log.Error(err, "while trying to reconcile the configuration", "type", gr, "mode", rsc.Mode)
+			r.writeCondition(inst, api.ConditionBadTypeConfiguration, reasonForGVKError(err), err.Error())
+			continue
+		}
+
+		// If there are multiple configurations of the same GVK, we will follow the
 		// first configuration and ignore the rest.
-		if gvkMode, exist := r.activeGVKMode[gr]; exist {
-			log := r.Log.WithValues("resource", gr, "appliedMode", gvkMode.mode)
+		if firstGR, exist := r.activeGR[gvk]; exist {
+			gvkMode := r.activeGVKMode[firstGR]
+			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 {
@@ -165,14 +176,6 @@ func (r *Reconciler) reconcileConfigTypes(inst *api.HNCConfiguration) {
 			continue
 		}
 
-		// Look if the resource exists in the API server.
-		gvk, err := r.resourceMapper.NamespacedKindFor(gr)
-		if err != nil {
-			// Log error and write conditions, but don't early exit since the other types can still be reconciled.
-			r.Log.Error(err, "while trying to reconcile the configuration", "type", gr, "mode", rsc.Mode)
-			r.writeCondition(inst, api.ConditionBadTypeConfiguration, reasonForGVKError(err), err.Error())
-			continue
-		}
 		r.activeGVKMode[gr] = gvkMode{gvk, rsc.Mode}
 		r.activeGR[gvk] = gr
 	}
@@ -539,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/reconciler_test.go

@@ -89,6 +89,22 @@ var _ = Describe("HNCConfiguration", func() {
 		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring(api.RoleResource))
 	})
 
+	It("should ignore the `roles` configuration in the spec and set `MultipleConfigurationsForType` condition even when given in a singular form", func() {
+		AddToHNCConfig(ctx, api.RBACGroup, "role", api.Ignore)
+
+		Eventually(typeSpecMode(ctx, api.RBACGroup, "role")).Should(Equal(api.Ignore))
+		Eventually(typeStatusMode(ctx, api.RBACGroup, api.RoleResource)).Should(Equal(api.Propagate))
+		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring("role"))
+	})
+
+	It("should ignore the `roles` configuration in the spec and set `MultipleConfigurationsForType` condition even when specified without group", func() {
+		AddToHNCConfig(ctx, "", api.RoleResource, api.Ignore)
+
+		Eventually(typeSpecMode(ctx, "", api.RoleResource)).Should(Equal(api.Ignore))
+		Eventually(typeStatusMode(ctx, api.RBACGroup, api.RoleResource)).Should(Equal(api.Propagate))
+		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring(api.RoleResource))
+	})
+
 	It("should propagate RoleBindings by default", func() {
 		SetParent(ctx, barName, fooName)
 		// Give foo a role binding.
@@ -106,6 +122,22 @@ var _ = Describe("HNCConfiguration", func() {
 		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring(api.RoleBindingResource))
 	})
 
+	It("should ignore the `rolebindings` configuration in the spec and set `MultipleConfigurationsForType` condition even when given in a singular form", func() {
+		AddToHNCConfig(ctx, api.RBACGroup, "rolebinding", api.Ignore)
+
+		Eventually(typeSpecMode(ctx, api.RBACGroup, "rolebinding")).Should(Equal(api.Ignore))
+		Eventually(typeStatusMode(ctx, api.RBACGroup, api.RoleBindingResource)).Should(Equal(api.Propagate))
+		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring("rolebinding"))
+	})
+
+	It("should ignore the `rolebindings` configuration in the spec and set `MultipleConfigurationsForType` condition even when specified without group", func() {
+		AddToHNCConfig(ctx, "", api.RoleBindingResource, api.Ignore)
+
+		Eventually(typeSpecMode(ctx, "", api.RoleBindingResource)).Should(Equal(api.Ignore))
+		Eventually(typeStatusMode(ctx, api.RBACGroup, api.RoleBindingResource)).Should(Equal(api.Propagate))
+		Eventually(GetHNCConfigCondition(ctx, api.ConditionBadTypeConfiguration, api.ReasonMultipleConfigsForType)).Should(ContainSubstring(api.RoleBindingResource))
+	})
+
 	It("should unset ResourceNotFound condition if a bad type spec is removed", func() {
 		// Group of ConfigMap should be ""
 		AddToHNCConfig(ctx, "wrong", "configmaps", api.Propagate)

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 {