From a342fd69d5df6a69607ce89461c9e2740f823244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Tue, 24 Oct 2023 14:11:22 +0200 Subject: [PATCH] feat: add KongUpstreamPolicy CRD validation rules --- ...ation.konghq.com_kongupstreampolicies.yaml | 35 ++++ docs/api-reference.md | 18 +- .../parser/translators/kongupstreampolicy.go | 29 ++- .../translators/kongupstreampolicy_test.go | 4 +- .../v1beta1/kongupstreampolicy_types.go | 15 +- .../v1beta1/zz_generated.deepcopy.go | 4 +- test/envtest/crds_envtest_test.go | 178 +++++++++++++++++- 7 files changed, 266 insertions(+), 17 deletions(-) diff --git a/config/crd/bases/configuration.konghq.com_kongupstreampolicies.yaml b/config/crd/bases/configuration.konghq.com_kongupstreampolicies.yaml index 6013050e58..4d741e7d5f 100644 --- a/config/crd/bases/configuration.konghq.com_kongupstreampolicies.yaml +++ b/config/crd/bases/configuration.konghq.com_kongupstreampolicies.yaml @@ -142,6 +142,9 @@ spec: description: HTTPStatuses is a list of HTTP status codes that Kong considers a success. items: + description: HTTPStatus is an HTTP status code. + maximum: 599 + minimum: 100 type: integer type: array interval: @@ -197,6 +200,9 @@ spec: description: HTTPStatuses is a list of HTTP status codes that Kong considers a failure. items: + description: HTTPStatus is an HTTP status code. + maximum: 599 + minimum: 100 type: integer type: array interval: @@ -228,6 +234,9 @@ spec: description: HTTPStatuses is a list of HTTP status codes that Kong considers a success. items: + description: HTTPStatus is an HTTP status code. + maximum: 599 + minimum: 100 type: integer type: array interval: @@ -267,6 +276,9 @@ spec: description: HTTPStatuses is a list of HTTP status codes that Kong considers a failure. items: + description: HTTPStatus is an HTTP status code. + maximum: 599 + minimum: 100 type: integer type: array interval: @@ -305,6 +317,29 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: Only one of spec.hashOn.(cookie|header|uriCapture|queryArg) can + be set. + rule: 'has(self.spec.hashOn) ? [has(self.spec.hashOn.cookie), has(self.spec.hashOn.header), + has(self.spec.hashOn.uriCapture), has(self.spec.hashOn.queryArg)].filter(fieldSet, + fieldSet == true).size() <= 1 : true' + - message: Only one of spec.hashOnFallback.(header|uriCapture|queryArg) can + be set. + rule: 'has(self.spec.hashOnFallback) ? [has(self.spec.hashOnFallback.header), + has(self.spec.hashOnFallback.uriCapture), has(self.spec.hashOnFallback.queryArg)].filter(fieldSet, + fieldSet == true).size() <= 1 : true' + - message: When spec.hashOn.cookie is set, spec.hashOn.cookiePath is required. + rule: 'has(self.spec.hashOn) && has(self.spec.hashOn.cookie) ? has(self.spec.hashOn.cookiePath) + : true' + - message: When spec.hashOn.cookiePath is set, spec.hashOn.cookie is required. + rule: 'has(self.spec.hashOn) && has(self.spec.hashOn.cookiePath) ? has(self.spec.hashOn.cookie) + : true' + - message: spec.hashOnFallback.cookie must not be set. + rule: 'has(self.spec.hashOnFallback) ? !has(self.spec.hashOnFallback.cookie) + : true' + - message: spec.hashOnFallback.cookiePath must not be set. + rule: 'has(self.spec.hashOnFallback) ? !has(self.spec.hashOnFallback.cookiePath) + : true' served: true storage: true subresources: diff --git a/docs/api-reference.md b/docs/api-reference.md index 1e28e7fa26..b8bdf0309b 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -399,6 +399,20 @@ UDPIngress is the Schema for the udpingresses API. +### HTTPStatus + +_Underlying type:_ `integer` + +HTTPStatus is an HTTP status code. + + + + + +_Appears in:_ +- [KongUpstreamHealthcheckHealthy](#kongupstreamhealthcheckhealthy) +- [KongUpstreamHealthcheckUnhealthy](#kongupstreamhealthcheckunhealthy) + ### IngressBackend @@ -526,7 +540,7 @@ KongUpstreamHealthcheckHealthy configures thresholds and HTTP status codes to ma | Field | Description | | --- | --- | -| `httpStatuses` _integer array_ | HTTPStatuses is a list of HTTP status codes that Kong considers a success. | +| `httpStatuses` _[HTTPStatus](#httpstatus) array_ | HTTPStatuses is a list of HTTP status codes that Kong considers a success. | | `interval` _integer_ | Interval is the interval between active health checks for an upstream in seconds when in a healthy state. | | `successes` _integer_ | Successes is the number of successes to consider a target healthy. | @@ -546,7 +560,7 @@ KongUpstreamHealthcheckUnhealthy configures thresholds and HTTP status codes to | Field | Description | | --- | --- | | `httpFailures` _integer_ | HTTPFailures is the number of failures to consider a target unhealthy. | -| `httpStatuses` _integer array_ | HTTPStatuses is a list of HTTP status codes that Kong considers a failure. | +| `httpStatuses` _[HTTPStatus](#httpstatus) array_ | HTTPStatuses is a list of HTTP status codes that Kong considers a failure. | | `tcpFailures` _integer_ | TCPFailures is the number of TCP failures in a row to consider a target unhealthy. | | `timeouts` _integer_ | Timeouts is the number of timeouts in a row to consider a target unhealthy. | | `interval` _integer_ | Interval is the interval between active health checks for an upstream in seconds when in an unhealthy state. | diff --git a/internal/dataplane/parser/translators/kongupstreampolicy.go b/internal/dataplane/parser/translators/kongupstreampolicy.go index 6573b216b3..99101b4894 100644 --- a/internal/dataplane/parser/translators/kongupstreampolicy.go +++ b/internal/dataplane/parser/translators/kongupstreampolicy.go @@ -7,6 +7,13 @@ import ( kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1beta1" ) +const ( + KongHashOnTypeHeader string = "header" + KongHashOnTypeCookie string = "cookie" + KongHashOnTypeQueryArg string = "query_arg" + KongHashOnTypeURICapture string = "uri_capture" +) + // TranslateKongUpstreamPolicy translates KongUpstreamPolicySpec to kong.Upstream. It makes assumption that // KongUpstreamPolicySpec has been validated on the API level. func TranslateKongUpstreamPolicy(policy kongv1beta1.KongUpstreamPolicySpec) *kong.Upstream { @@ -35,16 +42,15 @@ func translateHashOn(hashOn *kongv1beta1.KongUpstreamHash) *string { return nil } // CRD validations will ensure only one of hashOn fields can be set, therefore the order doesn't matter. - // TODO: https://github.com/Kong/kubernetes-ingress-controller/issues/4951 switch { case hashOn.Header != nil: - return lo.ToPtr("header") + return lo.ToPtr(KongHashOnTypeHeader) case hashOn.Cookie != nil: - return lo.ToPtr("cookie") + return lo.ToPtr(KongHashOnTypeCookie) case hashOn.QueryArg != nil: - return lo.ToPtr("query_arg") + return lo.ToPtr(KongHashOnTypeQueryArg) case hashOn.URICapture != nil: - return lo.ToPtr("uri_capture") + return lo.ToPtr(KongHashOnTypeURICapture) default: return nil } @@ -128,7 +134,7 @@ func translateHealthy(healthy *kongv1beta1.KongUpstreamHealthcheckHealthy) *kong return nil } return &kong.Healthy{ - HTTPStatuses: healthy.HTTPStatuses, + HTTPStatuses: translateHTTPStatuses(healthy.HTTPStatuses), Interval: healthy.Interval, Successes: healthy.Successes, } @@ -140,9 +146,18 @@ func translateUnhealthy(unhealthy *kongv1beta1.KongUpstreamHealthcheckUnhealthy) } return &kong.Unhealthy{ HTTPFailures: unhealthy.HTTPFailures, - HTTPStatuses: unhealthy.HTTPStatuses, + HTTPStatuses: translateHTTPStatuses(unhealthy.HTTPStatuses), TCPFailures: unhealthy.TCPFailures, Timeouts: unhealthy.Timeouts, Interval: unhealthy.Interval, } } + +func translateHTTPStatuses(statuses []kongv1beta1.HTTPStatus) []int { + if statuses == nil { + return nil + } + // Using lo.Map only in case healthy.HTTPStatuses is not nil, because lo.Map creates a non-nil slice even + // if the input slice is nil. + return lo.Map(statuses, func(s kongv1beta1.HTTPStatus, _ int) int { return int(s) }) +} diff --git a/internal/dataplane/parser/translators/kongupstreampolicy_test.go b/internal/dataplane/parser/translators/kongupstreampolicy_test.go index 1f05348ed7..dd8da70f0f 100644 --- a/internal/dataplane/parser/translators/kongupstreampolicy_test.go +++ b/internal/dataplane/parser/translators/kongupstreampolicy_test.go @@ -93,13 +93,13 @@ func TestTranslateKongUpstreamPolicy(t *testing.T) { Type: lo.ToPtr("http"), Concurrency: lo.ToPtr(10), Healthy: &kongv1beta1.KongUpstreamHealthcheckHealthy{ - HTTPStatuses: []int{200}, + HTTPStatuses: []kongv1beta1.HTTPStatus{200}, Interval: lo.ToPtr(20), Successes: lo.ToPtr(30), }, Unhealthy: &kongv1beta1.KongUpstreamHealthcheckUnhealthy{ HTTPFailures: lo.ToPtr(40), - HTTPStatuses: []int{500}, + HTTPStatuses: []kongv1beta1.HTTPStatus{500}, Timeouts: lo.ToPtr(60), Interval: lo.ToPtr(70), }, diff --git a/pkg/apis/configuration/v1beta1/kongupstreampolicy_types.go b/pkg/apis/configuration/v1beta1/kongupstreampolicy_types.go index 73d7a69336..489df950f3 100644 --- a/pkg/apis/configuration/v1beta1/kongupstreampolicy_types.go +++ b/pkg/apis/configuration/v1beta1/kongupstreampolicy_types.go @@ -34,6 +34,12 @@ func init() { // +kubebuilder:subresource:status // +kubebuilder:storageversion // +kubebuilder:metadata:labels=gateway.networking.k8s.io/policy=direct +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOn) ? [has(self.spec.hashOn.cookie), has(self.spec.hashOn.header), has(self.spec.hashOn.uriCapture), has(self.spec.hashOn.queryArg)].filter(fieldSet, fieldSet == true).size() <= 1 : true", message="Only one of spec.hashOn.(cookie|header|uriCapture|queryArg) can be set." +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOnFallback) ? [has(self.spec.hashOnFallback.header), has(self.spec.hashOnFallback.uriCapture), has(self.spec.hashOnFallback.queryArg)].filter(fieldSet, fieldSet == true).size() <= 1 : true", message="Only one of spec.hashOnFallback.(header|uriCapture|queryArg) can be set." +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOn) && has(self.spec.hashOn.cookie) ? has(self.spec.hashOn.cookiePath) : true", message="When spec.hashOn.cookie is set, spec.hashOn.cookiePath is required." +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOn) && has(self.spec.hashOn.cookiePath) ? has(self.spec.hashOn.cookie) : true", message="When spec.hashOn.cookiePath is set, spec.hashOn.cookie is required." +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOnFallback) ? !has(self.spec.hashOnFallback.cookie) : true", message="spec.hashOnFallback.cookie must not be set." +// +kubebuilder:validation:XValidation:rule="has(self.spec.hashOnFallback) ? !has(self.spec.hashOnFallback.cookiePath) : true", message="spec.hashOnFallback.cookiePath must not be set." type KongUpstreamPolicy struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` @@ -165,10 +171,15 @@ type KongUpstreamPassiveHealthcheck struct { Unhealthy *KongUpstreamHealthcheckUnhealthy `json:"unhealthy,omitempty"` } +// HTTPStatus is an HTTP status code. +// +kubebuilder:validation:Minimum=100 +// +kubebuilder:validation:Maximum=599 +type HTTPStatus int + // KongUpstreamHealthcheckHealthy configures thresholds and HTTP status codes to mark targets healthy for an upstream. type KongUpstreamHealthcheckHealthy struct { // HTTPStatuses is a list of HTTP status codes that Kong considers a success. - HTTPStatuses []int `json:"httpStatuses,omitempty"` + HTTPStatuses []HTTPStatus `json:"httpStatuses,omitempty"` // Interval is the interval between active health checks for an upstream in seconds when in a healthy state. // +kubebuilder:validation:Minimum=0 @@ -186,7 +197,7 @@ type KongUpstreamHealthcheckUnhealthy struct { HTTPFailures *int `json:"httpFailures,omitempty"` // HTTPStatuses is a list of HTTP status codes that Kong considers a failure. - HTTPStatuses []int `json:"httpStatuses,omitempty"` + HTTPStatuses []HTTPStatus `json:"httpStatuses,omitempty"` // TCPFailures is the number of TCP failures in a row to consider a target unhealthy. // +kubebuilder:validation:Minimum=0 diff --git a/pkg/apis/configuration/v1beta1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1beta1/zz_generated.deepcopy.go index 6d0f22ea1a..03f67897d1 100644 --- a/pkg/apis/configuration/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1beta1/zz_generated.deepcopy.go @@ -302,7 +302,7 @@ func (in *KongUpstreamHealthcheckHealthy) DeepCopyInto(out *KongUpstreamHealthch *out = *in if in.HTTPStatuses != nil { in, out := &in.HTTPStatuses, &out.HTTPStatuses - *out = make([]int, len(*in)) + *out = make([]HTTPStatus, len(*in)) copy(*out, *in) } if in.Interval != nil { @@ -337,7 +337,7 @@ func (in *KongUpstreamHealthcheckUnhealthy) DeepCopyInto(out *KongUpstreamHealth } if in.HTTPStatuses != nil { in, out := &in.HTTPStatuses, &out.HTTPStatuses - *out = make([]int, len(*in)) + *out = make([]HTTPStatus, len(*in)) copy(*out, *in) } if in.TCPFailures != nil { diff --git a/test/envtest/crds_envtest_test.go b/test/envtest/crds_envtest_test.go index 38bb88514c..295cfde3f7 100644 --- a/test/envtest/crds_envtest_test.go +++ b/test/envtest/crds_envtest_test.go @@ -4,6 +4,7 @@ package envtest import ( "context" + "fmt" "strings" "testing" "time" @@ -18,6 +19,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kong/kubernetes-ingress-controller/v2/internal/annotations" "github.com/kong/kubernetes-ingress-controller/v2/internal/manager" @@ -109,7 +111,7 @@ func TestCRDValidations(t *testing.T) { ctx := context.Background() scheme := Scheme(t, WithKong) envcfg := Setup(t, scheme) - client := NewControllerClient(t, scheme, envcfg) + ctrlClient := NewControllerClient(t, scheme, envcfg) testCases := []struct { name string @@ -175,12 +177,130 @@ func TestCRDValidations(t *testing.T) { require.ErrorContains(t, err, "spec.rules[0].port") }, }, + { + name: "KongUpstreamPolicy - only one of spec.hashOn.(cookie|header|uriCapture|queryArg) can be set", + scenario: func(ctx context.Context, t *testing.T, ns string) { + for i, invalidHashOn := range generateInvalidHashOns() { + invalidHashOn := invalidHashOn + t.Run(fmt.Sprintf("invalidHashOn[%d]", i), func(t *testing.T) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOn: &invalidHashOn, + }) + require.ErrorContains(t, err, "Only one of spec.hashOn.(cookie|header|uriCapture|queryArg) can be set.") + }) + } + }, + }, + { + name: "KongUpstreamPolicy - only one of spec.hashOnFallback.(header|uriCapture|queryArg) can be set", + scenario: func(ctx context.Context, t *testing.T, ns string) { + invalidHashOns := lo.Reject(generateInvalidHashOns(), func(hashOn kongv1beta1.KongUpstreamHash, _ int) bool { + // Filter out Cookie which is not allowed in spec.hashOnFallback. + return hashOn.Cookie != nil + }) + for i, invalidHashOn := range invalidHashOns { + invalidHashOn := invalidHashOn + t.Run(fmt.Sprintf("invalidHashOn[%d]", i), func(t *testing.T) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOnFallback: &invalidHashOn, + }) + require.ErrorContains(t, err, "Only one of spec.hashOnFallback.(header|uriCapture|queryArg) can be set.") + }) + } + }, + }, + { + name: "KongUpstreamPolicy - spec.hashOn.cookie and spec.hashOn.cookiePath are set", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOn: &kongv1beta1.KongUpstreamHash{ + Cookie: lo.ToPtr("cookie-name"), + CookiePath: lo.ToPtr("/"), + }, + }) + require.NoError(t, err) + }, + }, + { + name: "KongUpstreamPolicy - spec.hashOn.cookie is set, spec.hashOn.cookiePath is required", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOn: &kongv1beta1.KongUpstreamHash{ + Cookie: lo.ToPtr("cookie-name"), + }, + }) + require.ErrorContains(t, err, "When spec.hashOn.cookie is set, spec.hashOn.cookiePath is required.") + }, + }, + { + name: "KongUpstreamPolicy - spec.hashOn.cookiePath is set, spec.hashOn.cookie is required", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOn: &kongv1beta1.KongUpstreamHash{ + CookiePath: lo.ToPtr("/"), + }, + }) + require.ErrorContains(t, err, "When spec.hashOn.cookiePath is set, spec.hashOn.cookie is required.") + }, + }, + { + name: "KongUpstreamPolicy - spec.hashOnFallback.cookie must not be set", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOnFallback: &kongv1beta1.KongUpstreamHash{ + CookiePath: lo.ToPtr("/"), + }, + }) + require.ErrorContains(t, err, "spec.hashOnFallback.cookiePath must not be set.") + }, + }, + { + name: "KongUpstreamPolicy - spec.hashOnFallback.cookiePath must not be set", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + HashOnFallback: &kongv1beta1.KongUpstreamHash{ + CookiePath: lo.ToPtr("/"), + }, + }) + require.ErrorContains(t, err, "spec.hashOnFallback.cookiePath must not be set.") + }, + }, + { + name: "KongUpstreamPolicy - healthchecks.active.healthy.httpStatuses contains invalid HTTP status code", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + Healthchecks: &kongv1beta1.KongUpstreamHealthcheck{ + Active: &kongv1beta1.KongUpstreamActiveHealthcheck{ + Healthy: &kongv1beta1.KongUpstreamHealthcheckHealthy{ + HTTPStatuses: []kongv1beta1.HTTPStatus{600}, + }, + }, + }, + }) + require.ErrorContains(t, err, "should be less than or equal to 599") + }, + }, + { + name: "KongUpstreamPolicy - healthchecks.active.unhealthy.httpStatuses contains invalid HTTP status code", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongUpstreamPolicy(ctx, ctrlClient, ns, kongv1beta1.KongUpstreamPolicySpec{ + Healthchecks: &kongv1beta1.KongUpstreamHealthcheck{ + Active: &kongv1beta1.KongUpstreamActiveHealthcheck{ + Unhealthy: &kongv1beta1.KongUpstreamHealthcheckUnhealthy{ + HTTPStatuses: []kongv1beta1.HTTPStatus{99}, + }, + }, + }, + }) + require.ErrorContains(t, err, "should be greater than or equal to 100") + }, + }, } for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - ns := CreateNamespace(ctx, t, client) + ns := CreateNamespace(ctx, t, ctrlClient) tc.scenario(ctx, t, ns.Name) }) } @@ -259,3 +379,57 @@ func validUDPIngress() *kongv1beta1.UDPIngress { }, } } + +func createKongUpstreamPolicy(ctx context.Context, client client.Client, ns string, spec kongv1beta1.KongUpstreamPolicySpec) error { + return client.Create(ctx, &kongv1beta1.KongUpstreamPolicy{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + Namespace: ns, + }, + Spec: spec, + }) +} + +// generateInvalidHashOns generates a list of KongUpstreamHash objects with all possible invalid fields pairs. +func generateInvalidHashOns() []kongv1beta1.KongUpstreamHash { + fieldSetFns := []func(h *kongv1beta1.KongUpstreamHash){ + func(h *kongv1beta1.KongUpstreamHash) { + h.Cookie = lo.ToPtr("cookie-name") + h.CookiePath = lo.ToPtr("/") + }, + func(h *kongv1beta1.KongUpstreamHash) { + h.Header = lo.ToPtr("header-name") + }, + func(h *kongv1beta1.KongUpstreamHash) { + h.URICapture = lo.ToPtr("uri-capture") + }, + func(h *kongv1beta1.KongUpstreamHash) { + h.QueryArg = lo.ToPtr("query-arg") + }, + } + + var invalidHashOns []kongv1beta1.KongUpstreamHash + for outerIdx, fieldSetFn := range fieldSetFns { + hashOn := kongv1beta1.KongUpstreamHash{} + fieldSetFn(&hashOn) + + for innerIdx, innerFieldSetFn := range fieldSetFns { + if outerIdx == innerIdx { + continue + } + invalidHashOn := hashOn.DeepCopy() + innerFieldSetFn(invalidHashOn) + invalidHashOns = append(invalidHashOns, *invalidHashOn) + } + } + + optStr := func(s *string) string { + if s == nil { + return "" + } + return *s + } + return lo.UniqBy(invalidHashOns, func(h kongv1beta1.KongUpstreamHash) string { + return fmt.Sprintf("%s.%s.%s.%s", optStr(h.Cookie), optStr(h.Header), optStr(h.URICapture), optStr(h.QueryArg)) + }) +}