Update field descriptions and code review suggestions

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:

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)
 }

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)

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 {

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))
 		})

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
 }

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
 }

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

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))
 		})

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
-}

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) {
 }
 

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 {

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))
 		})