Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix incorrect validation for ServiceResolver #456

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Fix incorrect validation for ServiceResolver #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77f9b20
Fix incorrect validation for ServiceResolver
lkysow Mar 15, 2021
f0626bf
changelog
lkysow Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
23 changes: 14 additions & 9 deletions api/v1alpha1/serviceresolver_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using else if here because if they're setting:

field: cookie
sourceIP: true

then thought only one error (can't set both field and sourceIP) is clearer than two: (can't set both field and sourceIP, if field is set fieldValue must also be set)

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 {
Expand Down
60 changes: 58 additions & 2 deletions api/v1alpha1/serviceresolver_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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,
Expand Down