Validate CR namespaces based on consul namespaces

ServiceRouters, ServiceResolvers, ServiceSplitters, and ServiceIntentions should only have namespaces set in their spec when consul namespaces are enabled. When consul namespaces are disabled, the webhook now returns a validation error if these CRs have a namespace. Previously, users could apply CRs with namespaces set even when consul namespaces are disabled, causing them to think the namespaces might be having an effect when they are not. Co-authored-by: Ashwin Venkatesh <[email protected]>
hashicorp · Nov 2, 2020 · 4e01b71 · 4e01b71
1 parent 739ea4a
commit 4e01b71
Show file tree

Hide file tree

Showing 17 changed files with 364 additions and 171 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 BUG FIXES:
 * Federation: ensure replication ACL token can replicate policies and tokens in Consul namespaces other than `default` (Consul-enterprise only). [[GH-364](https://github.com/hashicorp/consul-k8s/issues/364)]
+* CRDs: validate custom resources can only set namespace fields if Consul namespaces are enabled [[GH-375](https://github.com/hashicorp/consul-k8s/pull/375)]
 
 ## 0.19.0 (October 12, 2020)
 

diff --git a/api/common/configentry.go b/api/common/configentry.go
@@ -56,5 +56,5 @@ type ConfigEntryResource interface {
 	// DeepCopyObject should be implemented by the generated code.
 	DeepCopyObject() runtime.Object
 	// Validate returns an error if the resource is invalid.
-	Validate() error
+	Validate(namespacesEnabled bool) error
 }
diff --git a/api/common/configentry_webhook.go b/api/common/configentry_webhook.go
@@ -53,7 +53,7 @@ func ValidateConfigEntry(
 			}
 		}
 	}
-	if err := cfgEntry.Validate(); err != nil {
+	if err := cfgEntry.Validate(enableConsulNamespaces); err != nil {
 		return admission.Errored(http.StatusBadRequest, err)
 	}
 	return admission.Allowed(fmt.Sprintf("valid %s request", cfgEntry.KubeKind()))

diff --git a/api/common/configentry_webhook_test.go b/api/common/configentry_webhook_test.go
@@ -193,7 +193,7 @@ func (in *mockConfigEntry) ToConsul(string) capi.ConfigEntry {
 	return &capi.ServiceConfigEntry{}
 }
 
-func (in *mockConfigEntry) Validate() error {
+func (in *mockConfigEntry) Validate(bool) error {
 	if !in.Valid {
 		return errors.New("invalid")
 	}

diff --git a/api/v1alpha1/proxydefaults_types.go b/api/v1alpha1/proxydefaults_types.go
@@ -143,7 +143,7 @@ func (in *ProxyDefaults) MatchesConsul(candidate api.ConfigEntry) bool {
 	return cmp.Equal(in.ToConsul(""), configEntry, cmpopts.IgnoreFields(capi.ProxyConfigEntry{}, "Namespace", "Meta", "ModifyIndex", "CreateIndex"), cmpopts.IgnoreUnexported(), cmpopts.EquateEmpty())
 }
 
-func (in *ProxyDefaults) Validate() error {
+func (in *ProxyDefaults) Validate(namespacesEnabled bool) error {
 	var allErrs field.ErrorList
 	path := field.NewPath("spec")
 

diff --git a/api/v1alpha1/proxydefaults_webhook.go b/api/v1alpha1/proxydefaults_webhook.go
@@ -61,7 +61,7 @@ func (v *ProxyDefaultsWebhook) Handle(ctx context.Context, req admission.Request
 		}
 	}
 
-	if err := proxyDefaults.Validate(); err != nil {
+	if err := proxyDefaults.Validate(v.EnableConsulNamespaces); err != nil {
 		return admission.Errored(http.StatusBadRequest, err)
 	}
 	return admission.Allowed(fmt.Sprintf("valid %s request", proxyDefaults.KubeKind()))

diff --git a/api/v1alpha1/servicedefaults_types.go b/api/v1alpha1/servicedefaults_types.go
@@ -141,7 +141,7 @@ func (in *ServiceDefaults) ToConsul(datacenter string) capi.ConfigEntry {
 
 // Validate validates the fields provided in the spec of the ServiceDefaults and
 // returns an error which lists all invalid fields in the resource spec.
-func (in *ServiceDefaults) Validate() error {
+func (in *ServiceDefaults) Validate(namespacesEnabled bool) error {
 	var allErrs field.ErrorList
 	path := field.NewPath("spec")
 

diff --git a/api/v1alpha1/servicedefaults_types_test.go b/api/v1alpha1/servicedefaults_types_test.go
@@ -312,7 +312,7 @@ func TestServiceDefaults_Validate(t *testing.T) {
 
 	for name, testCase := range cases {
 		t.Run(name, func(t *testing.T) {
-			err := testCase.input.Validate()
+			err := testCase.input.Validate(false)
 			if testCase.expectedErrMsg != "" {
 				require.EqualError(t, err, testCase.expectedErrMsg)
 			} else {

diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go
@@ -248,7 +248,7 @@ func (in *ServiceIntentions) MatchesConsul(candidate api.ConfigEntry) bool {
 	)
 }
 
-func (in *ServiceIntentions) Validate() error {
+func (in *ServiceIntentions) Validate(namespacesEnabled bool) error {
 	var errs field.ErrorList
 	path := field.NewPath("spec")
 	if len(in.Spec.Sources) == 0 {
@@ -268,6 +268,9 @@ func (in *ServiceIntentions) Validate() error {
 			}
 		}
 	}
+
+	errs = append(errs, in.validateNamespaces(namespacesEnabled)...)
+
 	if len(errs) > 0 {
 		return apierrors.NewInvalid(
 			schema.GroupKind{Group: ConsulHashicorpGroup, Kind: common.ServiceIntentions},
@@ -316,9 +319,7 @@ func (in *ServiceIntentions) Default(consulNamespacesEnabled bool, destinationNa
 	}
 }
 
-// ValidateNamespaces returns an error if spec.destination.namespace or spec.sources[i].namespace
-// is set but namespaces are disabled.
-func (in *ServiceIntentions) ValidateNamespaces(namespacesEnabled bool) error {
+func (in *ServiceIntentions) validateNamespaces(namespacesEnabled bool) field.ErrorList {
 	var errs field.ErrorList
 	path := field.NewPath("spec")
 	if !namespacesEnabled {
@@ -331,12 +332,7 @@ func (in *ServiceIntentions) ValidateNamespaces(namespacesEnabled bool) error {
 			}
 		}
 	}
-	if len(errs) > 0 {
-		return apierrors.NewInvalid(
-			schema.GroupKind{Group: ConsulHashicorpGroup, Kind: common.ServiceIntentions},
-			in.KubernetesName(), errs)
-	}
-	return nil
+	return errs
 }
 
 func (in IntentionAction) validate(path *field.Path) *field.Error {