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 b4e4ba938e..713bb5d546 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), } } @@ -249,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) @@ -297,16 +302,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() { - if in.Spec.Destination.Namespace == "" { - in.Spec.Destination.Namespace = in.Namespace +// 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 = 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 9d8ba32c84..07dc17ab70 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -198,7 +198,8 @@ func TestServiceIntentions_ToConsul(t *testing.T) { }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "svc-name", + Name: "svc-name", + Namespace: "dest-ns", }, Sources: []*SourceIntention{ { @@ -247,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", @@ -457,203 +459,308 @@ func TestServiceIntentions_ObjectMeta(t *testing.T) { require.Equal(t, meta, serviceResolver.GetObjectMeta()) } +// 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 + 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: "", + expectedDestination: "", + }, + "destinationNS": { + enabled: true, + destinationNamespace: "foo", + mirroring: false, + prefix: "", + expectedDestination: "foo", + }, + "mirroringEnabledWithoutPrefix": { + enabled: true, + destinationNamespace: "", + mirroring: true, + prefix: "", + expectedDestination: "bar", + }, + "mirroringWithPrefix": { + enabled: true, + destinationNamespace: "", + mirroring: true, + prefix: "ns-", + expectedDestination: "ns-bar", + }, + } + + for name, s := range namespaceConfig { + input := &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", }, - 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: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", + Namespace: s.expectedDestination, }, }, - output: &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: "foobar", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foobar", + 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", + }, }, }, }, + "", }, - "sources.namespace blank, meta.namespace default": { - input: &ServiceIntentions{ + "no sources": { + &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", + Name: "does-not-matter", }, Spec: ServiceIntentionsSpec{ Destination: Destination{ - Name: "bar", - Namespace: "foo", - }, - Sources: SourceIntentions{ - { - Name: "baz", - Action: "allow", - }, + Name: "dest-service", + Namespace: "namespace", }, + Sources: SourceIntentions{}, }, }, - output: &ServiceIntentions{ + `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{ - 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 { t.Run(name, func(t *testing.T) { - testCase.input.Default() - require.True(t, cmp.Equal(testCase.input, testCase.output)) + 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", @@ -709,7 +816,8 @@ func TestServiceIntentions_Validate(t *testing.T) { }, "", }, - "invalid action": { + "disabled: destination namespace specified": { + false, &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-matter", @@ -717,104 +825,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", @@ -822,31 +989,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 70bc87635a..b342667752 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" @@ -17,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. @@ -41,7 +45,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 +92,39 @@ 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())) + + // 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. + 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, v.ConsulDestinationNamespace, v.EnableNSMirroring, v.NSMirroringPrefix) + 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..33b14665f2 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 { @@ -35,6 +37,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: true, @@ -74,6 +83,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -85,6 +101,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -101,6 +124,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -112,6 +142,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "baz", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: true, @@ -128,6 +165,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "bar", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -139,6 +183,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Name: "foo", Namespace: "baz", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -154,6 +205,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Destination: Destination{ Name: "foo", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }}, newResource: &ServiceIntentions{ @@ -164,6 +222,13 @@ func TestValidateServiceIntentions_Create(t *testing.T) { Destination: Destination{ Name: "foo", }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, }, }, expAllow: false, @@ -209,7 +274,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 +463,168 @@ 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 + errMsg string + }{ + "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{}, + 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{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-intention", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "foo", + }, + Sources: SourceIntentions{ + { + Name: "bar", + Namespace: "foo", + Action: "allow", + }, + }, + }, + }, + expPatches: []jsonpatch.Operation{ + { + Operation: "add", + Path: "/spec/destination/namespace", + Value: "bar", + }, + }, + errMsg: "", + }, + "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", + }, + }, + errMsg: "", + }, + "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{}, + 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 { + 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, + EnableNSMirroring: true, + } + response := validator.Handle(ctx, admission.Request{ + AdmissionRequest: v1beta1.AdmissionRequest{ + Name: c.newResource.KubernetesName(), + Namespace: otherNS, + Operation: v1beta1.Create, + Object: runtime.RawExtension{ + Raw: marshalledRequestObject, + }, + }, + }) + + 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/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 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