Fix incorrect validation for ServiceResolver (#456)

* Fix incorrect validation for ServiceResolver * changelog
hashicorp · Mar 15, 2021 · c70361e · c70361e
1 parent a3a6dd3
commit c70361e
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,17 +3,20 @@
 FEATURES:
 * Metrics: add metrics configuration to inject-connect and metrics-merging capability to consul-sidecar. When metrics and metrics merging are enabled, the consul-sidecar will expose an endpoint that merges the app and proxy metrics.
 
-The flags `-merged-metrics-port`, `-service-metrics-port` and `-service-metrics-path` can be used to configure the merged metrics server, and the application service metrics endpoint on the consul sidecar.
+  The flags `-merged-metrics-port`, `-service-metrics-port` and `-service-metrics-path` can be used to configure the merged metrics server, and the application service metrics endpoint on the consul sidecar.
 
-The flags `-default-enable-metrics`, `-default-enable-metrics-merging`, `-default-merged-metrics-port`, `-default-prometheus-scrape-port` and `-default-prometheus-scrape-path` configure the inject-connect command.
+  The flags `-default-enable-metrics`, `-default-enable-metrics-merging`, `-default-merged-metrics-port`, `-default-prometheus-scrape-port` and `-default-prometheus-scrape-path` configure the inject-connect command.
 
-IMPROVEMENTS
+IMPROVEMENTS:
 * CRDs: add field Last Synced Time to CRD status and add printer column on CRD to display time since when the
   resource was last successfully synced with Consul. [[GH-448](https://github.com/hashicorp/consul-k8s/pull/448)]
+
+BUG FIXES:
+* CRDs: fix incorrect validation for `ServiceResolver`. [[GH-456](https://github.com/hashicorp/consul-k8s/pull/456)]
 
 ## 0.24.0 (February 16, 2021)
 
-BREAKING CHANGES
+BREAKING CHANGES:
 * Connect: the `lifecycle-sidecar` command has been renamed to `consul-sidecar`. [[GH-428](https://github.com/hashicorp/consul-k8s/pull/428)]
 * Connect: the `consul-connect-lifecycle-sidecar` container name has been changed to `consul-sidecar` and the `consul-connect-envoy-sidecar` container name has been changed to `envoy-sidecar`. 
 [[GH-428](https://github.com/hashicorp/consul-k8s/pull/428)]

diff --git a/api/v1alpha1/serviceresolver_types.go b/api/v1alpha1/serviceresolver_types.go
@@ -478,16 +478,21 @@ func (in *LoadBalancer) validate(path *field.Path) field.ErrorList {
 
 func (in HashPolicy) validate(path *field.Path) field.ErrorList {
 	var errs field.ErrorList
-	validFields := []string{"header", "cookie", "query_parameter"}
-	if !sliceContains(validFields, in.Field) {
-		errs = append(errs, field.Invalid(path.Child("field"), in.Field,
-			notInSliceMessage(validFields)))
-	}
+	if in.Field != "" {
+		validFields := []string{"header", "cookie", "query_parameter"}
+		if !sliceContains(validFields, in.Field) {
+			errs = append(errs, field.Invalid(path.Child("field"), in.Field,
+				notInSliceMessage(validFields)))
+		}
 
-	if in.Field != "" && in.SourceIP {
-		asJSON, _ := json.Marshal(in)
-		errs = append(errs, field.Invalid(path, string(asJSON),
-			"cannot set both field and sourceIP"))
+		if in.SourceIP {
+			asJSON, _ := json.Marshal(in)
+			errs = append(errs, field.Invalid(path, string(asJSON),
+				"cannot set both field and sourceIP"))
+		} else if in.FieldValue == "" {
+			errs = append(errs, field.Invalid(path.Child("fieldValue"), in.FieldValue,
+				"fieldValue cannot be empty if field is set"))
+		}
 	}
 
 	if err := in.CookieConfig.validate(path.Child("cookieConfig")); err != nil {

diff --git a/api/v1alpha1/serviceresolver_types_test.go b/api/v1alpha1/serviceresolver_types_test.go
@@ -542,9 +542,48 @@ func TestServiceResolver_Validate(t *testing.T) {
 			},
 			namespacesEnabled: false,
 			expectedErrMsgs: []string{
-				`serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].field: Invalid value: "invalid": must be one of "header", "cookie", "query_parameter"`,
+				`serviceresolver.consul.hashicorp.com "foo" is invalid: [spec.loadBalancer.hashPolicies[0].field: Invalid value: "invalid": must be one of "header", "cookie", "query_parameter"`,
+				`spec.loadBalancer.hashPolicies[0].fieldValue: Invalid value: "": fieldValue cannot be empty if field is set`,
 			},
 		},
+		"hashPolicy.field without fieldValue": {
+			input: &ServiceResolver{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: ServiceResolverSpec{
+					LoadBalancer: &LoadBalancer{
+						HashPolicies: []HashPolicy{
+							{
+								Field: "header",
+							},
+						},
+					},
+				},
+			},
+			namespacesEnabled: false,
+			expectedErrMsgs: []string{
+				`serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0].fieldValue: Invalid value: "": fieldValue cannot be empty if field is set`,
+			},
+		},
+		"hashPolicy just sourceIP set": {
+			input: &ServiceResolver{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: ServiceResolverSpec{
+					LoadBalancer: &LoadBalancer{
+						HashPolicies: []HashPolicy{
+							{
+								SourceIP: true,
+							},
+						},
+					},
+				},
+			},
+			namespacesEnabled: false,
+			expectedErrMsgs:   nil,
+		},
 		"hashPolicy sourceIP and field set": {
 			input: &ServiceResolver{
 				ObjectMeta: metav1.ObjectMeta{
@@ -566,6 +605,22 @@ func TestServiceResolver_Validate(t *testing.T) {
 				`serviceresolver.consul.hashicorp.com "foo" is invalid: spec.loadBalancer.hashPolicies[0]: Invalid value: "{\"field\":\"header\",\"sourceIP\":true}": cannot set both field and sourceIP`,
 			},
 		},
+		"hashPolicy nothing set is valid": {
+			input: &ServiceResolver{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: ServiceResolverSpec{
+					LoadBalancer: &LoadBalancer{
+						HashPolicies: []HashPolicy{
+							{},
+						},
+					},
+				},
+			},
+			namespacesEnabled: false,
+			expectedErrMsgs:   nil,
+		},
 		"cookieConfig session and ttl set": {
 			input: &ServiceResolver{
 				ObjectMeta: metav1.ObjectMeta{
@@ -575,7 +630,8 @@ func TestServiceResolver_Validate(t *testing.T) {
 					LoadBalancer: &LoadBalancer{
 						HashPolicies: []HashPolicy{
 							{
-								Field: "cookie",
+								Field:      "cookie",
+								FieldValue: "cookiename",
 								CookieConfig: &CookieConfig{
 									Session: true,
 									TTL:     100,