addressed review comments

hashicorp · Jan 12, 2023 · e15506a · e15506a
1 parent 057e1d3
commit e15506a
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 60 deletions.
diff --git a/control-plane/api/v1alpha1/proxydefaults_types.go b/control-plane/api/v1alpha1/proxydefaults_types.go
@@ -244,7 +244,7 @@ func (in *ProxyDefaults) validateConfig(path *field.Path) *field.Error {
 	}
 	var outConfig map[string]interface{}
 	if err := json.Unmarshal(in.Spec.Config, &outConfig); err != nil {
-		return field.Invalid(path, in.Spec.Config, fmt.Sprintf(`must be valid map value: %s`, err))
+		return field.Invalid(path, string(in.Spec.Config), fmt.Sprintf(`must be valid map value: %s`, err))
 	}
 	return nil
 }

diff --git a/control-plane/api/v1alpha1/proxydefaults_types_test.go b/control-plane/api/v1alpha1/proxydefaults_types_test.go
@@ -588,6 +588,23 @@ func TestProxyDefaults_Validate(t *testing.T) {
 			},
 			expectedErrMsg: `proxydefaults.consul.hashicorp.com "global" is invalid: [spec.envoyExtensions.envoyExtension[0].arguments: Required value: arguments must be defined, spec.envoyExtensions.envoyExtension[1].arguments: Required value: arguments must be defined]`,
 		},
+		"envoyExtension.arguments invalid json": {
+			input: &ProxyDefaults{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "global",
+				},
+				Spec: ProxyDefaultsSpec{
+					EnvoyExtensions: EnvoyExtensions{
+						EnvoyExtension{
+							Name:      "aws_request_signing",
+							Arguments: json.RawMessage(`{"SOME_INVALID_JSON"}`),
+							Required:  false,
+						},
+					},
+				},
+			},
+			expectedErrMsg: `proxydefaults.consul.hashicorp.com "global" is invalid: spec.envoyExtensions.envoyExtension[0].arguments: Invalid value: "{\"SOME_INVALID_JSON\"}": must be valid map value: invalid character '}' after object key`,
+		},
 		"multi-error": {
 			input: &ProxyDefaults{
 				ObjectMeta: metav1.ObjectMeta{

diff --git a/control-plane/api/v1alpha1/servicedefaults_types.go b/control-plane/api/v1alpha1/servicedefaults_types.go
@@ -1,7 +1,6 @@
 package v1alpha1
 
 import (
-	"encoding/json"
 	"fmt"
 	"net"
 	"strings"
@@ -190,18 +189,6 @@ type ServiceDefaultsDestination struct {
 	Port uint32 `json:"port,omitempty"`
 }
 
-// EnvoyExtension has configuration for an extension that patches Envoy resources.
-type EnvoyExtension struct {
-	Name     string `json:"name,omitempty"`
-	Required bool   `json:"required,omitempty"`
-	// +kubebuilder:validation:Type=object
-	// +kubebuilder:validation:Schemaless
-	// +kubebuilder:pruning:PreserveUnknownFields
-	Arguments json.RawMessage `json:"arguments,omitempty"`
-}
-
-type EnvoyExtensions []EnvoyExtension
-
 func (in *ServiceDefaults) ConsulKind() string {
 	return capi.ServiceDefaults
 }
@@ -518,49 +505,3 @@ func (in *ServiceDefaults) MatchesConsul(candidate capi.ConfigEntry) bool {
 func (in *ServiceDefaults) ConsulGlobalResource() bool {
 	return false
 }
-
-func (in EnvoyExtensions) toConsul() []capi.EnvoyExtension {
-	if in == nil {
-		return nil
-	}
-
-	outConfig := make([]capi.EnvoyExtension, 0)
-
-	for _, e := range in {
-		consulExtension := capi.EnvoyExtension{
-			Name:     e.Name,
-			Required: e.Required,
-		}
-
-		// We already validate that arguments is present
-		var args map[string]interface{}
-		_ = json.Unmarshal(e.Arguments, &args)
-		consulExtension.Arguments = args
-		outConfig = append(outConfig, consulExtension)
-	}
-
-	return outConfig
-}
-
-func (in EnvoyExtensions) validate(path *field.Path) field.ErrorList {
-	if len(in) == 0 {
-		return nil
-	}
-
-	var errs field.ErrorList
-	for i, e := range in {
-		if err := e.validate(path.Child("envoyExtension").Index(i)); err != nil {
-			errs = append(errs, err)
-		}
-	}
-
-	return errs
-}
-
-func (in EnvoyExtension) validate(path *field.Path) *field.Error {
-	if in.Arguments == nil {
-		err := field.Required(path.Child("arguments"), "arguments must be defined")
-		return err
-	}
-	return nil
-}
diff --git a/control-plane/api/v1alpha1/servicedefaults_types_test.go b/control-plane/api/v1alpha1/servicedefaults_types_test.go
@@ -1103,6 +1103,23 @@ func TestServiceDefaults_Validate(t *testing.T) {
 			},
 			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: [spec.envoyExtensions.envoyExtension[0].arguments: Required value: arguments must be defined, spec.envoyExtensions.envoyExtension[1].arguments: Required value: arguments must be defined]`,
 		},
+		"envoyExtension.arguments (invalid json)": {
+			input: &ServiceDefaults{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "my-service",
+				},
+				Spec: ServiceDefaultsSpec{
+					EnvoyExtensions: EnvoyExtensions{
+						EnvoyExtension{
+							Name:      "aws_request_signing",
+							Arguments: json.RawMessage(`{"SOME_INVALID_JSON"}`),
+							Required:  false,
+						},
+					},
+				},
+			},
+			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: spec.envoyExtensions.envoyExtension[0].arguments: Invalid value: "{\"SOME_INVALID_JSON\"}": must be valid map value: invalid character '}' after object key`,
+		},
 	}
 
 	for name, testCase := range cases {

diff --git a/control-plane/api/v1alpha1/shared_types.go b/control-plane/api/v1alpha1/shared_types.go
@@ -1,6 +1,7 @@
 package v1alpha1
 
 import (
+	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -79,6 +80,19 @@ type HTTPHeaderModifiers struct {
 	Remove []string `json:"remove,omitempty"`
 }
 
+// EnvoyExtension has configuration for an extension that patches Envoy resources.
+type EnvoyExtension struct {
+	Name     string `json:"name,omitempty"`
+	Required bool   `json:"required,omitempty"`
+	// +kubebuilder:validation:Type=object
+	// +kubebuilder:validation:Schemaless
+	// +kubebuilder:pruning:PreserveUnknownFields
+	Arguments json.RawMessage `json:"arguments,omitempty"`
+}
+
+// EnvoyExtensions represents a list of the EnvoyExtension configuration.
+type EnvoyExtensions []EnvoyExtension
+
 func (in MeshGateway) toConsul() capi.MeshGatewayConfig {
 	mode := capi.MeshGatewayMode(in.Mode)
 	switch mode {
@@ -176,6 +190,58 @@ func (in *HTTPHeaderModifiers) toConsul() *capi.HTTPHeaderModifiers {
 	}
 }
 
+func (in EnvoyExtensions) toConsul() []capi.EnvoyExtension {
+	if in == nil {
+		return nil
+	}
+
+	outConfig := make([]capi.EnvoyExtension, 0)
+
+	for _, e := range in {
+		consulExtension := capi.EnvoyExtension{
+			Name:     e.Name,
+			Required: e.Required,
+		}
+
+		// We already validate that arguments is present
+		var args map[string]interface{}
+		_ = json.Unmarshal(e.Arguments, &args)
+		consulExtension.Arguments = args
+		outConfig = append(outConfig, consulExtension)
+	}
+
+	return outConfig
+}
+
+func (in EnvoyExtensions) validate(path *field.Path) field.ErrorList {
+	if len(in) == 0 {
+		return nil
+	}
+
+	var errs field.ErrorList
+	for i, e := range in {
+		if err := e.validate(path.Child("envoyExtension").Index(i)); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	return errs
+}
+
+func (in EnvoyExtension) validate(path *field.Path) *field.Error {
+	// Validate that the arguments are not nil
+	if in.Arguments == nil {
+		err := field.Required(path.Child("arguments"), "arguments must be defined")
+		return err
+	}
+	// Validate that the arguments are valid json
+	var outConfig map[string]interface{}
+	if err := json.Unmarshal(in.Arguments, &outConfig); err != nil {
+		return field.Invalid(path.Child("arguments"), string(in.Arguments), fmt.Sprintf(`must be valid map value: %s`, err))
+	}
+	return nil
+}
+
 func notInSliceMessage(slice []string) string {
 	return fmt.Sprintf(`must be one of "%s"`, strings.Join(slice, `", "`))
 }