From 4e01b71018f2e00d533a270548b97894eb50b00a Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Mon, 2 Nov 2020 09:55:53 -0800 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + api/common/configentry.go | 2 +- api/common/configentry_webhook.go | 2 +- api/common/configentry_webhook_test.go | 2 +- api/v1alpha1/proxydefaults_types.go | 2 +- api/v1alpha1/proxydefaults_webhook.go | 2 +- api/v1alpha1/servicedefaults_types.go | 2 +- api/v1alpha1/servicedefaults_types_test.go | 2 +- api/v1alpha1/serviceintentions_types.go | 16 +- api/v1alpha1/serviceintentions_types_test.go | 197 ++++++++----------- api/v1alpha1/serviceintentions_webhook.go | 7 +- api/v1alpha1/serviceresolver_types.go | 23 ++- api/v1alpha1/serviceresolver_types_test.go | 92 ++++++++- api/v1alpha1/servicerouter_types.go | 19 +- api/v1alpha1/servicerouter_types_test.go | 75 ++++++- api/v1alpha1/servicesplitter_types.go | 18 +- api/v1alpha1/servicesplitter_types_test.go | 73 ++++++- 17 files changed, 364 insertions(+), 171 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f99bf92a5..7503935f53 100644 --- 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 index 40fbd604f4..5aec9ebee0 100644 --- 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 index e78182183c..10a9e46044 100644 --- 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 index 8fc062df8f..a8d7d754fe 100644 --- 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 index bdb7465978..3ff851602c 100644 --- 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 index 1451bf274c..61ac6cddb5 100644 --- 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 index 73d1b1276d..d4d727fd89 100644 --- 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 index c6736bb2e6..1f2e91866e 100644 --- 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 index 713bb5d546..e3532a77b2 100644 --- 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 { diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 07dc17ab70..23ea43eb23 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -532,11 +532,12 @@ func TestServiceIntentions_Default(t *testing.T) { func TestServiceIntentions_Validate(t *testing.T) { cases := map[string]struct { - input *ServiceIntentions - expectedErrMsg string + input *ServiceIntentions + namespacesEnabled bool + expectedErrMsg []string }{ "valid": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -589,10 +590,11 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - "", + namespacesEnabled: true, + expectedErrMsg: []string{}, }, "no sources": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -604,10 +606,13 @@ func TestServiceIntentions_Validate(t *testing.T) { Sources: SourceIntentions{}, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources: Required value: at least one source must be specified`, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources: Required value: at least one source must be specified`, + }, }, "invalid action": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -625,10 +630,13 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].action: Invalid value: "foo": must be one of "allow", "deny"`, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].action: Invalid value: "foo": must be one of "allow", "deny"`, + }, }, "invalid permissions.http.pathPrefix": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -653,10 +661,13 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathPrefix: Invalid value: "bar": must begin with a '/'`, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathPrefix: Invalid value: "bar": must begin with a '/'`, + }, }, "invalid permissions.http.pathExact": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -681,10 +692,13 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, + }, }, "invalid permissions.action": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -709,10 +723,13 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].action: Invalid value: "foobar": must be one of "allow", "deny"`, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].action: Invalid value: "foobar": must be one of "allow", "deny"`, + }, }, "both action and permissions specified": { - &ServiceIntentions{ + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -738,94 +755,20 @@ func TestServiceIntentions_Validate(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0]: Invalid value: "{\"name\":\"svc-2\",\"namespace\":\"bar\",\"action\":\"deny\",\"permissions\":[{\"action\":\"allow\",\"http\":{\"pathExact\":\"/bar\"}}]}": action and permissions are mutually exclusive and only one of them can be specified`, - }, - } - for name, testCase := range cases { - t.Run(name, func(t *testing.T) { - err := testCase.input.Validate() - if testCase.expectedErrMsg != "" { - require.EqualError(t, err, testCase.expectedErrMsg) - } else { - require.NoError(t, err) - } - }) - } -} - -func TestServiceIntentions_ValidateNamespaces(t *testing.T) { - cases := map[string]struct { - namespacesEnabled bool - input *ServiceIntentions - expectedErrMsg string - }{ - "enabled: valid": { - true, - &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "does-not-matter", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "dest-service", - Namespace: "namespace", - }, - Sources: SourceIntentions{ - { - Name: "web", - Namespace: "web", - Action: "allow", - }, - { - Name: "db", - Namespace: "db", - Action: "deny", - }, - { - Name: "bar", - Namespace: "bar", - Permissions: IntentionPermissions{ - { - Action: "allow", - HTTP: &IntentionHTTPPermission{ - PathExact: "/foo", - PathPrefix: "/bar", - PathRegex: "/baz", - Header: IntentionHTTPHeaderPermissions{ - { - Name: "header", - Present: true, - Exact: "exact", - Prefix: "prefix", - Suffix: "suffix", - Regex: "regex", - Invert: true, - }, - }, - Methods: []string{ - "GET", - "PUT", - }, - }, - }, - }, - Description: "an L7 config", - }, - }, - }, + namespacesEnabled: true, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0]: Invalid value: "{\"name\":\"svc-2\",\"namespace\":\"bar\",\"action\":\"deny\",\"permissions\":[{\"action\":\"allow\",\"http\":{\"pathExact\":\"/bar\"}}]}": action and permissions are mutually exclusive and only one of them can be specified`, }, - "", }, - "disabled: destination namespace specified": { - false, - &ServiceIntentions{ + "namespaces disabled: destination namespace specified": { + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ Name: "dest-service", - Namespace: "foo", + Namespace: "namespaceA", }, Sources: SourceIntentions{ { @@ -868,11 +811,13 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.destination.namespace: Invalid value: "foo": consul namespaces must be enabled to set destination.namespace`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.destination.namespace: Invalid value: "namespaceA": consul namespaces must be enabled to set destination.namespace`, + }, }, - "disabled: single source namespace specified": { - false, - &ServiceIntentions{ + "namespaces disabled: single source namespace specified": { + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -884,7 +829,7 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { { Name: "web", Action: "allow", - Namespace: "bar", + Namespace: "namespaceA", }, { Name: "db", @@ -922,11 +867,13 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].namespace: Invalid value: "bar": consul namespaces must be enabled to set source.namespace`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].namespace: Invalid value: "namespaceA": consul namespaces must be enabled to set source.namespace`, + }, }, - "disabled: multiple source namespace specified": { - false, - &ServiceIntentions{ + "namespaces disabled: multiple source namespaces specified": { + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, @@ -938,16 +885,16 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { { Name: "web", Action: "allow", - Namespace: "bar", + Namespace: "namespaceA", }, { Name: "db", Action: "deny", - Namespace: "baz", + Namespace: "namespaceB", }, { Name: "bar", - Namespace: "baz", + Namespace: "namespaceC", Permissions: IntentionPermissions{ { Action: "allow", @@ -978,33 +925,37 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].namespace: Invalid value: "bar": consul namespaces must be enabled to set source.namespace, spec.sources[1].namespace: Invalid value: "baz": consul namespaces must be enabled to set source.namespace, spec.sources[2].namespace: Invalid value: "baz": consul namespaces must be enabled to set source.namespace]`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `namespace: Invalid value: "namespaceA": consul namespaces must be enabled to set source.namespace`, + `namespace: Invalid value: "namespaceB": consul namespaces must be enabled to set source.namespace`, + `namespace: Invalid value: "namespaceC": consul namespaces must be enabled to set source.namespace`, + }, }, - "disabled: multiple source and destination namespace specified": { - false, - &ServiceIntentions{ + "namespaces disabled: destination and multiple source namespaces specified": { + input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ Name: "dest-service", - Namespace: "foo", + Namespace: "namespaceA", }, Sources: SourceIntentions{ { Name: "web", Action: "allow", - Namespace: "bar", + Namespace: "namespaceB", }, { Name: "db", Action: "deny", - Namespace: "baz", + Namespace: "namespaceC", }, { Name: "bar", - Namespace: "baz", + Namespace: "namespaceD", Permissions: IntentionPermissions{ { Action: "allow", @@ -1035,14 +986,22 @@ func TestServiceIntentions_ValidateNamespaces(t *testing.T) { }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.destination.namespace: Invalid value: "foo": consul namespaces must be enabled to set destination.namespace, spec.sources[0].namespace: Invalid value: "bar": consul namespaces must be enabled to set source.namespace, spec.sources[1].namespace: Invalid value: "baz": consul namespaces must be enabled to set source.namespace, spec.sources[2].namespace: Invalid value: "baz": consul namespaces must be enabled to set source.namespace]`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `spec.destination.namespace: Invalid value: "namespaceA": consul namespaces must be enabled to set destination.namespace`, + `namespace: Invalid value: "namespaceB": consul namespaces must be enabled to set source.namespace`, + `namespace: Invalid value: "namespaceC": consul namespaces must be enabled to set source.namespace`, + `namespace: Invalid value: "namespaceD": consul namespaces must be enabled to set source.namespace`, + }, }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { - err := testCase.input.ValidateNamespaces(testCase.namespacesEnabled) - if testCase.expectedErrMsg != "" { - require.EqualError(t, err, testCase.expectedErrMsg) + err := testCase.input.Validate(testCase.namespacesEnabled) + if len(testCase.expectedErrMsg) != 0 { + for _, s := range testCase.expectedErrMsg { + require.Contains(t, err.Error(), s) + } } else { require.NoError(t, err) } diff --git a/api/v1alpha1/serviceintentions_webhook.go b/api/v1alpha1/serviceintentions_webhook.go index b342667752..7d790e3a82 100644 --- a/api/v1alpha1/serviceintentions_webhook.go +++ b/api/v1alpha1/serviceintentions_webhook.go @@ -89,12 +89,7 @@ func (v *ServiceIntentionsWebhook) Handle(ctx context.Context, req admission.Req } } - if err := svcIntentions.Validate(); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - - // ServiceIntentions are invalid if destination namespaces or source namespaces are set when Consul Namespaces are not enabled. - if err := svcIntentions.ValidateNamespaces(v.EnableConsulNamespaces); err != nil { + if err := svcIntentions.Validate(v.EnableConsulNamespaces); err != nil { return admission.Errored(http.StatusBadRequest, err) } diff --git a/api/v1alpha1/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go index 88a4c604fe..16754050a3 100644 --- a/api/v1alpha1/serviceresolver_types.go +++ b/api/v1alpha1/serviceresolver_types.go @@ -128,7 +128,7 @@ func (in *ServiceResolver) ConsulGlobalResource() bool { return false } -func (in *ServiceResolver) Validate() error { +func (in *ServiceResolver) Validate(namespacesEnabled bool) error { var errs field.ErrorList path := field.NewPath("spec") @@ -148,6 +148,8 @@ func (in *ServiceResolver) Validate() error { errs = append(errs, in.Spec.LoadBalancer.validate(path.Child("loadBalancer"))...) + errs = append(errs, in.validateNamespaces(namespacesEnabled)...) + if len(errs) > 0 { return apierrors.NewInvalid( schema.GroupKind{Group: ConsulHashicorpGroup, Kind: ServiceResolverKubeKind}, @@ -156,6 +158,25 @@ func (in *ServiceResolver) Validate() error { return nil } +func (in *ServiceResolver) validateNamespaces(namespacesEnabled bool) field.ErrorList { + var errs field.ErrorList + path := field.NewPath("spec") + if !namespacesEnabled { + if in.Spec.Redirect != nil { + if in.Spec.Redirect.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("redirect").Child("namespace"), in.Spec.Redirect.Namespace, `consul namespaces must be enabled to set redirect.namespace`)) + } + } + for k, v := range in.Spec.Failover { + if v.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("failover").Key(k).Child("namespace"), v.Namespace, `consul namespaces must be enabled to set failover.namespace`)) + } + } + + } + return errs +} + func (in *ServiceResolverFailover) validate(path *field.Path) *field.Error { if in.Service == "" && in.ServiceSubset == "" && in.Namespace == "" && len(in.Datacenters) == 0 { // NOTE: We're passing "{}" here as our value because we know that the diff --git a/api/v1alpha1/serviceresolver_types_test.go b/api/v1alpha1/serviceresolver_types_test.go index c48aa3a324..50f2c081ba 100644 --- a/api/v1alpha1/serviceresolver_types_test.go +++ b/api/v1alpha1/serviceresolver_types_test.go @@ -445,8 +445,9 @@ func TestServiceResolver_ObjectMeta(t *testing.T) { func TestServiceResolver_Validate(t *testing.T) { cases := map[string]struct { - input *ServiceResolver - expectedErrMsg string + input *ServiceResolver + namespacesEnabled bool + expectedErrMsg []string }{ "valid": { input: &ServiceResolver{ @@ -459,7 +460,8 @@ func TestServiceResolver_Validate(t *testing.T) { }, }, }, - expectedErrMsg: "", + namespacesEnabled: false, + expectedErrMsg: []string{}, }, "failover service, servicesubset, namespace, datacenters empty": { input: &ServiceResolver{ @@ -483,7 +485,10 @@ func TestServiceResolver_Validate(t *testing.T) { }, }, }, - expectedErrMsg: "serviceresolver.consul.hashicorp.com \"foo\" is invalid: [spec.failover[failA]: Invalid value: \"{}\": service, serviceSubset, namespace and datacenters cannot all be empty at once, spec.failover[failB]: Invalid value: \"{}\": service, serviceSubset, namespace and datacenters cannot all be empty at once]", + namespacesEnabled: false, + expectedErrMsg: []string{ + "serviceresolver.consul.hashicorp.com \"foo\" is invalid: [spec.failover[failA]: Invalid value: \"{}\": service, serviceSubset, namespace and datacenters cannot all be empty at once, spec.failover[failB]: Invalid value: \"{}\": service, serviceSubset, namespace and datacenters cannot all be empty at once]", + }, }, "hashPolicy.field invalid": { input: &ServiceResolver{ @@ -500,7 +505,10 @@ func TestServiceResolver_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].field: Invalid value: "invalid": must be one of "header", "cookie", "query_parameter"`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].field: Invalid value: "invalid": must be one of "header", "cookie", "query_parameter"`, + }, }, "hashPolicy sourceIP and field set": { input: &ServiceResolver{ @@ -518,7 +526,10 @@ func TestServiceResolver_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0]: Invalid value: "{\"field\":\"header\",\"sourceIP\":true}": cannot set both field and sourceIP`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0]: Invalid value: "{\"field\":\"header\",\"sourceIP\":true}": cannot set both field and sourceIP`, + }, }, "cookieConfig session and ttl set": { input: &ServiceResolver{ @@ -539,14 +550,75 @@ func TestServiceResolver_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].cookieConfig: Invalid value: "{\"session\":true,\"ttl\":100}": cannot set both session and ttl`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].cookieConfig: Invalid value: "{\"session\":true,\"ttl\":100}": cannot set both session and ttl`, + }, + }, + "namespaces disabled: redirect namespace specified": { + input: &ServiceResolver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceResolverSpec{ + Redirect: &ServiceResolverRedirect{ + Namespace: "namespaceA", + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "serviceresolver.consul.hashicorp.com \"foo\" is invalid: spec.redirect.namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set redirect.namespace", + }, + }, + "namespaces disabled: single failover namespace specified": { + input: &ServiceResolver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceResolverSpec{ + Failover: map[string]ServiceResolverFailover{ + "failA": { + Namespace: "namespaceA", + }, + }, + }, + }, + expectedErrMsg: []string{ + "serviceresolver.consul.hashicorp.com \"foo\" is invalid: spec.failover[failA].namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set failover.namespace", + }, + namespacesEnabled: false, + }, + "namespaces disabled: multiple failover namespaces specified": { + input: &ServiceResolver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceResolverSpec{ + Failover: map[string]ServiceResolverFailover{ + "failA": { + Namespace: "namespaceA", + }, + "failB": { + Namespace: "namespaceB", + }, + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "spec.failover[failA].namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set failover.namespace", + "spec.failover[failB].namespace: Invalid value: \"namespaceB\": consul namespaces must be enabled to set failover.namespace", + }, }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { - err := testCase.input.Validate() - if testCase.expectedErrMsg != "" { - require.EqualError(t, err, testCase.expectedErrMsg) + err := testCase.input.Validate(testCase.namespacesEnabled) + if len(testCase.expectedErrMsg) != 0 { + for _, s := range testCase.expectedErrMsg { + require.Contains(t, err.Error(), s) + } } else { require.NoError(t, err) } diff --git a/api/v1alpha1/servicerouter_types.go b/api/v1alpha1/servicerouter_types.go index 2b31f55267..bae63b3771 100644 --- a/api/v1alpha1/servicerouter_types.go +++ b/api/v1alpha1/servicerouter_types.go @@ -376,7 +376,7 @@ func (in *ServiceRouter) MatchesConsul(candidate capi.ConfigEntry) bool { return cmp.Equal(in.ToConsul(""), configEntry, cmpopts.IgnoreFields(capi.ServiceRouterConfigEntry{}, "Namespace", "Meta", "ModifyIndex", "CreateIndex"), cmpopts.IgnoreUnexported(), cmpopts.EquateEmpty()) } -func (in *ServiceRouter) Validate() error { +func (in *ServiceRouter) Validate(namespacesEnabled bool) error { var errs field.ErrorList path := field.NewPath("spec") for i, r := range in.Spec.Routes { @@ -385,6 +385,8 @@ func (in *ServiceRouter) Validate() error { } } + errs = append(errs, in.validateNamespaces(namespacesEnabled)...) + if len(errs) > 0 { return apierrors.NewInvalid( schema.GroupKind{Group: ConsulHashicorpGroup, Kind: ServiceRouterKubeKind}, @@ -393,6 +395,21 @@ func (in *ServiceRouter) Validate() error { return nil } +func (in *ServiceRouter) validateNamespaces(namespacesEnabled bool) field.ErrorList { + var errs field.ErrorList + path := field.NewPath("spec") + if !namespacesEnabled { + for i, r := range in.Spec.Routes { + if r.Destination != nil { + if r.Destination.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("routes").Index(i).Child("destination").Child("namespace"), r.Destination.Namespace, `consul namespaces must be enabled to set destination.namespace`)) + } + } + } + } + return errs +} + // +kubebuilder:object:root=true // ServiceRouterList contains a list of ServiceRouter diff --git a/api/v1alpha1/servicerouter_types_test.go b/api/v1alpha1/servicerouter_types_test.go index 2f619e4070..5adb5f96c6 100644 --- a/api/v1alpha1/servicerouter_types_test.go +++ b/api/v1alpha1/servicerouter_types_test.go @@ -390,8 +390,9 @@ func TestServiceRouter_ObjectMeta(t *testing.T) { func TestServiceRouter_Validate(t *testing.T) { cases := map[string]struct { - input *ServiceRouter - expectedErrMsg string + input *ServiceRouter + namespacesEnabled bool + expectedErrMsg []string }{ "valid": { input: &ServiceRouter{ @@ -410,6 +411,7 @@ func TestServiceRouter_Validate(t *testing.T) { }, }, }, + namespacesEnabled: false, }, "http match queryParam": { input: &ServiceRouter{ @@ -438,7 +440,10 @@ func TestServiceRouter_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `servicerouter.consul.hashicorp.com "foo" is invalid: [spec.routes[0].match.http: Invalid value: "{\"pathExact\":\"exact\",\"pathPrefix\":\"prefix\",\"pathRegex\":\"regex\",\"queryParam\":[{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"regex\":\"regex\"}]}": at most only one of pathExact, pathPrefix, or pathRegex may be configured, spec.routes[0].match.http.pathExact: Invalid value: "exact": must begin with a '/', spec.routes[0].match.http.pathPrefix: Invalid value: "prefix": must begin with a '/', spec.routes[0].match.http.queryParam[0]: Invalid value: "{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"regex\":\"regex\"}": at most only one of exact, regex, or present may be configured]`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `servicerouter.consul.hashicorp.com "foo" is invalid: [spec.routes[0].match.http: Invalid value: "{\"pathExact\":\"exact\",\"pathPrefix\":\"prefix\",\"pathRegex\":\"regex\",\"queryParam\":[{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"regex\":\"regex\"}]}": at most only one of pathExact, pathPrefix, or pathRegex may be configured, spec.routes[0].match.http.pathExact: Invalid value: "exact": must begin with a '/', spec.routes[0].match.http.pathPrefix: Invalid value: "prefix": must begin with a '/', spec.routes[0].match.http.queryParam[0]: Invalid value: "{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"regex\":\"regex\"}": at most only one of exact, regex, or present may be configured]`, + }, }, "http match header": { input: &ServiceRouter{ @@ -469,7 +474,10 @@ func TestServiceRouter_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `servicerouter.consul.hashicorp.com "foo" is invalid: [spec.routes[0].match.http: Invalid value: "{\"pathExact\":\"exact\",\"pathPrefix\":\"prefix\",\"pathRegex\":\"regex\",\"header\":[{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"prefix\":\"prefix\",\"suffix\":\"suffix\",\"regex\":\"regex\"}]}": at most only one of pathExact, pathPrefix, or pathRegex may be configured, spec.routes[0].match.http.pathExact: Invalid value: "exact": must begin with a '/', spec.routes[0].match.http.pathPrefix: Invalid value: "prefix": must begin with a '/', spec.routes[0].match.http.header[0]: Invalid value: "{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"prefix\":\"prefix\",\"suffix\":\"suffix\",\"regex\":\"regex\"}": at most only one of exact, prefix, suffix, regex, or present may be configured]`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `servicerouter.consul.hashicorp.com "foo" is invalid: [spec.routes[0].match.http: Invalid value: "{\"pathExact\":\"exact\",\"pathPrefix\":\"prefix\",\"pathRegex\":\"regex\",\"header\":[{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"prefix\":\"prefix\",\"suffix\":\"suffix\",\"regex\":\"regex\"}]}": at most only one of pathExact, pathPrefix, or pathRegex may be configured, spec.routes[0].match.http.pathExact: Invalid value: "exact": must begin with a '/', spec.routes[0].match.http.pathPrefix: Invalid value: "prefix": must begin with a '/', spec.routes[0].match.http.header[0]: Invalid value: "{\"name\":\"name\",\"present\":true,\"exact\":\"exact\",\"prefix\":\"prefix\",\"suffix\":\"suffix\",\"regex\":\"regex\"}": at most only one of exact, prefix, suffix, regex, or present may be configured]`, + }, }, "destination and prefixRewrite": { input: &ServiceRouter{ @@ -496,14 +504,65 @@ func TestServiceRouter_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `servicerouter.consul.hashicorp.com "foo" is invalid: spec.routes[0]: Invalid value: "{\"match\":{\"http\":{}},\"destination\":{\"prefixRewrite\":\"prefixRewrite\"}}": destination.prefixRewrite requires that either match.http.pathPrefix or match.http.pathExact be configured on this route`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `servicerouter.consul.hashicorp.com "foo" is invalid: spec.routes[0]: Invalid value: "{\"match\":{\"http\":{}},\"destination\":{\"prefixRewrite\":\"prefixRewrite\"}}": destination.prefixRewrite requires that either match.http.pathPrefix or match.http.pathExact be configured on this route`, + }, + }, + "namespaces disabled: single destination namespace specified": { + input: &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Destination: &ServiceRouteDestination{ + Namespace: "namespaceA", + }, + }, + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "servicerouter.consul.hashicorp.com \"foo\" is invalid: spec.routes[0].destination.namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set destination.namespace", + }, + }, + "namespaces disabled: multiple destination namespaces specified": { + input: &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Destination: &ServiceRouteDestination{ + Namespace: "namespaceA", + }, + }, + { + Destination: &ServiceRouteDestination{ + Namespace: "namespaceB", + }, + }, + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "destination.namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set destination.namespace", + "destination.namespace: Invalid value: \"namespaceB\": consul namespaces must be enabled to set destination.namespace", + }, }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { - err := testCase.input.Validate() - if testCase.expectedErrMsg != "" { - require.EqualError(t, err, testCase.expectedErrMsg) + err := testCase.input.Validate(testCase.namespacesEnabled) + if len(testCase.expectedErrMsg) != 0 { + for _, s := range testCase.expectedErrMsg { + require.Contains(t, err.Error(), s) + } } else { require.NoError(t, err) } diff --git a/api/v1alpha1/servicesplitter_types.go b/api/v1alpha1/servicesplitter_types.go index b4fa643cca..67de911205 100644 --- a/api/v1alpha1/servicesplitter_types.go +++ b/api/v1alpha1/servicesplitter_types.go @@ -153,9 +153,11 @@ func (in *ServiceSplitter) MatchesConsul(candidate capi.ConfigEntry) bool { return cmp.Equal(in.ToConsul(""), configEntry, cmpopts.IgnoreFields(capi.ServiceSplitterConfigEntry{}, "Namespace", "Meta", "ModifyIndex", "CreateIndex"), cmpopts.IgnoreUnexported(), cmpopts.EquateEmpty()) } -func (in *ServiceSplitter) Validate() error { +func (in *ServiceSplitter) Validate(namespacesEnabled bool) error { errs := in.Spec.Splits.validate(field.NewPath("spec").Child("splits")) + errs = append(errs, in.validateNamespaces(namespacesEnabled)...) + if len(errs) > 0 { return apierrors.NewInvalid( schema.GroupKind{Group: ConsulHashicorpGroup, Kind: in.KubeKind()}, @@ -164,6 +166,20 @@ func (in *ServiceSplitter) Validate() error { return nil } +func (in *ServiceSplitter) validateNamespaces(namespacesEnabled bool) field.ErrorList { + var errs field.ErrorList + path := field.NewPath("spec") + if !namespacesEnabled { + for i, s := range in.Spec.Splits { + if s.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("splits").Index(i).Child("namespace"), s.Namespace, `consul namespaces must be enabled to set split.namespace`)) + } + + } + } + return errs +} + func (in ServiceSplits) validate(path *field.Path) field.ErrorList { var errs field.ErrorList diff --git a/api/v1alpha1/servicesplitter_types_test.go b/api/v1alpha1/servicesplitter_types_test.go index 7d82d3f0ed..0420c7b591 100644 --- a/api/v1alpha1/servicesplitter_types_test.go +++ b/api/v1alpha1/servicesplitter_types_test.go @@ -256,8 +256,9 @@ func TestServiceSplitter_ObjectMeta(t *testing.T) { func TestServiceSplitter_Validate(t *testing.T) { cases := map[string]struct { - input *ServiceSplitter - expectedErrMsg string + input *ServiceSplitter + namespacesEnabled bool + expectedErrMsg []string }{ "valid": { input: &ServiceSplitter{ @@ -275,8 +276,9 @@ func TestServiceSplitter_Validate(t *testing.T) { }, }, }, + namespacesEnabled: false, + expectedErrMsg: []string{}, }, - "valid - splits with 0 weight": { input: &ServiceSplitter{ ObjectMeta: metav1.ObjectMeta{ @@ -299,6 +301,8 @@ func TestServiceSplitter_Validate(t *testing.T) { }, }, }, + namespacesEnabled: false, + expectedErrMsg: []string{}, }, "sum of weights must be 100": { input: &ServiceSplitter{ @@ -316,7 +320,10 @@ func TestServiceSplitter_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `servicesplitter.consul.hashicorp.com "foo" is invalid: spec.splits: Invalid value: "[{\"weight\":90},{\"weight\":5}]": the sum of weights across all splits must add up to 100 percent, but adds up to 95.000000`, + namespacesEnabled: false, + expectedErrMsg: []string{ + `servicesplitter.consul.hashicorp.com "foo" is invalid: spec.splits: Invalid value: "[{\"weight\":90},{\"weight\":5}]": the sum of weights across all splits must add up to 100 percent, but adds up to 95.000000`, + }, }, "weight must be between 0.01 and 100": { input: &ServiceSplitter{ @@ -334,14 +341,64 @@ func TestServiceSplitter_Validate(t *testing.T) { }, }, }, - expectedErrMsg: `servicesplitter.consul.hashicorp.com "foo" is invalid: [spec.splits[0].weight: Invalid value: 101: weight must be a percentage between 0.01 and 100, spec.splits[1].weight: Invalid value: 0.001: weight must be a percentage between 0.01 and 100, spec.splits: Invalid value: "[{\"weight\":101},{\"weight\":0.001}]": the sum of weights across all splits must add up to 100 percent, but adds up to 101.000999]`, + namespacesEnabled: false, + expectedErrMsg: []string{ + "spec.splits[0].weight: Invalid value: 101: weight must be a percentage between 0.01 and 100", + "spec.splits[1].weight: Invalid value: 0.001: weight must be a percentage between 0.01 and 100", + `spec.splits: Invalid value: "[{\"weight\":101},{\"weight\":0.001}]": the sum of weights across all splits must add up to 100 percent, but adds up to 101.000999]`, + }, + }, + "namespaces disabled: single split namespace specified": { + input: &ServiceSplitter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceSplitterSpec{ + Splits: []ServiceSplit{ + { + Namespace: "namespaceA", + Weight: 100, + }, + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "servicesplitter.consul.hashicorp.com \"foo\" is invalid: spec.splits[0].namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set split.namespace", + }, + }, + "namespaces disabled: multiple split namespaces specified": { + input: &ServiceSplitter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: ServiceSplitterSpec{ + Splits: []ServiceSplit{ + { + Namespace: "namespaceA", + Weight: 50, + }, + { + Namespace: "namespaceB", + Weight: 50, + }, + }, + }, + }, + namespacesEnabled: false, + expectedErrMsg: []string{ + "namespace: Invalid value: \"namespaceA\": consul namespaces must be enabled to set split.namespace", + "namespace: Invalid value: \"namespaceB\": consul namespaces must be enabled to set split.namespace", + }, }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { - err := testCase.input.Validate() - if testCase.expectedErrMsg != "" { - require.EqualError(t, err, testCase.expectedErrMsg) + err := testCase.input.Validate(testCase.namespacesEnabled) + if len(testCase.expectedErrMsg) != 0 { + for _, s := range testCase.expectedErrMsg { + require.Contains(t, err.Error(), s) + } } else { require.NoError(t, err) }