From 1a31cf7d84ec652f3d69e7675759da52866daac5 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Fri, 18 Dec 2020 17:35:00 -0500 Subject: [PATCH 1/5] Default namespaces for CRDs that support namespace when enabled --- api/common/configentry.go | 2 + api/common/configentry_webhook.go | 32 ++++++- api/common/configentry_webhook_test.go | 21 +++-- api/v1alpha1/ingressgateway_types.go | 18 ++++ api/v1alpha1/ingressgateway_types_test.go | 86 ++++++++++++++++++ api/v1alpha1/ingressgateway_webhook.go | 7 +- api/v1alpha1/proxydefaults_types.go | 5 ++ api/v1alpha1/servicedefaults_types.go | 5 ++ api/v1alpha1/servicedefaults_webhook.go | 7 +- api/v1alpha1/serviceresolver_types.go | 23 +++++ api/v1alpha1/serviceresolver_types_test.go | 83 +++++++++++++++++ api/v1alpha1/serviceresolver_webhook.go | 7 +- api/v1alpha1/servicerouter_types.go | 18 ++++ api/v1alpha1/servicerouter_types_test.go | 90 +++++++++++++++++++ api/v1alpha1/servicerouter_webhook.go | 7 +- api/v1alpha1/servicesplitter_types.go | 17 +++- api/v1alpha1/servicesplitter_types_test.go | 83 +++++++++++++++++ api/v1alpha1/servicesplitter_webhook.go | 7 +- api/v1alpha1/terminatinggateway_types.go | 16 ++++ api/v1alpha1/terminatinggateway_types_test.go | 76 ++++++++++++++++ api/v1alpha1/terminatinggateway_webhook.go | 7 +- subcommand/controller/command.go | 72 ++++++++------- 22 files changed, 643 insertions(+), 46 deletions(-) diff --git a/api/common/configentry.go b/api/common/configentry.go index 5aec9ebee0..24f84a1bec 100644 --- a/api/common/configentry.go +++ b/api/common/configentry.go @@ -57,4 +57,6 @@ type ConfigEntryResource interface { DeepCopyObject() runtime.Object // Validate returns an error if the resource is invalid. Validate(namespacesEnabled bool) error + // Default sets the namespace field on the config entry spec to their default values if namespaces are enabled. + Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) } diff --git a/api/common/configentry_webhook.go b/api/common/configentry_webhook.go index 10a9e46044..0d12731897 100644 --- a/api/common/configentry_webhook.go +++ b/api/common/configentry_webhook.go @@ -2,10 +2,12 @@ package common import ( "context" + "encoding/json" "fmt" "net/http" "github.com/go-logr/logr" + "gomodules.xyz/jsonpatch/v2" "k8s.io/api/admission/v1beta1" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -28,8 +30,14 @@ func ValidateConfigEntry( configEntryLister ConfigEntryLister, cfgEntry ConfigEntryResource, enableConsulNamespaces bool, - nsMirroring bool) admission.Response { + nsMirroring bool, + consulDestinationNamespace string, + nsMirroringPrefix string) admission.Response { + defaultingPatches, err := defaultingPatches(cfgEntry, enableConsulNamespaces, nsMirroring, consulDestinationNamespace, nsMirroringPrefix) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } // On create we need to validate that there isn't already a resource with // the same name in a different namespace if we're need to mapping all Kube // resources to a single Consul namespace. The only case where we're not @@ -56,5 +64,25 @@ func ValidateConfigEntry( if err := cfgEntry.Validate(enableConsulNamespaces); err != nil { return admission.Errored(http.StatusBadRequest, err) } - return admission.Allowed(fmt.Sprintf("valid %s request", cfgEntry.KubeKind())) + return admission.Patched(fmt.Sprintf("valid %s request", cfgEntry.KubeKind()), defaultingPatches...) +} + +// defaultingPatches returns the patches needed to set fields to their +// defaults. +func defaultingPatches(svcIntentions ConfigEntryResource, enableConsulNamespaces bool, nsMirroring bool, consulDestinationNamespace string, nsMirroringPrefix string) ([]jsonpatch.Operation, error) { + beforeDefaulting, err := json.Marshal(svcIntentions) + if err != nil { + return nil, fmt.Errorf("marshalling input: %s", err) + } + svcIntentions.Default(enableConsulNamespaces, consulDestinationNamespace, nsMirroring, 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/common/configentry_webhook_test.go b/api/common/configentry_webhook_test.go index a8d7d754fe..85be8c2fb3 100644 --- a/api/common/configentry_webhook_test.go +++ b/api/common/configentry_webhook_test.go @@ -21,12 +21,14 @@ func TestValidateConfigEntry(t *testing.T) { otherNS := "other" cases := map[string]struct { - existingResources []ConfigEntryResource - newResource ConfigEntryResource - enableNamespaces bool - nsMirroring bool - expAllow bool - expErrMessage string + existingResources []ConfigEntryResource + newResource ConfigEntryResource + enableNamespaces bool + nsMirroring bool + consulDestinationNS string + nsMirroringPrefix string + expAllow bool + expErrMessage string }{ "no duplicates, valid": { existingResources: nil, @@ -112,7 +114,9 @@ func TestValidateConfigEntry(t *testing.T) { lister, c.newResource, c.enableNamespaces, - c.nsMirroring) + c.nsMirroring, + c.consulDestinationNS, + c.nsMirroringPrefix) require.Equal(t, c.expAllow, response.Allowed) if c.expErrMessage != "" { require.Equal(t, c.expErrMessage, response.AdmissionResponse.Result.Message) @@ -200,6 +204,9 @@ func (in *mockConfigEntry) Validate(bool) error { return nil } +func (in *mockConfigEntry) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { +} + func (in *mockConfigEntry) MatchesConsul(_ capi.ConfigEntry) bool { return false } diff --git a/api/v1alpha1/ingressgateway_types.go b/api/v1alpha1/ingressgateway_types.go index 39c4110a05..384b3abd90 100644 --- a/api/v1alpha1/ingressgateway_types.go +++ b/api/v1alpha1/ingressgateway_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/namespaces" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -225,6 +226,23 @@ func (in *IngressGateway) Validate(namespacesEnabled bool) error { return nil } +func (in *IngressGateway) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the namespace fields 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) + for i, listener := range in.Spec.Listeners { + for j, service := range listener.Services { + if service.Namespace == "" { + in.Spec.Listeners[i].Services[j].Namespace = namespace + } + } + } + } +} + func (in GatewayTLSConfig) toConsul() capi.GatewayTLSConfig { return capi.GatewayTLSConfig{ Enabled: in.Enabled, diff --git a/api/v1alpha1/ingressgateway_types_test.go b/api/v1alpha1/ingressgateway_types_test.go index 1a5c0247ee..57b93b1ea2 100644 --- a/api/v1alpha1/ingressgateway_types_test.go +++ b/api/v1alpha1/ingressgateway_types_test.go @@ -3,6 +3,7 @@ package v1alpha1 import ( "testing" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -459,6 +460,91 @@ func TestIngressGateway_Validate(t *testing.T) { } } +// Test defaulting behavior when namespaces are enabled as well as disabled. +func TestIngressGateway_Default(t *testing.T) { + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + expectedDestination string + }{ + "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 := &IngressGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: IngressGatewaySpec{ + Listeners: []IngressListener{ + { + Protocol: "tcp", + Services: []IngressService{ + { + Name: "name", + }, + }, + }, + }, + }, + } + output := &IngressGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: IngressGatewaySpec{ + Listeners: []IngressListener{ + { + Protocol: "tcp", + Services: []IngressService{ + { + Name: "name", + Namespace: s.expectedDestination, + }, + }, + }, + }, + }, + } + + 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 TestIngressGateway_AddFinalizer(t *testing.T) { resource := &IngressGateway{} resource.AddFinalizer("finalizer") diff --git a/api/v1alpha1/ingressgateway_webhook.go b/api/v1alpha1/ingressgateway_webhook.go index 447de8a294..791351c1ad 100644 --- a/api/v1alpha1/ingressgateway_webhook.go +++ b/api/v1alpha1/ingressgateway_webhook.go @@ -26,6 +26,9 @@ type IngressGatewayWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -51,7 +54,9 @@ func (v *IngressGatewayWebhook) Handle(ctx context.Context, req admission.Reques v, &resource, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *IngressGatewayWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/api/v1alpha1/proxydefaults_types.go b/api/v1alpha1/proxydefaults_types.go index 02445ddcf2..c8a2fa909e 100644 --- a/api/v1alpha1/proxydefaults_types.go +++ b/api/v1alpha1/proxydefaults_types.go @@ -176,6 +176,11 @@ func (in *ProxyDefaults) Validate(namespacesEnabled bool) error { return nil } +// Default has no behaviour here as proxy-defaults have no namespace specific fields. +func (in *ProxyDefaults) Default(_ bool, _ string, _ bool, _ string) { + return +} + // convertConfig converts the config of type json.RawMessage which is stored // by the resource into type map[string]interface{} which is saved by the // consul API. diff --git a/api/v1alpha1/servicedefaults_types.go b/api/v1alpha1/servicedefaults_types.go index e75f2cfb21..a3f028bd06 100644 --- a/api/v1alpha1/servicedefaults_types.go +++ b/api/v1alpha1/servicedefaults_types.go @@ -185,6 +185,11 @@ func (in *ServiceDefaults) Validate(namespacesEnabled bool) error { return nil } +// Default has no behaviour here as service-defaults have no namespace specific fields. +func (in *ServiceDefaults) Default(_ bool, _ string, _ bool, _ string) { + return +} + // MatchesConsul returns true if entry has the same config as this struct. func (in *ServiceDefaults) MatchesConsul(candidate capi.ConfigEntry) bool { configEntry, ok := candidate.(*capi.ServiceConfigEntry) diff --git a/api/v1alpha1/servicedefaults_webhook.go b/api/v1alpha1/servicedefaults_webhook.go index e45e8e5ad2..26f9f70425 100644 --- a/api/v1alpha1/servicedefaults_webhook.go +++ b/api/v1alpha1/servicedefaults_webhook.go @@ -26,6 +26,9 @@ type ServiceDefaultsWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -51,7 +54,9 @@ func (v *ServiceDefaultsWebhook) Handle(ctx context.Context, req admission.Reque v, &svcDefaults, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *ServiceDefaultsWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/api/v1alpha1/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go index dc1df226d8..36474b26c6 100644 --- a/api/v1alpha1/serviceresolver_types.go +++ b/api/v1alpha1/serviceresolver_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/namespaces" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -306,6 +307,28 @@ func (in *ServiceResolver) Validate(namespacesEnabled bool) error { return nil } +func (in *ServiceResolver) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the namespaces 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.Redirect != nil { + if in.Spec.Redirect.Namespace == "" { + in.Spec.Redirect.Namespace = namespace + } + } + for k, v := range in.Spec.Failover { + if v.Namespace == "" { + failover := in.Spec.Failover[k] + failover.Namespace = namespace + in.Spec.Failover[k] = failover + } + } + } +} + func (in ServiceResolverSubsetMap) toConsul() map[string]capi.ServiceResolverSubset { if in == nil { return nil diff --git a/api/v1alpha1/serviceresolver_types_test.go b/api/v1alpha1/serviceresolver_types_test.go index 73db74515e..2366a1b548 100644 --- a/api/v1alpha1/serviceresolver_types_test.go +++ b/api/v1alpha1/serviceresolver_types_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -653,3 +654,85 @@ func TestServiceResolver_Validate(t *testing.T) { }) } } + +// Test defaulting behavior when namespaces are enabled as well as disabled. +func TestServiceResolver_Default(t *testing.T) { + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + expectedDestination string + }{ + "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 := &ServiceResolver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceResolverSpec{ + Redirect: &ServiceResolverRedirect{ + Service: "bar", + }, + Failover: map[string]ServiceResolverFailover{ + "failA": { + Service: "baz", + }, + }, + }, + } + output := &ServiceResolver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceResolverSpec{ + Redirect: &ServiceResolverRedirect{ + Service: "bar", + Namespace: s.expectedDestination, + }, + Failover: map[string]ServiceResolverFailover{ + "failA": { + Service: "baz", + Namespace: s.expectedDestination, + }, + }, + }, + } + + t.Run(name, func(t *testing.T) { + input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + require.True(t, cmp.Equal(input, output)) + }) + } +} diff --git a/api/v1alpha1/serviceresolver_webhook.go b/api/v1alpha1/serviceresolver_webhook.go index 7b80e6af09..078332b7c7 100644 --- a/api/v1alpha1/serviceresolver_webhook.go +++ b/api/v1alpha1/serviceresolver_webhook.go @@ -26,6 +26,9 @@ type ServiceResolverWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -51,7 +54,9 @@ func (v *ServiceResolverWebhook) Handle(ctx context.Context, req admission.Reque v, &svcResolver, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *ServiceResolverWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/api/v1alpha1/servicerouter_types.go b/api/v1alpha1/servicerouter_types.go index 0571e3aa47..1eee13fe3d 100644 --- a/api/v1alpha1/servicerouter_types.go +++ b/api/v1alpha1/servicerouter_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/namespaces" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -255,6 +256,23 @@ func (in *ServiceRouter) Validate(namespacesEnabled bool) error { return nil } +func (in *ServiceRouter) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the namespace fields 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) + for i, r := range in.Spec.Routes { + if r.Destination != nil { + if r.Destination.Namespace == "" { + in.Spec.Routes[i].Destination.Namespace = namespace + } + } + } + } +} + func (in ServiceRoute) toConsul() capi.ServiceRoute { return capi.ServiceRoute{ Match: in.Match.toConsul(), diff --git a/api/v1alpha1/servicerouter_types_test.go b/api/v1alpha1/servicerouter_types_test.go index be320e7626..4cbb3f6ff9 100644 --- a/api/v1alpha1/servicerouter_types_test.go +++ b/api/v1alpha1/servicerouter_types_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -598,3 +599,92 @@ func TestServiceRouter_Validate(t *testing.T) { }) } } + +// Test defaulting behavior when namespaces are enabled as well as disabled. +func TestServiceRouter_Default(t *testing.T) { + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + expectedDestination string + }{ + "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 := &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destA", + }, + }, + }, + }, + } + output := &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destA", + Namespace: s.expectedDestination, + }, + }, + }, + }, + } + + t.Run(name, func(t *testing.T) { + input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + require.True(t, cmp.Equal(input, output)) + }) + } +} diff --git a/api/v1alpha1/servicerouter_webhook.go b/api/v1alpha1/servicerouter_webhook.go index 4719b216f9..1c04b7afb7 100644 --- a/api/v1alpha1/servicerouter_webhook.go +++ b/api/v1alpha1/servicerouter_webhook.go @@ -26,6 +26,9 @@ type ServiceRouterWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -51,7 +54,9 @@ func (v *ServiceRouterWebhook) Handle(ctx context.Context, req admission.Request v, &svcRouter, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *ServiceRouterWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/api/v1alpha1/servicesplitter_types.go b/api/v1alpha1/servicesplitter_types.go index f6224c25b3..e0f689472a 100644 --- a/api/v1alpha1/servicesplitter_types.go +++ b/api/v1alpha1/servicesplitter_types.go @@ -7,6 +7,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" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -170,6 +171,21 @@ func (in *ServiceSplitter) Validate(namespacesEnabled bool) error { return nil } +func (in *ServiceSplitter) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the namespace fields 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) + for i, s := range in.Spec.Splits { + if s.Namespace == "" { + in.Spec.Splits[i].Namespace = namespace + } + } + } +} + func (in ServiceSplits) toConsul() []capi.ServiceSplit { var consulServiceSplits []capi.ServiceSplit for _, split := range in { @@ -196,7 +212,6 @@ func (in *ServiceSplitter) validateNamespaces(namespacesEnabled bool) field.Erro if s.Namespace != "" { errs = append(errs, field.Invalid(path.Child("splits").Index(i).Child("namespace"), s.Namespace, `Consul Enterprise namespaces must be enabled to set split.namespace`)) } - } } return errs diff --git a/api/v1alpha1/servicesplitter_types_test.go b/api/v1alpha1/servicesplitter_types_test.go index 39c6ec4652..ad6e525819 100644 --- a/api/v1alpha1/servicesplitter_types_test.go +++ b/api/v1alpha1/servicesplitter_types_test.go @@ -3,6 +3,7 @@ package v1alpha1 import ( "testing" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -427,3 +428,85 @@ func TestServiceSplitter_Validate(t *testing.T) { }) } } + +// Test defaulting behavior when namespaces are enabled as well as disabled. +func TestServiceSplitter_Default(t *testing.T) { + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + expectedDestination string + }{ + "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 := &ServiceSplitter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceSplitterSpec{ + Splits: []ServiceSplit{ + { + Weight: 99.99, + }, + { + Weight: 0.01, + }, + }, + }, + } + output := &ServiceSplitter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceSplitterSpec{ + Splits: []ServiceSplit{ + { + Weight: 99.99, + Namespace: s.expectedDestination, + }, + { + Weight: 0.01, + Namespace: s.expectedDestination, + }, + }, + }, + } + + t.Run(name, func(t *testing.T) { + input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + require.True(t, cmp.Equal(input, output)) + }) + } +} diff --git a/api/v1alpha1/servicesplitter_webhook.go b/api/v1alpha1/servicesplitter_webhook.go index 7d31b0510e..533ded9564 100644 --- a/api/v1alpha1/servicesplitter_webhook.go +++ b/api/v1alpha1/servicesplitter_webhook.go @@ -26,6 +26,9 @@ type ServiceSplitterWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -52,7 +55,9 @@ func (v *ServiceSplitterWebhook) Handle(ctx context.Context, req admission.Reque v, &serviceSplitter, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *ServiceSplitterWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/api/v1alpha1/terminatinggateway_types.go b/api/v1alpha1/terminatinggateway_types.go index 0c5bd379a5..712e67a1ef 100644 --- a/api/v1alpha1/terminatinggateway_types.go +++ b/api/v1alpha1/terminatinggateway_types.go @@ -5,6 +5,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/hashicorp/consul-k8s/namespaces" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -188,6 +189,21 @@ func (in *TerminatingGateway) Validate(namespacesEnabled bool) error { return nil } +func (in *TerminatingGateway) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + // If namespaces are enabled we want to set the namespace fields 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) + for i, service := range in.Spec.Services { + if service.Namespace == "" { + in.Spec.Services[i].Namespace = namespace + } + } + } +} + func (in LinkedService) toConsul() capi.LinkedService { return capi.LinkedService{ Namespace: in.Namespace, diff --git a/api/v1alpha1/terminatinggateway_types_test.go b/api/v1alpha1/terminatinggateway_types_test.go index 51edc2521b..6f61a25597 100644 --- a/api/v1alpha1/terminatinggateway_types_test.go +++ b/api/v1alpha1/terminatinggateway_types_test.go @@ -3,6 +3,7 @@ package v1alpha1 import ( "testing" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -279,6 +280,81 @@ func TestTerminatingGateway_Validate(t *testing.T) { } } +// Test defaulting behavior when namespaces are enabled as well as disabled. +func TestTerminatingGateway_Default(t *testing.T) { + namespaceConfig := map[string]struct { + enabled bool + destinationNamespace string + mirroring bool + prefix string + expectedDestination string + }{ + "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 := &TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: TerminatingGatewaySpec{ + Services: []LinkedService{ + { + Name: "foo", + }, + }, + }, + } + output := &TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: TerminatingGatewaySpec{ + Services: []LinkedService{ + { + Name: "foo", + Namespace: s.expectedDestination, + }, + }, + }, + } + + 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 TestTerminatingGateway_AddFinalizer(t *testing.T) { resource := &TerminatingGateway{} resource.AddFinalizer("finalizer") diff --git a/api/v1alpha1/terminatinggateway_webhook.go b/api/v1alpha1/terminatinggateway_webhook.go index 59b3d91e5e..6d1cd35846 100644 --- a/api/v1alpha1/terminatinggateway_webhook.go +++ b/api/v1alpha1/terminatinggateway_webhook.go @@ -26,6 +26,9 @@ type TerminatingGatewayWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + ConsulDestinationNamespace string + NSMirroringPrefix string + decoder *admission.Decoder client.Client } @@ -51,7 +54,9 @@ func (v *TerminatingGatewayWebhook) Handle(ctx context.Context, req admission.Re v, &resource, v.EnableConsulNamespaces, - v.EnableNSMirroring) + v.EnableNSMirroring, + v.ConsulDestinationNamespace, + v.NSMirroringPrefix) } func (v *TerminatingGatewayWebhook) List(ctx context.Context) ([]common.ConfigEntryResource, error) { diff --git a/subcommand/controller/command.go b/subcommand/controller/command.go index de0aa5ab61..5dc859e166 100644 --- a/subcommand/controller/command.go +++ b/subcommand/controller/command.go @@ -226,19 +226,23 @@ func (c *Command) Run(args []string) int { // annotation in each webhook file. mgr.GetWebhookServer().Register("/mutate-v1alpha1-servicedefaults", &webhook.Admission{Handler: &v1alpha1.ServiceDefaultsWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceDefaults), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceDefaults), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-serviceresolver", &webhook.Admission{Handler: &v1alpha1.ServiceResolverWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceResolver), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceResolver), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-proxydefaults", &webhook.Admission{Handler: &v1alpha1.ProxyDefaultsWebhook{ @@ -250,19 +254,23 @@ func (c *Command) Run(args []string) int { }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-servicerouter", &webhook.Admission{Handler: &v1alpha1.ServiceRouterWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceRouter), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceRouter), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-servicesplitter", &webhook.Admission{Handler: &v1alpha1.ServiceSplitterWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceSplitter), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.ServiceSplitter), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-serviceintentions", &webhook.Admission{Handler: &v1alpha1.ServiceIntentionsWebhook{ @@ -276,19 +284,23 @@ func (c *Command) Run(args []string) int { }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-ingressgateway", &webhook.Admission{Handler: &v1alpha1.IngressGatewayWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.IngressGateway), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.IngressGateway), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) mgr.GetWebhookServer().Register("/mutate-v1alpha1-terminatinggateway", &webhook.Admission{Handler: &v1alpha1.TerminatingGatewayWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.TerminatingGateway), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, + Client: mgr.GetClient(), + ConsulClient: consulClient, + Logger: ctrl.Log.WithName("webhooks").WithName(common.TerminatingGateway), + EnableConsulNamespaces: c.flagEnableNamespaces, + EnableNSMirroring: c.flagEnableNSMirroring, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + NSMirroringPrefix: c.flagNSMirroringPrefix, }}) } // +kubebuilder:scaffold:builder From 97dd62574b1d48016226d351cdd43c5665393c5e Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Tue, 12 Jan 2021 12:17:34 -0500 Subject: [PATCH 2/5] Only default config entries that perdform defaulting on namespaces --- api/common/configentry.go | 2 +- api/common/configentry_webhook.go | 8 +- api/common/configentry_webhook_test.go | 2 +- api/v1alpha1/ingressgateway_types.go | 2 +- api/v1alpha1/ingressgateway_types_test.go | 2 +- api/v1alpha1/proxydefaults_types.go | 2 +- api/v1alpha1/servicedefaults_types.go | 2 +- api/v1alpha1/serviceintentions_types.go | 2 +- api/v1alpha1/serviceintentions_types_test.go | 2 +- api/v1alpha1/serviceintentions_webhook.go | 2 +- api/v1alpha1/serviceresolver_types.go | 22 +---- api/v1alpha1/serviceresolver_types_test.go | 83 ------------------- api/v1alpha1/servicerouter_types.go | 2 +- api/v1alpha1/servicerouter_types_test.go | 2 +- api/v1alpha1/servicesplitter_types.go | 15 +--- api/v1alpha1/servicesplitter_types_test.go | 83 ------------------- api/v1alpha1/terminatinggateway_types.go | 2 +- api/v1alpha1/terminatinggateway_types_test.go | 2 +- 18 files changed, 19 insertions(+), 218 deletions(-) diff --git a/api/common/configentry.go b/api/common/configentry.go index 24f84a1bec..d2ca64f24b 100644 --- a/api/common/configentry.go +++ b/api/common/configentry.go @@ -58,5 +58,5 @@ type ConfigEntryResource interface { // Validate returns an error if the resource is invalid. Validate(namespacesEnabled bool) error // Default sets the namespace field on the config entry spec to their default values if namespaces are enabled. - Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) + DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) } diff --git a/api/common/configentry_webhook.go b/api/common/configentry_webhook.go index 0d12731897..85e25d64f2 100644 --- a/api/common/configentry_webhook.go +++ b/api/common/configentry_webhook.go @@ -69,13 +69,13 @@ func ValidateConfigEntry( // defaultingPatches returns the patches needed to set fields to their // defaults. -func defaultingPatches(svcIntentions ConfigEntryResource, enableConsulNamespaces bool, nsMirroring bool, consulDestinationNamespace string, nsMirroringPrefix string) ([]jsonpatch.Operation, error) { - beforeDefaulting, err := json.Marshal(svcIntentions) +func defaultingPatches(cfgEntry ConfigEntryResource, enableConsulNamespaces bool, nsMirroring bool, consulDestinationNamespace string, nsMirroringPrefix string) ([]jsonpatch.Operation, error) { + beforeDefaulting, err := json.Marshal(cfgEntry) if err != nil { return nil, fmt.Errorf("marshalling input: %s", err) } - svcIntentions.Default(enableConsulNamespaces, consulDestinationNamespace, nsMirroring, nsMirroringPrefix) - afterDefaulting, err := json.Marshal(svcIntentions) + cfgEntry.DefaultNamespaceFields(enableConsulNamespaces, consulDestinationNamespace, nsMirroring, nsMirroringPrefix) + afterDefaulting, err := json.Marshal(cfgEntry) if err != nil { return nil, fmt.Errorf("marshalling after defaulting: %s", err) } diff --git a/api/common/configentry_webhook_test.go b/api/common/configentry_webhook_test.go index 85be8c2fb3..4b5ae29454 100644 --- a/api/common/configentry_webhook_test.go +++ b/api/common/configentry_webhook_test.go @@ -204,7 +204,7 @@ func (in *mockConfigEntry) Validate(bool) error { return nil } -func (in *mockConfigEntry) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { +func (in *mockConfigEntry) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { } func (in *mockConfigEntry) MatchesConsul(_ capi.ConfigEntry) bool { diff --git a/api/v1alpha1/ingressgateway_types.go b/api/v1alpha1/ingressgateway_types.go index 384b3abd90..680001cbc1 100644 --- a/api/v1alpha1/ingressgateway_types.go +++ b/api/v1alpha1/ingressgateway_types.go @@ -226,7 +226,7 @@ func (in *IngressGateway) Validate(namespacesEnabled bool) error { return nil } -func (in *IngressGateway) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { +func (in *IngressGateway) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { // If namespaces are enabled we want to set the namespace fields to it's // default. If namespaces are not enabled (i.e. OSS) we don't set the // namespace fields because this would cause errors diff --git a/api/v1alpha1/ingressgateway_types_test.go b/api/v1alpha1/ingressgateway_types_test.go index 57b93b1ea2..42155e6ed9 100644 --- a/api/v1alpha1/ingressgateway_types_test.go +++ b/api/v1alpha1/ingressgateway_types_test.go @@ -539,7 +539,7 @@ func TestIngressGateway_Default(t *testing.T) { } t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) } diff --git a/api/v1alpha1/proxydefaults_types.go b/api/v1alpha1/proxydefaults_types.go index c8a2fa909e..916a74a555 100644 --- a/api/v1alpha1/proxydefaults_types.go +++ b/api/v1alpha1/proxydefaults_types.go @@ -177,7 +177,7 @@ func (in *ProxyDefaults) Validate(namespacesEnabled bool) error { } // Default has no behaviour here as proxy-defaults have no namespace specific fields. -func (in *ProxyDefaults) Default(_ bool, _ string, _ bool, _ string) { +func (in *ProxyDefaults) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { return } diff --git a/api/v1alpha1/servicedefaults_types.go b/api/v1alpha1/servicedefaults_types.go index a3f028bd06..8b401559fe 100644 --- a/api/v1alpha1/servicedefaults_types.go +++ b/api/v1alpha1/servicedefaults_types.go @@ -186,7 +186,7 @@ func (in *ServiceDefaults) Validate(namespacesEnabled bool) error { } // Default has no behaviour here as service-defaults have no namespace specific fields. -func (in *ServiceDefaults) Default(_ bool, _ string, _ bool, _ string) { +func (in *ServiceDefaults) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { return } diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 084f9616fd..84e6c640e9 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -272,7 +272,7 @@ func (in *ServiceIntentions) Validate(namespacesEnabled bool) error { } // 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) { +func (in *ServiceIntentions) DefaultNamespaceFields(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 diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index fc563a9c38..51c5013fad 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -567,7 +567,7 @@ func TestServiceIntentions_Default(t *testing.T) { } t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) } diff --git a/api/v1alpha1/serviceintentions_webhook.go b/api/v1alpha1/serviceintentions_webhook.go index e46797e715..52587e3007 100644 --- a/api/v1alpha1/serviceintentions_webhook.go +++ b/api/v1alpha1/serviceintentions_webhook.go @@ -112,7 +112,7 @@ func (v *ServiceIntentionsWebhook) defaultingPatches(err error, svcIntentions *S if err != nil { return nil, fmt.Errorf("marshalling input: %s", err) } - svcIntentions.Default(v.EnableConsulNamespaces, v.ConsulDestinationNamespace, v.EnableNSMirroring, v.NSMirroringPrefix) + svcIntentions.DefaultNamespaceFields(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/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go index 36474b26c6..113de42102 100644 --- a/api/v1alpha1/serviceresolver_types.go +++ b/api/v1alpha1/serviceresolver_types.go @@ -6,7 +6,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/hashicorp/consul-k8s/namespaces" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -307,26 +306,7 @@ func (in *ServiceResolver) Validate(namespacesEnabled bool) error { return nil } -func (in *ServiceResolver) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { - // If namespaces are enabled we want to set the namespaces 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.Redirect != nil { - if in.Spec.Redirect.Namespace == "" { - in.Spec.Redirect.Namespace = namespace - } - } - for k, v := range in.Spec.Failover { - if v.Namespace == "" { - failover := in.Spec.Failover[k] - failover.Namespace = namespace - in.Spec.Failover[k] = failover - } - } - } +func (in *ServiceResolver) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { } func (in ServiceResolverSubsetMap) toConsul() map[string]capi.ServiceResolverSubset { diff --git a/api/v1alpha1/serviceresolver_types_test.go b/api/v1alpha1/serviceresolver_types_test.go index 2366a1b548..73db74515e 100644 --- a/api/v1alpha1/serviceresolver_types_test.go +++ b/api/v1alpha1/serviceresolver_types_test.go @@ -4,7 +4,6 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -654,85 +653,3 @@ func TestServiceResolver_Validate(t *testing.T) { }) } } - -// Test defaulting behavior when namespaces are enabled as well as disabled. -func TestServiceResolver_Default(t *testing.T) { - namespaceConfig := map[string]struct { - enabled bool - destinationNamespace string - mirroring bool - prefix string - expectedDestination string - }{ - "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 := &ServiceResolver{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceResolverSpec{ - Redirect: &ServiceResolverRedirect{ - Service: "bar", - }, - Failover: map[string]ServiceResolverFailover{ - "failA": { - Service: "baz", - }, - }, - }, - } - output := &ServiceResolver{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceResolverSpec{ - Redirect: &ServiceResolverRedirect{ - Service: "bar", - Namespace: s.expectedDestination, - }, - Failover: map[string]ServiceResolverFailover{ - "failA": { - Service: "baz", - Namespace: s.expectedDestination, - }, - }, - }, - } - - t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) - require.True(t, cmp.Equal(input, output)) - }) - } -} diff --git a/api/v1alpha1/servicerouter_types.go b/api/v1alpha1/servicerouter_types.go index 1eee13fe3d..567f0306cf 100644 --- a/api/v1alpha1/servicerouter_types.go +++ b/api/v1alpha1/servicerouter_types.go @@ -256,7 +256,7 @@ func (in *ServiceRouter) Validate(namespacesEnabled bool) error { return nil } -func (in *ServiceRouter) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { +func (in *ServiceRouter) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { // If namespaces are enabled we want to set the namespace fields to it's // default. If namespaces are not enabled (i.e. OSS) we don't set the // namespace fields because this would cause errors diff --git a/api/v1alpha1/servicerouter_types_test.go b/api/v1alpha1/servicerouter_types_test.go index 4cbb3f6ff9..871849b755 100644 --- a/api/v1alpha1/servicerouter_types_test.go +++ b/api/v1alpha1/servicerouter_types_test.go @@ -683,7 +683,7 @@ func TestServiceRouter_Default(t *testing.T) { } t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) } diff --git a/api/v1alpha1/servicesplitter_types.go b/api/v1alpha1/servicesplitter_types.go index e0f689472a..cab3a68b9a 100644 --- a/api/v1alpha1/servicesplitter_types.go +++ b/api/v1alpha1/servicesplitter_types.go @@ -7,7 +7,6 @@ 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" capi "github.com/hashicorp/consul/api" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -171,19 +170,7 @@ func (in *ServiceSplitter) Validate(namespacesEnabled bool) error { return nil } -func (in *ServiceSplitter) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { - // If namespaces are enabled we want to set the namespace fields 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) - for i, s := range in.Spec.Splits { - if s.Namespace == "" { - in.Spec.Splits[i].Namespace = namespace - } - } - } +func (in *ServiceSplitter) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { } func (in ServiceSplits) toConsul() []capi.ServiceSplit { diff --git a/api/v1alpha1/servicesplitter_types_test.go b/api/v1alpha1/servicesplitter_types_test.go index ad6e525819..39c6ec4652 100644 --- a/api/v1alpha1/servicesplitter_types_test.go +++ b/api/v1alpha1/servicesplitter_types_test.go @@ -3,7 +3,6 @@ package v1alpha1 import ( "testing" - "github.com/google/go-cmp/cmp" "github.com/hashicorp/consul-k8s/api/common" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" @@ -428,85 +427,3 @@ func TestServiceSplitter_Validate(t *testing.T) { }) } } - -// Test defaulting behavior when namespaces are enabled as well as disabled. -func TestServiceSplitter_Default(t *testing.T) { - namespaceConfig := map[string]struct { - enabled bool - destinationNamespace string - mirroring bool - prefix string - expectedDestination string - }{ - "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 := &ServiceSplitter{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceSplitterSpec{ - Splits: []ServiceSplit{ - { - Weight: 99.99, - }, - { - Weight: 0.01, - }, - }, - }, - } - output := &ServiceSplitter{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceSplitterSpec{ - Splits: []ServiceSplit{ - { - Weight: 99.99, - Namespace: s.expectedDestination, - }, - { - Weight: 0.01, - Namespace: s.expectedDestination, - }, - }, - }, - } - - t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) - require.True(t, cmp.Equal(input, output)) - }) - } -} diff --git a/api/v1alpha1/terminatinggateway_types.go b/api/v1alpha1/terminatinggateway_types.go index 712e67a1ef..0478b22384 100644 --- a/api/v1alpha1/terminatinggateway_types.go +++ b/api/v1alpha1/terminatinggateway_types.go @@ -189,7 +189,7 @@ func (in *TerminatingGateway) Validate(namespacesEnabled bool) error { return nil } -func (in *TerminatingGateway) Default(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { +func (in *TerminatingGateway) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { // If namespaces are enabled we want to set the namespace fields to it's // default. If namespaces are not enabled (i.e. OSS) we don't set the // namespace fields because this would cause errors diff --git a/api/v1alpha1/terminatinggateway_types_test.go b/api/v1alpha1/terminatinggateway_types_test.go index 6f61a25597..3dbcb877e0 100644 --- a/api/v1alpha1/terminatinggateway_types_test.go +++ b/api/v1alpha1/terminatinggateway_types_test.go @@ -349,7 +349,7 @@ func TestTerminatingGateway_Default(t *testing.T) { } t.Run(name, func(t *testing.T) { - input.Default(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) + input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) } From cf2a61814ceb330f9a6e73a41e7cbe3c7a04cf35 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Tue, 12 Jan 2021 16:07:32 -0500 Subject: [PATCH 3/5] Update CHANGELOG --- CHANGELOG.md | 3 ++- api/v1alpha1/ingressgateway_webhook.go | 11 ++++++++++- api/v1alpha1/servicedefaults_webhook.go | 11 ++++++++++- api/v1alpha1/serviceresolver_webhook.go | 11 ++++++++++- api/v1alpha1/servicerouter_webhook.go | 11 ++++++++++- api/v1alpha1/servicesplitter_webhook.go | 11 ++++++++++- api/v1alpha1/terminatinggateway_webhook.go | 11 ++++++++++- 7 files changed, 62 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d93944286..15db374be9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,8 @@ BUG FIXES: * CRDs: Fix issue where a `ServiceIntentions` resource could be continually resynced with Consul because Consul's internal representation had a different order for an array than the Kubernetes resource. [[GH-416](https://github.com/hashicorp/consul-k8s/pull/416)] - +* CRDs: default the `namespace` fields on resources where Consul performs namespace defaulting to prevent constant re-syncing. + [[GH-413](https://github.com/hashicorp/consul-k8s/pull/413)] ## 0.22.0 (December 21, 2020) BUG FIXES: diff --git a/api/v1alpha1/ingressgateway_webhook.go b/api/v1alpha1/ingressgateway_webhook.go index 791351c1ad..b78852c230 100644 --- a/api/v1alpha1/ingressgateway_webhook.go +++ b/api/v1alpha1/ingressgateway_webhook.go @@ -26,8 +26,17 @@ type IngressGatewayWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client diff --git a/api/v1alpha1/servicedefaults_webhook.go b/api/v1alpha1/servicedefaults_webhook.go index 26f9f70425..5b5f600eba 100644 --- a/api/v1alpha1/servicedefaults_webhook.go +++ b/api/v1alpha1/servicedefaults_webhook.go @@ -26,8 +26,17 @@ type ServiceDefaultsWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client diff --git a/api/v1alpha1/serviceresolver_webhook.go b/api/v1alpha1/serviceresolver_webhook.go index 078332b7c7..806b777646 100644 --- a/api/v1alpha1/serviceresolver_webhook.go +++ b/api/v1alpha1/serviceresolver_webhook.go @@ -26,8 +26,17 @@ type ServiceResolverWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client diff --git a/api/v1alpha1/servicerouter_webhook.go b/api/v1alpha1/servicerouter_webhook.go index 1c04b7afb7..3c7ffb83a5 100644 --- a/api/v1alpha1/servicerouter_webhook.go +++ b/api/v1alpha1/servicerouter_webhook.go @@ -26,8 +26,17 @@ type ServiceRouterWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client diff --git a/api/v1alpha1/servicesplitter_webhook.go b/api/v1alpha1/servicesplitter_webhook.go index 533ded9564..bcda641bd7 100644 --- a/api/v1alpha1/servicesplitter_webhook.go +++ b/api/v1alpha1/servicesplitter_webhook.go @@ -26,8 +26,17 @@ type ServiceSplitterWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client diff --git a/api/v1alpha1/terminatinggateway_webhook.go b/api/v1alpha1/terminatinggateway_webhook.go index 6d1cd35846..efde0a6f44 100644 --- a/api/v1alpha1/terminatinggateway_webhook.go +++ b/api/v1alpha1/terminatinggateway_webhook.go @@ -26,8 +26,17 @@ type TerminatingGatewayWebhook struct { // be created in the matching Consul namespace. EnableNSMirroring bool + // ConsulDestinationNamespace is the namespace in Consul that the config entry created + // in k8s will get mapped into. If the Consul namespace does not already exist, it will + // be created. ConsulDestinationNamespace string - NSMirroringPrefix string + + // NSMirroringPrefix works in conjunction with Namespace Mirroring. + // It is the prefix added to the Consul namespace to map to a specific. + // k8s namespace. For example, if `mirroringK8SPrefix` is set to "k8s-", a + // service in the k8s `staging` namespace will be registered into the + // `k8s-staging` Consul namespace. + NSMirroringPrefix string decoder *admission.Decoder client.Client From 7acec7abe9b6845050ec24f408d9d40dc4c00281 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Wed, 13 Jan 2021 09:54:22 -0500 Subject: [PATCH 4/5] Update field descriptions and code review suggestions --- CHANGELOG.md | 3 +- api/common/configentry.go | 3 +- api/common/configentry_webhook.go | 6 +- api/v1alpha1/ingressgateway_types.go | 5 +- api/v1alpha1/ingressgateway_types_test.go | 63 ++++++++-------- api/v1alpha1/proxydefaults_types.go | 2 +- api/v1alpha1/servicedefaults_types.go | 2 +- api/v1alpha1/serviceintentions_types.go | 2 +- api/v1alpha1/serviceintentions_types_test.go | 47 ++++++------ api/v1alpha1/serviceintentions_webhook.go | 25 +------ api/v1alpha1/serviceresolver_types.go | 2 + api/v1alpha1/servicerouter_types.go | 5 +- api/v1alpha1/servicerouter_types_test.go | 71 +++++++++---------- api/v1alpha1/servicesplitter_types.go | 2 + api/v1alpha1/terminatinggateway_types.go | 5 +- api/v1alpha1/terminatinggateway_types_test.go | 51 +++++++------ 16 files changed, 139 insertions(+), 155 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15db374be9..66f20fb3c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,9 @@ BUG FIXES: * CRDs: Fix issue where a `ServiceIntentions` resource could be continually resynced with Consul because Consul's internal representation had a different order for an array than the Kubernetes resource. [[GH-416](https://github.com/hashicorp/consul-k8s/pull/416)] -* CRDs: default the `namespace` fields on resources where Consul performs namespace defaulting to prevent constant re-syncing. +* CRDs: **(Consul Enterprise only)** default the `namespace` fields on resources where Consul performs namespace defaulting to prevent constant re-syncing. [[GH-413](https://github.com/hashicorp/consul-k8s/pull/413)] + ## 0.22.0 (December 21, 2020) BUG FIXES: diff --git a/api/common/configentry.go b/api/common/configentry.go index d2ca64f24b..063361aecb 100644 --- a/api/common/configentry.go +++ b/api/common/configentry.go @@ -57,6 +57,7 @@ type ConfigEntryResource interface { DeepCopyObject() runtime.Object // Validate returns an error if the resource is invalid. Validate(namespacesEnabled bool) error - // Default sets the namespace field on the config entry spec to their default values if namespaces are enabled. + // DefaultNamespaceFields sets Consul namespace fields on the config entry + // spec to their default values if namespaces are enabled. DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) } diff --git a/api/common/configentry_webhook.go b/api/common/configentry_webhook.go index 85e25d64f2..034665b66a 100644 --- a/api/common/configentry_webhook.go +++ b/api/common/configentry_webhook.go @@ -34,7 +34,7 @@ func ValidateConfigEntry( consulDestinationNamespace string, nsMirroringPrefix string) admission.Response { - defaultingPatches, err := defaultingPatches(cfgEntry, enableConsulNamespaces, nsMirroring, consulDestinationNamespace, nsMirroringPrefix) + defaultingPatches, err := DefaultingPatches(cfgEntry, enableConsulNamespaces, nsMirroring, consulDestinationNamespace, nsMirroringPrefix) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -67,9 +67,9 @@ func ValidateConfigEntry( return admission.Patched(fmt.Sprintf("valid %s request", cfgEntry.KubeKind()), defaultingPatches...) } -// defaultingPatches returns the patches needed to set fields to their +// DefaultingPatches returns the patches needed to set fields to their // defaults. -func defaultingPatches(cfgEntry ConfigEntryResource, enableConsulNamespaces bool, nsMirroring bool, consulDestinationNamespace string, nsMirroringPrefix string) ([]jsonpatch.Operation, error) { +func DefaultingPatches(cfgEntry ConfigEntryResource, enableConsulNamespaces bool, nsMirroring bool, consulDestinationNamespace string, nsMirroringPrefix string) ([]jsonpatch.Operation, error) { beforeDefaulting, err := json.Marshal(cfgEntry) if err != nil { return nil, fmt.Errorf("marshalling input: %s", err) diff --git a/api/v1alpha1/ingressgateway_types.go b/api/v1alpha1/ingressgateway_types.go index 680001cbc1..e96f876fc3 100644 --- a/api/v1alpha1/ingressgateway_types.go +++ b/api/v1alpha1/ingressgateway_types.go @@ -226,9 +226,10 @@ func (in *IngressGateway) Validate(namespacesEnabled bool) error { return nil } +// DefaultNamespaceFields sets the namespace field on spec.listeners[].services to their default values if namespaces are enabled. func (in *IngressGateway) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { - // If namespaces are enabled we want to set the namespace fields to it's - // default. If namespaces are not enabled (i.e. OSS) we don't set the + // If namespaces are enabled we want to set the namespace fields to their + // defaults. 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 { diff --git a/api/v1alpha1/ingressgateway_types_test.go b/api/v1alpha1/ingressgateway_types_test.go index 42155e6ed9..76ee463d3b 100644 --- a/api/v1alpha1/ingressgateway_types_test.go +++ b/api/v1alpha1/ingressgateway_types_test.go @@ -461,7 +461,7 @@ func TestIngressGateway_Validate(t *testing.T) { } // Test defaulting behavior when namespaces are enabled as well as disabled. -func TestIngressGateway_Default(t *testing.T) { +func TestIngressGateway_DefaultNamespaceFields(t *testing.T) { namespaceConfig := map[string]struct { enabled bool destinationNamespace string @@ -500,45 +500,44 @@ func TestIngressGateway_Default(t *testing.T) { } for name, s := range namespaceConfig { - input := &IngressGateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: IngressGatewaySpec{ - Listeners: []IngressListener{ - { - Protocol: "tcp", - Services: []IngressService{ - { - Name: "name", + t.Run(name, func(t *testing.T) { + input := &IngressGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: IngressGatewaySpec{ + Listeners: []IngressListener{ + { + Protocol: "tcp", + Services: []IngressService{ + { + Name: "name", + }, }, }, }, }, - }, - } - output := &IngressGateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: IngressGatewaySpec{ - Listeners: []IngressListener{ - { - Protocol: "tcp", - Services: []IngressService{ - { - Name: "name", - Namespace: s.expectedDestination, + } + output := &IngressGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: IngressGatewaySpec{ + Listeners: []IngressListener{ + { + Protocol: "tcp", + Services: []IngressService{ + { + Name: "name", + Namespace: s.expectedDestination, + }, }, }, }, }, - }, - } - - t.Run(name, func(t *testing.T) { + } input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) diff --git a/api/v1alpha1/proxydefaults_types.go b/api/v1alpha1/proxydefaults_types.go index 916a74a555..07c6470a61 100644 --- a/api/v1alpha1/proxydefaults_types.go +++ b/api/v1alpha1/proxydefaults_types.go @@ -176,7 +176,7 @@ func (in *ProxyDefaults) Validate(namespacesEnabled bool) error { return nil } -// Default has no behaviour here as proxy-defaults have no namespace specific fields. +// DefaultNamespaceFields has no behaviour here as proxy-defaults have no namespace specific fields. func (in *ProxyDefaults) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { return } diff --git a/api/v1alpha1/servicedefaults_types.go b/api/v1alpha1/servicedefaults_types.go index 8b401559fe..f60445adce 100644 --- a/api/v1alpha1/servicedefaults_types.go +++ b/api/v1alpha1/servicedefaults_types.go @@ -185,7 +185,7 @@ func (in *ServiceDefaults) Validate(namespacesEnabled bool) error { return nil } -// Default has no behaviour here as service-defaults have no namespace specific fields. +// DefaultNamespaceFields has no behaviour here as service-defaults have no namespace specific fields. func (in *ServiceDefaults) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { return } diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 84e6c640e9..5253ec0368 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -271,7 +271,7 @@ func (in *ServiceIntentions) Validate(namespacesEnabled bool) error { return nil } -// Default sets the namespace field on spec.destination to their default values if namespaces are enabled. +// DefaultNamespaceFields sets the namespace field on spec.destination to their default values if namespaces are enabled. func (in *ServiceIntentions) DefaultNamespaceFields(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 diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 51c5013fad..baf8ba6837 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -503,7 +503,7 @@ func TestServiceIntentions_ObjectMeta(t *testing.T) { } // Test defaulting behavior when namespaces are enabled as well as disabled. -func TestServiceIntentions_Default(t *testing.T) { +func TestServiceIntentions_DefaultNamespaceFields(t *testing.T) { namespaceConfig := map[string]struct { enabled bool destinationNamespace string @@ -542,31 +542,30 @@ func TestServiceIntentions_Default(t *testing.T) { } for name, s := range namespaceConfig { - input := &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", + t.Run(name, func(t *testing.T) { + input := &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", }, - }, - } - output := &ServiceIntentions{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceIntentionsSpec{ - Destination: Destination{ - Name: "bar", - Namespace: s.expectedDestination, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", + }, }, - }, - } - - t.Run(name, func(t *testing.T) { + } + output := &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", + Namespace: s.expectedDestination, + }, + }, + } input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) diff --git a/api/v1alpha1/serviceintentions_webhook.go b/api/v1alpha1/serviceintentions_webhook.go index 52587e3007..8dfeb9d8d4 100644 --- a/api/v1alpha1/serviceintentions_webhook.go +++ b/api/v1alpha1/serviceintentions_webhook.go @@ -2,14 +2,13 @@ package v1alpha1 import ( "context" - "encoding/json" "errors" "fmt" "net/http" "github.com/go-logr/logr" + "github.com/hashicorp/consul-k8s/api/common" 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" @@ -45,7 +44,7 @@ func (v *ServiceIntentionsWebhook) Handle(ctx context.Context, req admission.Req return admission.Errored(http.StatusBadRequest, err) } - defaultingPatches, err := v.defaultingPatches(err, &svcIntentions) + defaultingPatches, err := common.DefaultingPatches(&svcIntentions, v.EnableConsulNamespaces, v.EnableNSMirroring, v.ConsulDestinationNamespace, v.NSMirroringPrefix) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -104,23 +103,3 @@ 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.DefaultNamespaceFields(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/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go index 113de42102..5398bd5415 100644 --- a/api/v1alpha1/serviceresolver_types.go +++ b/api/v1alpha1/serviceresolver_types.go @@ -306,6 +306,8 @@ func (in *ServiceResolver) Validate(namespacesEnabled bool) error { return nil } +// DefaultNamespaceFields has no behaviour here as service-resolver have namespace fields +// that do not default. func (in *ServiceResolver) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { } diff --git a/api/v1alpha1/servicerouter_types.go b/api/v1alpha1/servicerouter_types.go index 567f0306cf..e896668f3a 100644 --- a/api/v1alpha1/servicerouter_types.go +++ b/api/v1alpha1/servicerouter_types.go @@ -256,9 +256,10 @@ func (in *ServiceRouter) Validate(namespacesEnabled bool) error { return nil } +// DefaultNamespaceFields sets the namespace field on spec.routes[].destination to their default values if namespaces are enabled. func (in *ServiceRouter) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { - // If namespaces are enabled we want to set the namespace fields to it's - // default. If namespaces are not enabled (i.e. OSS) we don't set the + // If namespaces are enabled we want to set the namespace fields to their + // defaults. 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 { diff --git a/api/v1alpha1/servicerouter_types_test.go b/api/v1alpha1/servicerouter_types_test.go index 871849b755..fddc7b3e0d 100644 --- a/api/v1alpha1/servicerouter_types_test.go +++ b/api/v1alpha1/servicerouter_types_test.go @@ -601,7 +601,7 @@ func TestServiceRouter_Validate(t *testing.T) { } // Test defaulting behavior when namespaces are enabled as well as disabled. -func TestServiceRouter_Default(t *testing.T) { +func TestServiceRouter_DefaultNamespaceFields(t *testing.T) { namespaceConfig := map[string]struct { enabled bool destinationNamespace string @@ -640,49 +640,48 @@ func TestServiceRouter_Default(t *testing.T) { } for name, s := range namespaceConfig { - input := &ServiceRouter{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceRouterSpec{ - Routes: []ServiceRoute{ - { - Match: &ServiceRouteMatch{ - HTTP: &ServiceRouteHTTPMatch{ - PathPrefix: "/admin", + t.Run(name, func(t *testing.T) { + input := &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destA", }, - }, - Destination: &ServiceRouteDestination{ - Service: "destA", }, }, }, - }, - } - output := &ServiceRouter{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ServiceRouterSpec{ - Routes: []ServiceRoute{ - { - Match: &ServiceRouteMatch{ - HTTP: &ServiceRouteHTTPMatch{ - PathPrefix: "/admin", + } + output := &ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ServiceRouterSpec{ + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destA", + Namespace: s.expectedDestination, }, - }, - Destination: &ServiceRouteDestination{ - Service: "destA", - Namespace: s.expectedDestination, }, }, }, - }, - } - - t.Run(name, func(t *testing.T) { + } input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) diff --git a/api/v1alpha1/servicesplitter_types.go b/api/v1alpha1/servicesplitter_types.go index cab3a68b9a..075399d448 100644 --- a/api/v1alpha1/servicesplitter_types.go +++ b/api/v1alpha1/servicesplitter_types.go @@ -170,6 +170,8 @@ func (in *ServiceSplitter) Validate(namespacesEnabled bool) error { return nil } +// DefaultNamespaceFields has no behaviour here as service-splitter have namespace fields +// that do not default. func (in *ServiceSplitter) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { } diff --git a/api/v1alpha1/terminatinggateway_types.go b/api/v1alpha1/terminatinggateway_types.go index 0478b22384..a8a402d560 100644 --- a/api/v1alpha1/terminatinggateway_types.go +++ b/api/v1alpha1/terminatinggateway_types.go @@ -189,9 +189,10 @@ func (in *TerminatingGateway) Validate(namespacesEnabled bool) error { return nil } +// DefaultNamespaceFields sets the namespace field on spec.services to their default values if namespaces are enabled. func (in *TerminatingGateway) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { - // If namespaces are enabled we want to set the namespace fields to it's - // default. If namespaces are not enabled (i.e. OSS) we don't set the + // If namespaces are enabled we want to set the namespace fields to their + // defaults. 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 { diff --git a/api/v1alpha1/terminatinggateway_types_test.go b/api/v1alpha1/terminatinggateway_types_test.go index 3dbcb877e0..10ba5c79ee 100644 --- a/api/v1alpha1/terminatinggateway_types_test.go +++ b/api/v1alpha1/terminatinggateway_types_test.go @@ -281,7 +281,7 @@ func TestTerminatingGateway_Validate(t *testing.T) { } // Test defaulting behavior when namespaces are enabled as well as disabled. -func TestTerminatingGateway_Default(t *testing.T) { +func TestTerminatingGateway_DefaultNamespaceFields(t *testing.T) { namespaceConfig := map[string]struct { enabled bool destinationNamespace string @@ -320,35 +320,34 @@ func TestTerminatingGateway_Default(t *testing.T) { } for name, s := range namespaceConfig { - input := &TerminatingGateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: TerminatingGatewaySpec{ - Services: []LinkedService{ - { - Name: "foo", + t.Run(name, func(t *testing.T) { + input := &TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: TerminatingGatewaySpec{ + Services: []LinkedService{ + { + Name: "foo", + }, }, }, - }, - } - output := &TerminatingGateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: TerminatingGatewaySpec{ - Services: []LinkedService{ - { - Name: "foo", - Namespace: s.expectedDestination, + } + output := &TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: TerminatingGatewaySpec{ + Services: []LinkedService{ + { + Name: "foo", + Namespace: s.expectedDestination, + }, }, }, - }, - } - - t.Run(name, func(t *testing.T) { + } input.DefaultNamespaceFields(s.enabled, s.destinationNamespace, s.mirroring, s.prefix) require.True(t, cmp.Equal(input, output)) }) From 642d79ba57c942af84cda4d9ce2a80d69ea7483d Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Thu, 14 Jan 2021 10:20:15 -0500 Subject: [PATCH 5/5] Validate namespace field do not get over-written if already set. - Add test to verify DefaultingPatches invokes DefaultNamespaceFields. - Add return statement to DefaultNamespaceFields with no behavior. --- api/common/configentry_webhook_test.go | 21 ++++++++++++++++++ api/v1alpha1/ingressgateway_types.go | 1 + api/v1alpha1/ingressgateway_types_test.go | 8 +++++++ api/v1alpha1/serviceintentions_types.go | 1 + api/v1alpha1/serviceresolver_types.go | 1 + api/v1alpha1/servicerouter_types.go | 1 + api/v1alpha1/servicerouter_types_test.go | 22 +++++++++++++++++++ api/v1alpha1/servicesplitter_types.go | 1 + api/v1alpha1/terminatinggateway_types.go | 1 + api/v1alpha1/terminatinggateway_types_test.go | 8 +++++++ 10 files changed, 65 insertions(+) diff --git a/api/common/configentry_webhook_test.go b/api/common/configentry_webhook_test.go index 4b5ae29454..f49b3ea453 100644 --- a/api/common/configentry_webhook_test.go +++ b/api/common/configentry_webhook_test.go @@ -9,6 +9,7 @@ import ( logrtest "github.com/go-logr/logr/testing" capi "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" + "gomodules.xyz/jsonpatch/v2" "k8s.io/api/admission/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -125,6 +126,25 @@ func TestValidateConfigEntry(t *testing.T) { } } +func TestDefaultingPatches(t *testing.T) { + cfgEntry := &mockConfigEntry{ + MockName: "test", + Valid: true, + } + + // This test validates that DefaultingPatches invokes DefaultNamespaceFields on the Config Entry. + patches, err := DefaultingPatches(cfgEntry, false, false, "", "") + require.NoError(t, err) + + require.Equal(t, []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/MockNamespace", + Value: "bar", + }, + }, patches) +} + type mockConfigEntryLister struct { Resources []ConfigEntryResource } @@ -205,6 +225,7 @@ func (in *mockConfigEntry) Validate(bool) error { } func (in *mockConfigEntry) DefaultNamespaceFields(consulNamespacesEnabled bool, destinationNamespace string, mirroring bool, prefix string) { + in.MockNamespace = "bar" } func (in *mockConfigEntry) MatchesConsul(_ capi.ConfigEntry) bool { diff --git a/api/v1alpha1/ingressgateway_types.go b/api/v1alpha1/ingressgateway_types.go index e96f876fc3..40d3d9c1d3 100644 --- a/api/v1alpha1/ingressgateway_types.go +++ b/api/v1alpha1/ingressgateway_types.go @@ -233,6 +233,7 @@ func (in *IngressGateway) DefaultNamespaceFields(consulNamespacesEnabled bool, d // namespace fields because this would cause errors // making API calls (because namespace fields can't be set in OSS). if consulNamespacesEnabled { + // Default to the current namespace (i.e. the namespace of the config entry). namespace := namespaces.ConsulNamespace(in.Namespace, consulNamespacesEnabled, destinationNamespace, mirroring, prefix) for i, listener := range in.Spec.Listeners { for j, service := range listener.Services { diff --git a/api/v1alpha1/ingressgateway_types_test.go b/api/v1alpha1/ingressgateway_types_test.go index 76ee463d3b..789320ef6e 100644 --- a/api/v1alpha1/ingressgateway_types_test.go +++ b/api/v1alpha1/ingressgateway_types_test.go @@ -514,6 +514,10 @@ func TestIngressGateway_DefaultNamespaceFields(t *testing.T) { { Name: "name", }, + { + Name: "other-name", + Namespace: "other", + }, }, }, }, @@ -533,6 +537,10 @@ func TestIngressGateway_DefaultNamespaceFields(t *testing.T) { Name: "name", Namespace: s.expectedDestination, }, + { + Name: "other-name", + Namespace: "other", + }, }, }, }, diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 5253ec0368..b58e80944a 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -278,6 +278,7 @@ func (in *ServiceIntentions) DefaultNamespaceFields(consulNamespacesEnabled bool // namespace fields because this would cause errors // making API calls (because namespace fields can't be set in OSS). if consulNamespacesEnabled { + // Default to the current namespace (i.e. the namespace of the config entry). namespace := namespaces.ConsulNamespace(in.Namespace, consulNamespacesEnabled, destinationNamespace, mirroring, prefix) if in.Spec.Destination.Namespace == "" { in.Spec.Destination.Namespace = namespace diff --git a/api/v1alpha1/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go index 5398bd5415..d2d9ce30ca 100644 --- a/api/v1alpha1/serviceresolver_types.go +++ b/api/v1alpha1/serviceresolver_types.go @@ -309,6 +309,7 @@ func (in *ServiceResolver) Validate(namespacesEnabled bool) error { // DefaultNamespaceFields has no behaviour here as service-resolver have namespace fields // that do not default. func (in *ServiceResolver) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { + return } func (in ServiceResolverSubsetMap) toConsul() map[string]capi.ServiceResolverSubset { diff --git a/api/v1alpha1/servicerouter_types.go b/api/v1alpha1/servicerouter_types.go index e896668f3a..c08042b7af 100644 --- a/api/v1alpha1/servicerouter_types.go +++ b/api/v1alpha1/servicerouter_types.go @@ -263,6 +263,7 @@ func (in *ServiceRouter) DefaultNamespaceFields(consulNamespacesEnabled bool, de // namespace fields because this would cause errors // making API calls (because namespace fields can't be set in OSS). if consulNamespacesEnabled { + // Default to the current namespace (i.e. the namespace of the config entry). namespace := namespaces.ConsulNamespace(in.Namespace, consulNamespacesEnabled, destinationNamespace, mirroring, prefix) for i, r := range in.Spec.Routes { if r.Destination != nil { diff --git a/api/v1alpha1/servicerouter_types_test.go b/api/v1alpha1/servicerouter_types_test.go index fddc7b3e0d..00efa1c44f 100644 --- a/api/v1alpha1/servicerouter_types_test.go +++ b/api/v1alpha1/servicerouter_types_test.go @@ -658,6 +658,17 @@ func TestServiceRouter_DefaultNamespaceFields(t *testing.T) { Service: "destA", }, }, + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/other", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destB", + Namespace: "other", + }, + }, }, }, } @@ -679,6 +690,17 @@ func TestServiceRouter_DefaultNamespaceFields(t *testing.T) { Namespace: s.expectedDestination, }, }, + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/other", + }, + }, + Destination: &ServiceRouteDestination{ + Service: "destB", + Namespace: "other", + }, + }, }, }, } diff --git a/api/v1alpha1/servicesplitter_types.go b/api/v1alpha1/servicesplitter_types.go index 075399d448..8c4379d826 100644 --- a/api/v1alpha1/servicesplitter_types.go +++ b/api/v1alpha1/servicesplitter_types.go @@ -173,6 +173,7 @@ func (in *ServiceSplitter) Validate(namespacesEnabled bool) error { // DefaultNamespaceFields has no behaviour here as service-splitter have namespace fields // that do not default. func (in *ServiceSplitter) DefaultNamespaceFields(_ bool, _ string, _ bool, _ string) { + return } func (in ServiceSplits) toConsul() []capi.ServiceSplit { diff --git a/api/v1alpha1/terminatinggateway_types.go b/api/v1alpha1/terminatinggateway_types.go index a8a402d560..fa4e47eb8d 100644 --- a/api/v1alpha1/terminatinggateway_types.go +++ b/api/v1alpha1/terminatinggateway_types.go @@ -196,6 +196,7 @@ func (in *TerminatingGateway) DefaultNamespaceFields(consulNamespacesEnabled boo // namespace fields because this would cause errors // making API calls (because namespace fields can't be set in OSS). if consulNamespacesEnabled { + // Default to the current namespace (i.e. the namespace of the config entry). namespace := namespaces.ConsulNamespace(in.Namespace, consulNamespacesEnabled, destinationNamespace, mirroring, prefix) for i, service := range in.Spec.Services { if service.Namespace == "" { diff --git a/api/v1alpha1/terminatinggateway_types_test.go b/api/v1alpha1/terminatinggateway_types_test.go index 10ba5c79ee..e659ee28b9 100644 --- a/api/v1alpha1/terminatinggateway_types_test.go +++ b/api/v1alpha1/terminatinggateway_types_test.go @@ -331,6 +331,10 @@ func TestTerminatingGateway_DefaultNamespaceFields(t *testing.T) { { Name: "foo", }, + { + Name: "bar", + Namespace: "other", + }, }, }, } @@ -345,6 +349,10 @@ func TestTerminatingGateway_DefaultNamespaceFields(t *testing.T) { Name: "foo", Namespace: s.expectedDestination, }, + { + Name: "bar", + Namespace: "other", + }, }, }, }