From 71fa7e635e4f71d77c4e4cf58126eedaae6e8f77 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Wed, 14 Oct 2020 16:52:48 -0700 Subject: [PATCH 1/4] Support Consul namespaces for service-intentions --- api/v1alpha1/serviceintentions_types.go | 20 ++- api/v1alpha1/serviceintentions_types_test.go | 20 ++- api/v1alpha1/serviceintentions_webhook.go | 32 +++- .../serviceintentions_webhook_test.go | 170 +++++++++++++++++- go.mod | 1 + 5 files changed, 228 insertions(+), 15 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index b4e4ba938e..33a8092c54 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -298,13 +298,19 @@ func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { } // Default sets zero value fields on this object to their defaults. -func (in *ServiceIntentions) Default() { - if in.Spec.Destination.Namespace == "" { - in.Spec.Destination.Namespace = in.Namespace - } - for _, source := range in.Spec.Sources { - if source.Namespace == "" { - source.Namespace = in.Namespace +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 + // namespace fields because this would cause errors + // making API calls (because namespace fields can't be set in OSS). + if consulNamespacesEnabled { + if in.Spec.Destination.Namespace == "" { + in.Spec.Destination.Namespace = in.Namespace + } + for _, source := range in.Spec.Sources { + if source.Namespace == "" { + source.Namespace = in.Namespace + } } } } diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 9d8ba32c84..e4d38975ec 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -457,6 +458,7 @@ func TestServiceIntentions_ObjectMeta(t *testing.T) { require.Equal(t, meta, serviceResolver.GetObjectMeta()) } +// Test defaulting behavior for both OSS and enterprise. func TestServiceIntentions_Default(t *testing.T) { cases := map[string]struct { input *ServiceIntentions @@ -641,10 +643,20 @@ func TestServiceIntentions_Default(t *testing.T) { }, } for name, testCase := range cases { - t.Run(name, func(t *testing.T) { - testCase.input.Default() - require.True(t, cmp.Equal(testCase.input, testCase.output)) - }) + // 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)) + } + }) + } } } diff --git a/api/v1alpha1/serviceintentions_webhook.go b/api/v1alpha1/serviceintentions_webhook.go index 70bc87635a..b70ff30544 100644 --- a/api/v1alpha1/serviceintentions_webhook.go +++ b/api/v1alpha1/serviceintentions_webhook.go @@ -2,12 +2,14 @@ package v1alpha1 import ( "context" + "encoding/json" "errors" "fmt" "net/http" "github.com/go-logr/logr" capi "github.com/hashicorp/consul/api" + "gomodules.xyz/jsonpatch/v2" "k8s.io/api/admission/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -41,7 +43,10 @@ func (v *ServiceIntentionsWebhook) Handle(ctx context.Context, req admission.Req return admission.Errored(http.StatusBadRequest, err) } - svcIntentions.Default() + defaultingPatches, err := v.defaultingPatches(err, &svcIntentions) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } singleConsulDestNS := !(v.EnableConsulNamespaces && v.EnableNSMirroring) if req.Operation == v1beta1.Create { @@ -85,10 +90,33 @@ func (v *ServiceIntentionsWebhook) Handle(ctx context.Context, req admission.Req if err := svcIntentions.Validate(); err != nil { return admission.Errored(http.StatusBadRequest, err) } - return admission.Allowed(fmt.Sprintf("valid %s request", svcIntentions.KubeKind())) + // 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. + return admission.Patched(fmt.Sprintf("valid %s request", svcIntentions.KubeKind()), defaultingPatches...) } func (v *ServiceIntentionsWebhook) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil } + +// defaultingPatches returns the patches needed to set fields to their +// defaults. +func (v *ServiceIntentionsWebhook) defaultingPatches(err error, svcIntentions *ServiceIntentions) ([]jsonpatch.Operation, error) { + beforeDefaulting, err := json.Marshal(svcIntentions) + if err != nil { + return nil, fmt.Errorf("marshalling input: %s", err) + } + svcIntentions.Default(v.EnableConsulNamespaces) + afterDefaulting, err := json.Marshal(svcIntentions) + if err != nil { + return nil, fmt.Errorf("marshalling after defaulting: %s", err) + } + + defaultingPatches, err := jsonpatch.CreatePatch(beforeDefaulting, afterDefaulting) + if err != nil { + return nil, fmt.Errorf("creating patches: %s", err) + } + return defaultingPatches, nil +} diff --git a/api/v1alpha1/serviceintentions_webhook_test.go b/api/v1alpha1/serviceintentions_webhook_test.go index 5eb4e8ba76..6713906aeb 100644 --- a/api/v1alpha1/serviceintentions_webhook_test.go +++ b/api/v1alpha1/serviceintentions_webhook_test.go @@ -3,10 +3,12 @@ package v1alpha1 import ( "context" "encoding/json" + "fmt" "testing" logrtest "github.com/go-logr/logr/testing" "github.com/stretchr/testify/require" + "gomodules.xyz/jsonpatch/v2" "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -14,7 +16,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) -func TestValidateServiceIntentions_Create(t *testing.T) { +func TestHandle_ServiceIntentions_Create(t *testing.T) { otherNS := "other" cases := map[string]struct { @@ -209,7 +211,7 @@ func TestValidateServiceIntentions_Create(t *testing.T) { } } -func TestValidateServiceIntentions_Update(t *testing.T) { +func TestHandle_ServiceIntentions_Update(t *testing.T) { otherNS := "other" cases := map[string]struct { @@ -398,3 +400,167 @@ func TestValidateServiceIntentions_Update(t *testing.T) { }) } } + +// Test that we return patches to set Consul namespace fields to their defaults. +// This test also tests OSS where we expect no patches since OSS has no +// Consul namespaces. +func TestHandle_ServiceIntentions_Patches(t *testing.T) { + otherNS := "other" + + cases := map[string]struct { + newResource *ServiceIntentions + expPatches []jsonpatch.Operation + }{ + "all namespace fields set": { + newResource: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-intention", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "foo", + Namespace: "bar", + }, + Sources: SourceIntentions{ + { + Name: "baz", + Namespace: "baz", + Action: "allow", + }, + }, + }, + }, + expPatches: []jsonpatch.Operation{}, + }, + "destination.namespace empty": { + newResource: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-intention", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "foo", + }, + }, + }, + expPatches: []jsonpatch.Operation{ + { + Operation: "add", + Path: "/spec/destination/namespace", + Value: "bar", + }, + }, + }, + "destination.namespace empty and sources.namespace empty": { + newResource: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-intention", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "foo", + }, + Sources: SourceIntentions{ + { + Name: "baz", + Action: "allow", + }, + }, + }, + }, + expPatches: []jsonpatch.Operation{ + { + Operation: "add", + Path: "/spec/destination/namespace", + Value: "bar", + }, + { + Operation: "add", + Path: "/spec/sources/0/namespace", + Value: "bar", + }, + }, + }, + "multiple sources.namespace empty": { + newResource: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-intention", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "foo", + Namespace: "bar", + }, + Sources: SourceIntentions{ + { + Name: "baz", + Action: "allow", + }, + { + Name: "svc", + Action: "allow", + }, + }, + }, + }, + expPatches: []jsonpatch.Operation{ + { + Operation: "add", + Path: "/spec/sources/0/namespace", + Value: "bar", + }, + { + Operation: "add", + Path: "/spec/sources/1/namespace", + Value: "bar", + }, + }, + }, + } + for name, c := range cases { + for _, namespacesEnabled := range []bool{false, true} { + testName := fmt.Sprintf("%s namespaces-enabled=%t", name, namespacesEnabled) + t.Run(testName, func(t *testing.T) { + ctx := context.Background() + marshalledRequestObject, err := json.Marshal(c.newResource) + require.NoError(t, err) + s := runtime.NewScheme() + s.AddKnownTypes(GroupVersion, &ServiceIntentions{}, &ServiceIntentionsList{}) + client := fake.NewFakeClientWithScheme(s) + decoder, err := admission.NewDecoder(s) + require.NoError(t, err) + + validator := &ServiceIntentionsWebhook{ + Client: client, + ConsulClient: nil, + Logger: logrtest.TestLogger{T: t}, + decoder: decoder, + EnableConsulNamespaces: namespacesEnabled, + } + response := validator.Handle(ctx, admission.Request{ + AdmissionRequest: v1beta1.AdmissionRequest{ + Name: c.newResource.KubernetesName(), + Namespace: otherNS, + Operation: v1beta1.Create, + Object: runtime.RawExtension{ + Raw: marshalledRequestObject, + }, + }, + }) + + require.Equal(t, true, response.Allowed, response.AdmissionResponse.Result.Message) + if namespacesEnabled { + require.ElementsMatch(t, c.expPatches, response.Patches) + } else { + // 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/go.mod b/go.mod index 0d693d4936..e90903a575 100644 --- a/go.mod +++ b/go.mod @@ -36,6 +36,7 @@ require ( golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae // indirect golang.org/x/time v0.0.0-20200416051211-89c76fbcd5d1 // indirect golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect + gomodules.xyz/jsonpatch/v2 v2.0.1 google.golang.org/api v0.9.0 // indirect google.golang.org/appengine v1.6.0 // indirect k8s.io/api v0.18.6 From 275bb235438bee0ef164597a155fa4c77a917714 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Wed, 28 Oct 2020 18:04:58 -0400 Subject: [PATCH 2/4] Create service intentions in the desntination.namspace - Use Defaulting to ensure the correct destination namespace is set on the spec.destination of the config entry. Additionally, explicity set the destination namespace on the service intention config entry. Utilize this namespace to ensure the config entry is managed in the expected Consul namespace. --- api/v1alpha1/serviceintentions_types.go | 45 +- api/v1alpha1/serviceintentions_types_test.go | 516 ++++++++++++------ api/v1alpha1/serviceintentions_webhook.go | 20 +- .../serviceintentions_webhook_test.go | 30 +- controller/configentry_controller.go | 28 +- controller/configentry_controller_ent_test.go | 6 +- subcommand/controller/command.go | 12 +- 7 files changed, 432 insertions(+), 225 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 33a8092c54..ecd267aae7 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" @@ -153,10 +154,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), } } @@ -297,22 +299,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 d45e4dd70e..6298099b5c 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 r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError, @@ -148,7 +150,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) { @@ -157,7 +159,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, @@ -169,8 +171,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, @@ -195,8 +197,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, @@ -211,7 +213,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 From 72ee1c38b2cde17d354d6a137af53816a2b8a374 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Wed, 28 Oct 2020 18:20:26 -0400 Subject: [PATCH 3/4] Validate at least one source in specified for a service intention - Do not fail if empty string value is provided for exposePath.Protocol. Consul defaults it to the correct value. --- api/v1alpha1/servicedefaults_types.go | 2 +- api/v1alpha1/servicedefaults_types_test.go | 11 +++ api/v1alpha1/serviceintentions_types.go | 3 + api/v1alpha1/serviceintentions_types_test.go | 15 ++++ .../serviceintentions_webhook_test.go | 70 +++++++++++++++++++ 5 files changed, 100 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/servicedefaults_types.go b/api/v1alpha1/servicedefaults_types.go index 67a62c2ddb..73d1b1276d 100644 --- a/api/v1alpha1/servicedefaults_types.go +++ b/api/v1alpha1/servicedefaults_types.go @@ -227,7 +227,7 @@ func (e ExposeConfig) validate(path *field.Path) []*field.Error { pathCfg.Path, `must begin with a '/'`)) } - if !sliceContains(protocols, pathCfg.Protocol) { + if pathCfg.Protocol != "" && !sliceContains(protocols, pathCfg.Protocol) { errs = append(errs, field.Invalid( indexPath.Child("protocol"), pathCfg.Protocol, diff --git a/api/v1alpha1/servicedefaults_types_test.go b/api/v1alpha1/servicedefaults_types_test.go index b6fd9eaa1b..c6736bb2e6 100644 --- a/api/v1alpha1/servicedefaults_types_test.go +++ b/api/v1alpha1/servicedefaults_types_test.go @@ -223,6 +223,17 @@ func TestServiceDefaults_Validate(t *testing.T) { MeshGateway: MeshGatewayConfig{ Mode: "remote", }, + Expose: ExposeConfig{ + Checks: false, + Paths: []ExposePath{ + { + ListenerPort: 100, + Path: "/bar", + LocalPathPort: 1000, + Protocol: "", + }, + }, + }, }, }, expectedErrMsg: "", diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index ecd267aae7..713bb5d546 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -251,6 +251,9 @@ func (in *ServiceIntentions) MatchesConsul(candidate api.ConfigEntry) bool { func (in *ServiceIntentions) Validate() error { var errs field.ErrorList path := field.NewPath("spec") + if len(in.Spec.Sources) == 0 { + errs = append(errs, field.Required(path.Child("sources"), `at least one source must be specified`)) + } for i, source := range in.Spec.Sources { if len(source.Permissions) > 0 && source.Action != "" { asJSON, _ := json.Marshal(source) diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 58ed5632d3..60b49d2c40 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -596,6 +596,21 @@ func TestServiceIntentions_Validate(t *testing.T) { }, "", }, + "no sources": { + &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{}, + }, + }, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources: Required value: at least one source must be specified`, + }, "invalid action": { &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ diff --git a/api/v1alpha1/serviceintentions_webhook_test.go b/api/v1alpha1/serviceintentions_webhook_test.go index 549e309c48..33b14665f2 100644 --- a/api/v1alpha1/serviceintentions_webhook_test.go +++ b/api/v1alpha1/serviceintentions_webhook_test.go @@ -37,6 +37,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: true, @@ -76,6 +83,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -87,6 +101,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -103,6 +124,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -114,6 +142,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "baz", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: true, @@ -130,6 +165,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -141,6 +183,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "baz", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -156,6 +205,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Destination: Destination{ Name: "foo", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -166,6 +222,13 @@ func TestHandle_ServiceIntentions_Create(t *testing.T) { Destination: Destination{ Name: "foo", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -445,6 +508,13 @@ func TestHandle_ServiceIntentions_Patches(t *testing.T) { Destination: Destination{ Name: "foo", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expPatches: []jsonpatch.Operation{ From 9ce9fab63eeea9847e640cb8398f8befc95793b4 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Fri, 30 Oct 2020 17:14:29 -0400 Subject: [PATCH 4/4] Remove redundant source namespace field --- api/v1alpha1/serviceintentions_types_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 60b49d2c40..07dc17ab70 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -466,7 +466,6 @@ func TestServiceIntentions_Default(t *testing.T) { destinationNamespace string mirroring bool prefix string - sourceNamespace string expectedDestination string }{ "disabled": { @@ -474,7 +473,6 @@ func TestServiceIntentions_Default(t *testing.T) { destinationNamespace: "", mirroring: false, prefix: "", - sourceNamespace: "bar", expectedDestination: "", }, "destinationNS": { @@ -482,7 +480,6 @@ func TestServiceIntentions_Default(t *testing.T) { destinationNamespace: "foo", mirroring: false, prefix: "", - sourceNamespace: "bar", expectedDestination: "foo", }, "mirroringEnabledWithoutPrefix": { @@ -490,7 +487,6 @@ func TestServiceIntentions_Default(t *testing.T) { destinationNamespace: "", mirroring: true, prefix: "", - sourceNamespace: "bar", expectedDestination: "bar", }, "mirroringWithPrefix": { @@ -498,7 +494,6 @@ func TestServiceIntentions_Default(t *testing.T) { destinationNamespace: "", mirroring: true, prefix: "ns-", - sourceNamespace: "bar", expectedDestination: "ns-bar", }, } @@ -507,7 +502,7 @@ func TestServiceIntentions_Default(t *testing.T) { input := &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", - Namespace: s.sourceNamespace, + Namespace: "bar", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ @@ -518,7 +513,7 @@ func TestServiceIntentions_Default(t *testing.T) { output := &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", - Namespace: s.sourceNamespace, + Namespace: "bar", }, Spec: ServiceIntentionsSpec{ Destination: Destination{