From 35c62b65db7b22cb0919ebd53678c8df9fb89e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Tue, 31 Oct 2023 12:59:35 +0100 Subject: [PATCH] do not allow setting proxy and route --- CHANGELOG.md | 9 +++--- ...onfiguration.konghq.com_kongingresses.yaml | 7 +++++ internal/admission/handler.go | 5 ++-- internal/admission/handler_test.go | 2 +- internal/dataplane/kongstate/kongstate.go | 8 ++--- .../configuration/v1/kongingress_types.go | 5 ++-- .../all-in-one-dbless-k4k8s-enterprise.yaml | 7 +++++ .../all-in-one-dbless-konnect-enterprise.yaml | 7 +++++ .../manifests/all-in-one-dbless-konnect.yaml | 7 +++++ test/e2e/manifests/all-in-one-dbless.yaml | 7 +++++ .../all-in-one-postgres-enterprise.yaml | 7 +++++ test/e2e/manifests/all-in-one-postgres.yaml | 7 +++++ test/envtest/crds_envtest_test.go | 29 +++++++++++++++++++ test/integration/kongingress_webhook_test.go | 14 +++------ 14 files changed, 97 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fee9bfaaf0..e0208fe1e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,10 +130,11 @@ Adding a new version? You'll need three changes: expected to functionally affect routing, but may affect performance for some configurations. [#4934](https://github.com/Kong/kubernetes-ingress-controller/pull/4934) -- KongIngress is deprecated and will be removed in a future release. Settings - previously managed by KongIngress must be migrated to annotations and the new - KongUpstreamPolicy resource. - [#4989](https://github.com/Kong/kubernetes-ingress-controller/pull/4989) +- KongIngress is now entirely deprecated and will be removed in a future release. + Its fields that were previously deprecated (`proxy` and `route`) are now not allowed to be set. + They must be migrated to annotations. `upstream` field is deprecated - it's recommended + to migrate its settings to the new KongUpstreamPolicy resource. + [#5029](https://github.com/Kong/kubernetes-ingress-controller/pull/5029) ### Fixed diff --git a/config/crd/bases/configuration.konghq.com_kongingresses.yaml b/config/crd/bases/configuration.konghq.com_kongingresses.yaml index 1fd2b23738..302eaff0ab 100644 --- a/config/crd/bases/configuration.konghq.com_kongingresses.yaml +++ b/config/crd/bases/configuration.konghq.com_kongingresses.yaml @@ -351,6 +351,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/internal/admission/handler.go b/internal/admission/handler.go index fa7f722918..507b2a9d9b 100644 --- a/internal/admission/handler.go +++ b/internal/admission/handler.go @@ -313,10 +313,11 @@ func (h RequestHandler) handleKongIngress(_ context.Context, request admissionv1 // KongIngress is always allowed. responseBuilder = responseBuilder.Allowed(true) + // Proxy and Route fields are now disallowed to be set with the use of CEL rules in the CRD definition. + // We still warn about them here only just in case someone doesn't install new CRDs with CEL rules. if kongIngress.Proxy != nil { responseBuilder = responseBuilder.WithWarning(proxyWarning) } - if kongIngress.Route != nil { responseBuilder = responseBuilder.WithWarning(routeWarning) } @@ -345,7 +346,7 @@ func (h RequestHandler) handleService(_ context.Context, request admissionv1.Adm if annotations.ExtractConfigurationName(service.Annotations) != "" { warning := fmt.Sprintf(serviceWarning, annotations.AnnotationPrefix+annotations.ConfigurationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey) + kongv1beta1.KongUpstreamPolicyAnnotationKey) responseBuilder = responseBuilder.WithWarning(warning) } diff --git a/internal/admission/handler_test.go b/internal/admission/handler_test.go index ab5d25ee31..c99d8e6583 100644 --- a/internal/admission/handler_test.go +++ b/internal/admission/handler_test.go @@ -102,7 +102,7 @@ func TestHandleService(t *testing.T) { }, wantWarnings: []string{ fmt.Sprintf(serviceWarning, annotations.AnnotationPrefix+annotations.ConfigurationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey), + kongv1beta1.KongUpstreamPolicyAnnotationKey), }, }, { diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go index afc7490481..9482c78904 100644 --- a/internal/dataplane/kongstate/kongstate.go +++ b/internal/dataplane/kongstate/kongstate.go @@ -225,7 +225,7 @@ func (ks *KongState) FillUpstreamOverrides( logger.Error(nil, fmt.Sprintf( "Service uses deprecated %s annotation and KongIngress, migrate to %s and KongUpstreamPolicy", annotations.AnnotationPrefix+annotations.ConfigurationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey), + kongv1beta1.KongUpstreamPolicyAnnotationKey), "namespace", svc.Namespace, "name", svc.Name) } } @@ -241,9 +241,9 @@ func (ks *KongState) FillUpstreamOverrides( fmt.Sprintf("Service uses both %s and %s annotations, should use only %s annotation. Settings "+ "from %s will take precedence", annotations.AnnotationPrefix+annotations.ConfigurationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey, - annotations.AnnotationPrefix+kongv1beta1.KongUpstreamPolicyAnnotationKey), + kongv1beta1.KongUpstreamPolicyAnnotationKey, + kongv1beta1.KongUpstreamPolicyAnnotationKey, + kongv1beta1.KongUpstreamPolicyAnnotationKey), "namespace", svc.Namespace, "name", svc.Name) } } diff --git a/pkg/apis/configuration/v1/kongingress_types.go b/pkg/apis/configuration/v1/kongingress_types.go index 96820bceb0..3e171b9fc0 100644 --- a/pkg/apis/configuration/v1/kongingress_types.go +++ b/pkg/apis/configuration/v1/kongingress_types.go @@ -28,6 +28,8 @@ import ( // +kubebuilder:storageversion // +kubebuilder:resource:shortName=ki,categories=kong-ingress-controller // +kubebuilder:validation:Optional +// +kubebuilder:validation:XValidation:rule="!has(self.proxy)", message="'proxy' field is no longer supported, use Service's annotations instead" +// +kubebuilder:validation:XValidation:rule="!has(self.route)", message="'route' field is no longer supported, use Ingress' annotations instead" // KongIngress is the Schema for the kongingresses API. type KongIngress struct { @@ -210,9 +212,6 @@ type KongIngressUpstream struct { // HashFallbackURICapture is the "hash_fallback" version of HashOnURICapture. HashFallbackURICapture *string `json:"hash_fallback_uri_capture,omitempty" yaml:"hash_fallback_uri_capture,omitempty"` - - // we need to check this one TODO https://github.com/Kong/kubernetes-ingress-controller/issues/2075 - // ClientCertificate *CertificateSecretRef `json:"client_certificate,omitempty" yaml:"client_certificate,omitempty"` } func init() { diff --git a/test/e2e/manifests/all-in-one-dbless-k4k8s-enterprise.yaml b/test/e2e/manifests/all-in-one-dbless-k4k8s-enterprise.yaml index f2aaa8e8f8..6fa36e055e 100644 --- a/test/e2e/manifests/all-in-one-dbless-k4k8s-enterprise.yaml +++ b/test/e2e/manifests/all-in-one-dbless-k4k8s-enterprise.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/e2e/manifests/all-in-one-dbless-konnect-enterprise.yaml b/test/e2e/manifests/all-in-one-dbless-konnect-enterprise.yaml index 9e1788389d..1e7c8ad69b 100644 --- a/test/e2e/manifests/all-in-one-dbless-konnect-enterprise.yaml +++ b/test/e2e/manifests/all-in-one-dbless-konnect-enterprise.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/e2e/manifests/all-in-one-dbless-konnect.yaml b/test/e2e/manifests/all-in-one-dbless-konnect.yaml index 8750affeb6..61ae35ca70 100644 --- a/test/e2e/manifests/all-in-one-dbless-konnect.yaml +++ b/test/e2e/manifests/all-in-one-dbless-konnect.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/e2e/manifests/all-in-one-dbless.yaml b/test/e2e/manifests/all-in-one-dbless.yaml index 254547d49b..a11a8deb1a 100644 --- a/test/e2e/manifests/all-in-one-dbless.yaml +++ b/test/e2e/manifests/all-in-one-dbless.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/e2e/manifests/all-in-one-postgres-enterprise.yaml b/test/e2e/manifests/all-in-one-postgres-enterprise.yaml index 8e8b97897b..816e184c19 100644 --- a/test/e2e/manifests/all-in-one-postgres-enterprise.yaml +++ b/test/e2e/manifests/all-in-one-postgres-enterprise.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/e2e/manifests/all-in-one-postgres.yaml b/test/e2e/manifests/all-in-one-postgres.yaml index 3c570281d7..55edbb4d51 100644 --- a/test/e2e/manifests/all-in-one-postgres.yaml +++ b/test/e2e/manifests/all-in-one-postgres.yaml @@ -953,6 +953,13 @@ spec: type: integer type: object type: object + x-kubernetes-validations: + - message: '''proxy'' field is no longer supported, use Service''s annotations + instead' + rule: '!has(self.proxy)' + - message: '''route'' field is no longer supported, use Ingress'' annotations + instead' + rule: '!has(self.route)' served: true storage: true subresources: diff --git a/test/envtest/crds_envtest_test.go b/test/envtest/crds_envtest_test.go index 3cf27d44fa..de979919fa 100644 --- a/test/envtest/crds_envtest_test.go +++ b/test/envtest/crds_envtest_test.go @@ -11,6 +11,7 @@ import ( "github.com/go-logr/zapr" "github.com/google/uuid" + kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1" "github.com/samber/lo" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -457,6 +458,28 @@ func TestCRDValidations(t *testing.T) { require.ErrorContains(t, err, `spec.hashOnFallback must not be set when spec.hashOn.cookie is set.`) }, }, + { + name: "KongIngress - proxy is not allowed", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongIngress(ctx, ctrlClient, ns, &kongv1.KongIngress{ + Proxy: &kongv1.KongIngressService{ + Retries: lo.ToPtr(5), + }, + }) + require.ErrorContains(t, err, "'proxy' field is no longer supported, use Service's annotations instead") + }, + }, + { + name: "KongIngress - route is not allowed", + scenario: func(ctx context.Context, t *testing.T, ns string) { + err := createKongIngress(ctx, ctrlClient, ns, &kongv1.KongIngress{ + Route: &kongv1.KongIngressRoute{ + PreserveHost: lo.ToPtr(true), + }, + }) + require.ErrorContains(t, err, "'route' field is no longer supported, use Ingress' annotations instead") + }, + }, } for _, tc := range testCases { @@ -598,3 +621,9 @@ func generateInvalidHashOns() []kongv1beta1.KongUpstreamHash { return fmt.Sprintf("%s.%s.%s.%s", optStr(h.Cookie), optStr(h.Header), optStr(h.URICapture), optStr(h.QueryArg)) }) } + +func createKongIngress(ctx context.Context, client client.Client, ns string, ingress *kongv1.KongIngress) error { + ingress.GenerateName = "test-" + ingress.Namespace = ns + return client.Create(ctx, ingress) +} diff --git a/test/integration/kongingress_webhook_test.go b/test/integration/kongingress_webhook_test.go index 28643ea06e..95e88c77a2 100644 --- a/test/integration/kongingress_webhook_test.go +++ b/test/integration/kongingress_webhook_test.go @@ -55,13 +55,9 @@ func TestKongIngressValidationWebhook(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "kong-ingress-validation-", }, - // Proxy field is deprecated, expecting warning for it. - Proxy: &kongv1.KongIngressService{ - Protocol: lo.ToPtr("tcp"), - }, - // Route field is deprecated, expecting warning for it. - Route: &kongv1.KongIngressRoute{ - Methods: []*string{lo.ToPtr("POST")}, + // Upstream field is deprecated, expecting warning for it. + Upstream: &kongv1.KongIngressUpstream{ + HashOn: lo.ToPtr("none"), }, } @@ -73,10 +69,8 @@ func TestKongIngressValidationWebhook(t *testing.T) { Do(ctx) assert.NoError(t, result.Error()) - require.Len(t, result.Warnings(), 2) expectedWarnings := []string{ - "Support for 'proxy' was removed in 3.0. It will have no effect. Use Service's annotations instead.", - "Support for 'route' was removed in 3.0. It will have no effect. Use Ingress' annotations instead.", + "'upstream' is DEPRECATED. It will have no effect. Use KongUpstreamPolicy instead.", } receivedWarnings := lo.Map(result.Warnings(), func(item net.WarningHeader, index int) string { return item.Text