From 7acec7abe9b6845050ec24f408d9d40dc4c00281 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Wed, 13 Jan 2021 09:54:22 -0500 Subject: [PATCH] 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)) })