diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 073d4e0dec..27cea16848 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -6,6 +6,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/consul-k8s/api/common" + "github.com/hashicorp/consul-k8s/namespaces" "github.com/hashicorp/consul/api" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" @@ -152,10 +153,11 @@ func (in *ServiceIntentions) SyncedConditionStatus() corev1.ConditionStatus { func (in *ServiceIntentions) ToConsul(datacenter string) api.ConfigEntry { return &capi.ServiceIntentionsConfigEntry{ - Kind: in.ConsulKind(), - Name: in.Spec.Destination.Name, - Sources: in.Spec.Sources.toConsul(), - Meta: meta(datacenter), + Kind: in.ConsulKind(), + Name: in.Spec.Destination.Name, + Namespace: in.Spec.Destination.Namespace, + Sources: in.Spec.Sources.toConsul(), + Meta: meta(datacenter), } } @@ -296,22 +298,41 @@ func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { return errs } -// Default sets zero value fields on this object to their defaults. -func (in *ServiceIntentions) Default(consulNamespacesEnabled bool) { - // If namespaces are enabled we want to set all the namespace fields to their - // defaults. If namespaces are not enabled (i.e. OSS) we don't set any +// Default sets the namespace field on spec.destination to their default values if namespaces are enabled. +func (in *ServiceIntentions) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the destination namespace field to it's + // default. If namespaces are not enabled (i.e. OSS) we don't set the // namespace fields because this would cause errors // making API calls (because namespace fields can't be set in OSS). if consulNamespacesEnabled { + namespace := namespaces.ConsulNamespace(in.Namespace, consulNamespacesEnabled, destinationNamespace, mirroring, prefix) if in.Spec.Destination.Namespace == "" { - in.Spec.Destination.Namespace = in.Namespace + in.Spec.Destination.Namespace = namespace } - for _, source := range in.Spec.Sources { - if source.Namespace == "" { - source.Namespace = in.Namespace + } +} + +// 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 { + var errs field.ErrorList + path := field.NewPath("spec") + if !namespacesEnabled { + if in.Spec.Destination.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("destination").Child("namespace"), in.Spec.Destination.Namespace, `consul namespaces must be enabled to set destination.namespace`)) + } + for i, source := range in.Spec.Sources { + if source.Namespace != "" { + errs = append(errs, field.Invalid(path.Child("sources").Index(i).Child("namespace"), source.Namespace, `consul namespaces must be enabled to set source.namespace`)) } } } + if len(errs) > 0 { + return apierrors.NewInvalid( + schema.GroupKind{Group: ConsulHashicorpGroup, Kind: common.ServiceIntentions}, + in.KubernetesName(), errs) + } + return nil } 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 e4d38975ec..58ed5632d3 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -1,7 +1,6 @@ package v1alpha1 import ( - "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -199,7 +198,8 @@ func TestServiceIntentions_ToConsul(t *testing.T) { }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "svc-name", + Name: "svc-name", + Namespace: "dest-ns", }, Sources: []*SourceIntention{ { @@ -248,8 +248,9 @@ func TestServiceIntentions_ToConsul(t *testing.T) { }, }, Exp: &capi.ServiceIntentionsConfigEntry{ - Kind: capi.ServiceIntentions, - Name: "svc-name", + Kind: capi.ServiceIntentions, + Name: "svc-name", + Namespace: "dest-ns", Sources: []*capi.SourceIntention{ { Name: "svc1", @@ -458,214 +459,298 @@ func TestServiceIntentions_ObjectMeta(t *testing.T) { require.Equal(t, meta, serviceResolver.GetObjectMeta()) } -// Test defaulting behavior for both OSS and enterprise. +// Test defaulting behavior when namespaces are enabled as well as disabled. func TestServiceIntentions_Default(t *testing.T) { - cases := map[string]struct { - input *ServiceIntentions - output *ServiceIntentions + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + sourceNamespace string + expectedDestination string }{ - "destination.namespace blank, meta.namespace default": { - input: &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", - }, - }, + "disabled": { + enabled: false, + destinationNamespace: "", + mirroring: false, + prefix: "", + sourceNamespace: "bar", + expectedDestination: "", + }, + "destinationNS": { + enabled: true, + destinationNamespace: "foo", + mirroring: false, + prefix: "", + sourceNamespace: "bar", + expectedDestination: "foo", + }, + "mirroringEnabledWithoutPrefix": { + enabled: true, + destinationNamespace: "", + mirroring: true, + prefix: "", + sourceNamespace: "bar", + expectedDestination: "bar", + }, + "mirroringWithPrefix": { + enabled: true, + destinationNamespace: "", + mirroring: true, + prefix: "ns-", + sourceNamespace: "bar", + expectedDestination: "ns-bar", + }, + } + + for name, s := range namespaceConfig { + input := &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: s.sourceNamespace, }, - output: &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", - Namespace: "default", - }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", }, }, - }, - "destination.namespace blank, meta.namespace foobar": { - input: &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", - }, - }, + } + output := &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: s.sourceNamespace, }, - output: &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", - Namespace: "foobar", - }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", + Namespace: s.expectedDestination, }, }, - }, - "sources.namespace blank, meta.namespace default": { - input: &ServiceIntentions{ + } + + t.Run(name, func(t *testing.T) { + input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + require.True(t, cmp.Equal(input, output)) + }) + } +} + +func TestServiceIntentions_Validate(t *testing.T) { + cases := map[string]struct { + input *ServiceIntentions + expectedErrMsg string + }{ + "valid": { + &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", + 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", }, }, }, }, - output: &ServiceIntentions{ + "", + }, + "invalid action": { + &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", - Namespace: "default", + Name: "web", + Namespace: "web", + Action: "foo", }, }, }, }, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].action: Invalid value: "foo": must be one of "allow", "deny"`, }, - "sources.namespace blank, meta.namespace foobar": { - input: &ServiceIntentions{ + "invalid permissions.http.pathPrefix": { + &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathPrefix: "bar", + }, + }, + }, }, }, }, }, - output: &ServiceIntentions{ + `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{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", - Namespace: "foobar", + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathExact: "bar", + }, + }, + }, }, }, }, }, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, }, - "only populated blank namespaces": { - input: &ServiceIntentions{ + "invalid permissions.action": { + &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", - }, - { - Name: "baz2", - Action: "allow", - Namespace: "another-namespace", + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "foobar", + HTTP: &IntentionHTTPPermission{ + PathExact: "/bar", + }, + }, + }, }, }, }, }, - output: &ServiceIntentions{ + `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{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "foobar", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", + Name: "dest-service", + Namespace: "namespace", }, Sources: SourceIntentions{ { - Name: "baz", - Action: "allow", - Namespace: "foobar", - }, - { - Name: "baz2", - Action: "allow", - Namespace: "another-namespace", + Name: "svc-2", + Namespace: "bar", + Action: "deny", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathExact: "/bar", + }, + }, + }, }, }, }, }, + `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 { - // We also test with consul namespaces enabled/disabled. When disabled, - // the input shouldn't change since we don't set any namespace defaults - // in OSS. - for _, namespacesEnabled := range []bool{false, true} { - testName := fmt.Sprintf("%s namespaces=%t", name, namespacesEnabled) - t.Run(testName, func(t *testing.T) { - testCase.input.Default(namespacesEnabled) - if namespacesEnabled { - require.True(t, cmp.Equal(testCase.input, testCase.output)) - } else { - require.True(t, cmp.Equal(testCase.input, testCase.input)) - } - }) - } + 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_Validate(t *testing.T) { +func TestServiceIntentions_ValidateNamespaces(t *testing.T) { cases := map[string]struct { - input *ServiceIntentions - expectedErrMsg string + namespacesEnabled bool + input *ServiceIntentions + expectedErrMsg string }{ - "valid": { + "enabled: valid": { + true, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", @@ -721,7 +806,8 @@ func TestServiceIntentions_Validate(t *testing.T) { }, "", }, - "invalid action": { + "disabled: destination namespace specified": { + false, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", @@ -729,104 +815,163 @@ func TestServiceIntentions_Validate(t *testing.T) { Spec: ServiceIntentionsSpec{ Destination: Destination{ Name: "dest-service", - Namespace: "namespace", + Namespace: "foo", }, Sources: SourceIntentions{ { - Name: "web", - Namespace: "web", - Action: "foo", + Name: "web", + Action: "allow", }, - }, - }, - }, - `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{ - ObjectMeta: metav1.ObjectMeta{ - Name: "does-not-matter", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "dest-service", - Namespace: "namespace", - }, - Sources: SourceIntentions{ { - Name: "svc-2", - Namespace: "bar", + Name: "db", + Action: "deny", + }, + { + Name: "bar", Permissions: IntentionPermissions{ { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathPrefix: "bar", + 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", }, }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathPrefix: Invalid value: "bar": must begin with a '/'`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.destination.namespace: Invalid value: "foo": consul namespaces must be enabled to set destination.namespace`, }, - "invalid permissions.http.pathExact": { + "disabled: single source namespace specified": { + false, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "dest-service", - Namespace: "namespace", + Name: "dest-service", }, Sources: SourceIntentions{ { - Name: "svc-2", + Name: "web", + Action: "allow", Namespace: "bar", + }, + { + Name: "db", + Action: "deny", + }, + { + Name: "bar", Permissions: IntentionPermissions{ { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "bar", + 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", }, }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, + `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`, }, - "invalid permissions.action": { + "disabled: multiple source namespace specified": { + false, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "dest-service", - Namespace: "namespace", + Name: "dest-service", }, Sources: SourceIntentions{ { - Name: "svc-2", + Name: "web", + Action: "allow", Namespace: "bar", + }, + { + Name: "db", + Action: "deny", + Namespace: "baz", + }, + { + Name: "bar", + Namespace: "baz", Permissions: IntentionPermissions{ { - Action: "foobar", + Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/bar", + 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", }, }, }, }, - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].action: Invalid value: "foobar": must be one of "allow", "deny"`, + `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]`, }, - "both action and permissions specified": { + "disabled: multiple source and destination namespace specified": { + false, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", @@ -834,31 +979,58 @@ func TestServiceIntentions_Validate(t *testing.T) { Spec: ServiceIntentionsSpec{ Destination: Destination{ Name: "dest-service", - Namespace: "namespace", + Namespace: "foo", }, Sources: SourceIntentions{ { - Name: "svc-2", + Name: "web", + Action: "allow", Namespace: "bar", + }, + { + Name: "db", Action: "deny", + Namespace: "baz", + }, + { + Name: "bar", + Namespace: "baz", Permissions: IntentionPermissions{ { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/bar", + 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", }, }, }, }, - `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`, + `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]`, }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { - err := testCase.input.Validate() + err := testCase.input.ValidateNamespaces(testCase.namespacesEnabled) if testCase.expectedErrMsg != "" { require.EqualError(t, err, testCase.expectedErrMsg) } else { diff --git a/api/v1alpha1/serviceintentions_webhook.go b/api/v1alpha1/serviceintentions_webhook.go index b70ff30544..b342667752 100644 --- a/api/v1alpha1/serviceintentions_webhook.go +++ b/api/v1alpha1/serviceintentions_webhook.go @@ -19,11 +19,13 @@ import ( type ServiceIntentionsWebhook struct { client.Client - ConsulClient *capi.Client - Logger logr.Logger - decoder *admission.Decoder - EnableConsulNamespaces bool - EnableNSMirroring bool + ConsulClient *capi.Client + Logger logr.Logger + decoder *admission.Decoder + EnableConsulNamespaces bool + EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string } // NOTE: The path value in the below line is the path to the webhook. @@ -90,6 +92,12 @@ 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 { + return admission.Errored(http.StatusBadRequest, err) + } + // We always return an admission.Patched() response, even if there are no patches, since // admission.Patched() with no patches is equal to admission.Allowed() under // the hood. @@ -108,7 +116,7 @@ func (v *ServiceIntentionsWebhook) defaultingPatches(err error, svcIntentions *S if err != nil { return nil, fmt.Errorf("marshalling input: %s", err) } - svcIntentions.Default(v.EnableConsulNamespaces) + svcIntentions.Default(v.EnableConsulNamespaces, v.ConsulDestinationNamespace, v.EnableNSMirroring, v.NSMirroringPrefix) afterDefaulting, err := json.Marshal(svcIntentions) if err != nil { return nil, fmt.Errorf("marshalling after defaulting: %s", err) diff --git a/api/v1alpha1/serviceintentions_webhook_test.go b/api/v1alpha1/serviceintentions_webhook_test.go index 6713906aeb..549e309c48 100644 --- a/api/v1alpha1/serviceintentions_webhook_test.go +++ b/api/v1alpha1/serviceintentions_webhook_test.go @@ -410,6 +410,7 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { cases := map[string]struct { newResource *ServiceIntentions expPatches []jsonpatch.Operation + errMsg string }{ "all namespace fields set": { newResource: &ServiceIntentions{ @@ -432,6 +433,7 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { }, }, expPatches: []jsonpatch.Operation{}, + errMsg: `serviceintentions.consul.hashicorp.com "foo-intention" is invalid: [spec.destination.namespace: Invalid value: "bar": consul namespaces must be enabled to set destination.namespace, spec.sources[0].namespace: Invalid value: "baz": consul namespaces must be enabled to set source.namespace]`, }, "destination.namespace empty": { newResource: &ServiceIntentions{ @@ -452,6 +454,7 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { Value: "bar", }, }, + errMsg: "", }, "destination.namespace empty and sources.namespace empty": { newResource: &ServiceIntentions{ @@ -477,12 +480,8 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { Path: "/spec/destination/namespace", Value: "bar", }, - { - Operation: "add", - Path: "/spec/sources/0/namespace", - Value: "bar", - }, }, + errMsg: "", }, "multiple sources.namespace empty": { newResource: &ServiceIntentions{ @@ -507,18 +506,8 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { }, }, }, - expPatches: []jsonpatch.Operation{ - { - Operation: "add", - Path: "/spec/sources/0/namespace", - Value: "bar", - }, - { - Operation: "add", - Path: "/spec/sources/1/namespace", - Value: "bar", - }, - }, + expPatches: []jsonpatch.Operation{}, + errMsg: `serviceintentions.consul.hashicorp.com "foo-intention" is invalid: spec.destination.namespace: Invalid value: "bar": consul namespaces must be enabled to set destination.namespace`, }, } for name, c := range cases { @@ -540,6 +529,7 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { Logger: logrtest.TestLogger{T: t}, decoder: decoder, EnableConsulNamespaces: namespacesEnabled, + EnableNSMirroring: true, } response := validator.Handle(ctx, admission.Request{ AdmissionRequest: v1beta1.AdmissionRequest{ @@ -552,10 +542,14 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { }, }) - require.Equal(t, true, response.Allowed, response.AdmissionResponse.Result.Message) if namespacesEnabled { + require.Equal(t, true, response.Allowed, response.AdmissionResponse.Result.Message) require.ElementsMatch(t, c.expPatches, response.Patches) } else { + if c.errMsg != "" { + require.Equal(t, false, response.Allowed) + require.Equal(t, c.errMsg, response.AdmissionResponse.Result.Message) + } // If namespaces are disabled there should be no patches // because we don't default any namespace fields. require.Len(t, response.Patches, 0) diff --git a/controller/configentry_controller.go b/controller/configentry_controller.go index fed1eda278..b4fe3d7488 100644 --- a/controller/configentry_controller.go +++ b/controller/configentry_controller.go @@ -96,6 +96,8 @@ func (r *ConfigEntryController) ReconcileEntry( return ctrl.Result{}, err } + consulEntry := configEntry.ToConsul(r.DatacenterName) + if configEntry.GetObjectMeta().DeletionTimestamp.IsZero() { // The object is not being deleted, so if it does not have our finalizer, // then let's add the finalizer and update the object. This is equivalent @@ -112,7 +114,7 @@ func (r *ConfigEntryController) ReconcileEntry( logger.Info("deletion event") // Check to see if consul has config entry with the same name entry, _, err := r.ConsulClient.ConfigEntries().Get(configEntry.ConsulKind(), configEntry.ConsulName(), &capi.QueryOptions{ - Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), + Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), }) // Ignore the error where the config entry isn't found in Consul. @@ -123,7 +125,7 @@ func (r *ConfigEntryController) ReconcileEntry( // Only delete the resource from Consul if it is owned by our datacenter. if entry.GetMeta()[common.DatacenterKey] == r.DatacenterName { _, err := r.ConsulClient.ConfigEntries().Delete(configEntry.ConsulKind(), configEntry.ConsulName(), &capi.WriteOptions{ - Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), + Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), }) if err != nil { return ctrl.Result{}, fmt.Errorf("deleting config entry from consul: %w", err) @@ -147,7 +149,7 @@ func (r *ConfigEntryController) ReconcileEntry( // Check to see if consul has config entry with the same name entry, _, err := r.ConsulClient.ConfigEntries().Get(configEntry.ConsulKind(), configEntry.ConsulName(), &capi.QueryOptions{ - Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), + Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), }) // If a config entry with this name does not exist if isNotFoundErr(err) { @@ -156,7 +158,7 @@ func (r *ConfigEntryController) ReconcileEntry( // If Consul namespaces are enabled we may need to create the // destination consul namespace first. if r.EnableConsulNamespaces { - consulNS := r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()) + consulNS := r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()) created, err := namespaces.EnsureExists(r.ConsulClient, consulNS, r.CrossNSACLPolicy) if err != nil { return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError, @@ -168,8 +170,8 @@ func (r *ConfigEntryController) ReconcileEntry( } // Create the config entry - _, writeMeta, err := r.ConsulClient.ConfigEntries().Set(configEntry.ToConsul(r.DatacenterName), &capi.WriteOptions{ - Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), + _, writeMeta, err := r.ConsulClient.ConfigEntries().Set(consulEntry, &capi.WriteOptions{ + Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), }) if err != nil { return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError, @@ -194,8 +196,8 @@ func (r *ConfigEntryController) ReconcileEntry( if !configEntry.MatchesConsul(entry) { logger.Info("config entry does not match consul", "modify-index", entry.GetModifyIndex()) - _, writeMeta, err := r.ConsulClient.ConfigEntries().Set(configEntry.ToConsul(r.DatacenterName), &capi.WriteOptions{ - Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), + _, writeMeta, err := r.ConsulClient.ConfigEntries().Set(consulEntry, &capi.WriteOptions{ + Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()), }) if err != nil { return r.syncUnknownWithError(ctx, logger, crdCtrl, configEntry, ConsulAgentError, @@ -210,7 +212,15 @@ func (r *ConfigEntryController) ReconcileEntry( return ctrl.Result{}, nil } -func (r *ConfigEntryController) consulNamespace(namespace string, globalResource bool) string { +func (r *ConfigEntryController) consulNamespace(configEntry capi.ConfigEntry, namespace string, globalResource bool) string { + // ServiceIntentions have the appropriate Consul Namespace set on them as the value + // is defaulted by the webhook. These are then set on the ServiceIntentions config entry + // but not on the others. In case the ConfigEntry has the Consul Namespace set, we just + // use the namespace assigned instead of attempting to determine it. + if configEntry.GetNamespace() != "" { + return configEntry.GetNamespace() + } + // Does not attempt to parse the namespace for global resources like ProxyDefaults or // wildcard namespace destinations are they will not be prefixed and will remain "default"/"*". if !globalResource && namespace != common.WildcardNamespace { diff --git a/controller/configentry_controller_ent_test.go b/controller/configentry_controller_ent_test.go index be4be6c0aa..b6b0345663 100644 --- a/controller/configentry_controller_ent_test.go +++ b/controller/configentry_controller_ent_test.go @@ -156,7 +156,7 @@ func TestConfigEntryController_createsConfigEntry_consulNamespaces(tt *testing.T Spec: v1alpha1.ServiceIntentionsSpec{ Destination: v1alpha1.Destination{ Name: "test", - Namespace: c.SourceKubeNS, + Namespace: c.ExpConsulNS, }, Sources: v1alpha1.SourceIntentions{ &v1alpha1.SourceIntention{ @@ -404,7 +404,7 @@ func TestConfigEntryController_updatesConfigEntry_consulNamespaces(tt *testing.T Spec: v1alpha1.ServiceIntentionsSpec{ Destination: v1alpha1.Destination{ Name: "foo", - Namespace: c.SourceKubeNS, + Namespace: c.ExpConsulNS, }, Sources: v1alpha1.SourceIntentions{ &v1alpha1.SourceIntention{ @@ -669,7 +669,7 @@ func TestConfigEntryController_deletesConfigEntry_consulNamespaces(tt *testing.T Spec: v1alpha1.ServiceIntentionsSpec{ Destination: v1alpha1.Destination{ Name: "test", - Namespace: c.SourceKubeNS, + Namespace: c.ExpConsulNS, }, Sources: v1alpha1.SourceIntentions{ &v1alpha1.SourceIntention{ diff --git a/subcommand/controller/command.go b/subcommand/controller/command.go index e4744cba2b..94e89eb37b 100644 --- a/subcommand/controller/command.go +++ b/subcommand/controller/command.go @@ -248,11 +248,13 @@ func (c *Command) Run(args []string) int { }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-serviceintentions", &webhook.Admission{Handler: &v1alpha1.ServiceIntentionsWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceIntentions), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceIntentions), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) } // +kubebuilder:scaffold:builder