Handle KongIngress deprecation in admission webhook and in KongIngress validation rules

CHANGELOG.md

@@ -130,6 +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 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.
+  [#5022](https://github.com/Kong/kubernetes-ingress-controller/pull/5022)
 
 ### Fixed
 

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:

go.mod

@@ -82,7 +82,7 @@ require (
 	github.com/moby/sys/sequential v0.5.0 // indirect
 	github.com/morikuni/aec v1.0.0 // indirect
 	github.com/onsi/ginkgo v1.16.4 // indirect
-	github.com/opencontainers/runc v1.1.5 // indirect
+	github.com/opencontainers/runc v1.1.7 // indirect
 	github.com/puzpuzpuz/xsync/v2 v2.5.1 // indirect
 	github.com/sergi/go-diff v1.2.0 // indirect
 	github.com/shoenig/go-m1cpu v0.1.6 // indirect

hack/deploy-admission-controller.sh

@@ -68,6 +68,7 @@ webhooks:
     - UPDATE
     resources:
     - secrets
+    - services
   - apiGroups:
     - networking.k8s.io
     apiVersions:

internal/admission/handler.go

@@ -12,6 +12,7 @@ import (
 	netv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"github.com/kong/kubernetes-ingress-controller/v3/internal/annotations"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
 	kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
@@ -95,6 +96,11 @@ var (
 		Version:  netv1.SchemeGroupVersion.Version,
 		Resource: "ingresses",
 	}
+	serviceGVResource = metav1.GroupVersionResource{
+		Group:    corev1.SchemeGroupVersion.Group,
+		Version:  corev1.SchemeGroupVersion.Version,
+		Resource: "services",
+	}
 )
 
 func (h RequestHandler) handleValidation(ctx context.Context, request admissionv1.AdmissionRequest) (
@@ -119,6 +125,8 @@ func (h RequestHandler) handleValidation(ctx context.Context, request admissionv
 		return h.handleHTTPRoute(ctx, request, responseBuilder)
 	case kongIngressGVResource:
 		return h.handleKongIngress(ctx, request, responseBuilder)
+	case serviceGVResource:
+		return h.handleService(ctx, request, responseBuilder)
 	case ingressGVResource:
 		return h.handleIngress(ctx, request, responseBuilder)
 	default:
@@ -289,6 +297,12 @@ func (h RequestHandler) handleHTTPRoute(
 	return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
 }
 
+const (
+	proxyWarning    = "Support for 'proxy' was removed in 3.0. It will have no effect. Use Service's annotations instead."
+	routeWarning    = "Support for 'route' was removed in 3.0. It will have no effect. Use Ingress' annotations instead."
+	upstreamWarning = "'upstream' is DEPRECATED and will be removed in a future version. Use a KongUpstreamPolicy resource instead."
+)
+
 func (h RequestHandler) handleKongIngress(_ context.Context, request admissionv1.AdmissionRequest, responseBuilder *ResponseBuilder) (*admissionv1.AdmissionResponse, error) {
 	kongIngress := kongv1.KongIngress{}
 	_, _, err := codecs.UniversalDeserializer().Decode(request.Object.Raw, nil, &kongIngress)
@@ -299,13 +313,41 @@ 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 {
-		const warning = "'proxy' is DEPRECATED. It will have no effect. Use Service's annotations instead."
-		responseBuilder = responseBuilder.WithWarning(warning)
+		responseBuilder = responseBuilder.WithWarning(proxyWarning)
 	}
-
 	if kongIngress.Route != nil {
-		const warning = "'route' is DEPRECATED. It will have no effect. Use Ingress' annotations instead."
+		responseBuilder = responseBuilder.WithWarning(routeWarning)
+	}
+
+	if kongIngress.Upstream != nil {
+		responseBuilder = responseBuilder.WithWarning(upstreamWarning)
+	}
+
+	return responseBuilder.Build(), nil
+}
+
+const (
+	serviceWarning = "%s is deprecated and will be removed in a future release. Use Service annotations " +
+		"for the 'proxy' section and %s with a KongUpstreamPolicy resource instead."
+)
+
+func (h RequestHandler) handleService(_ context.Context, request admissionv1.AdmissionRequest, responseBuilder *ResponseBuilder) (*admissionv1.AdmissionResponse, error) {
+	service := corev1.Service{}
+	_, _, err := codecs.UniversalDeserializer().Decode(request.Object.Raw, nil, &service)
+	if err != nil {
+		return nil, err
+	}
+
+	// Service is always allowed.
+	responseBuilder = responseBuilder.Allowed(true)
+
+	if annotations.ExtractConfigurationName(service.Annotations) != "" {
+		warning := fmt.Sprintf(serviceWarning, annotations.AnnotationPrefix+annotations.ConfigurationKey,
+			kongv1beta1.KongUpstreamPolicyAnnotationKey)
+
 		responseBuilder = responseBuilder.WithWarning(warning)
 	}
 

internal/admission/handler_test.go

@@ -0,0 +1,147 @@
+package admission
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"testing"
+
+	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/require"
+	admissionv1 "k8s.io/api/admission/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	k8stypes "k8s.io/apimachinery/pkg/types"
+
+	"github.com/kong/kubernetes-ingress-controller/v3/internal/annotations"
+	kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
+	kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1"
+)
+
+func TestHandleKongIngress(t *testing.T) {
+	tests := []struct {
+		name         string
+		resource     kongv1.KongIngress
+		wantWarnings []string
+	}{
+		{
+			name: "has proxy",
+			resource: kongv1.KongIngress{
+				Proxy: &kongv1.KongIngressService{},
+			},
+			wantWarnings: []string{proxyWarning},
+		},
+		{
+			name: "has route",
+			resource: kongv1.KongIngress{
+				Route: &kongv1.KongIngressRoute{},
+			},
+			wantWarnings: []string{routeWarning},
+		},
+		{
+			name: "has upstream",
+			resource: kongv1.KongIngress{
+				Upstream: &kongv1.KongIngressUpstream{},
+			},
+			wantWarnings: []string{upstreamWarning},
+		},
+		{
+			name: "has everything",
+			resource: kongv1.KongIngress{
+				Proxy:    &kongv1.KongIngressService{},
+				Route:    &kongv1.KongIngressRoute{},
+				Upstream: &kongv1.KongIngressUpstream{},
+			},
+			wantWarnings: []string{proxyWarning, routeWarning, upstreamWarning},
+		},
+	}
+	for _, tt := range tests {
+		resource := tt.resource
+		t.Run(tt.name, func(t *testing.T) {
+			validator := KongHTTPValidator{}
+			raw, err := json.Marshal(tt.resource)
+			require.NoError(t, err)
+			request := admissionv1.AdmissionRequest{
+				Object: runtime.RawExtension{
+					Object: &resource,
+					Raw:    raw,
+				},
+			}
+
+			handler := RequestHandler{
+				Validator: validator,
+				Logger:    logr.Discard(),
+			}
+
+			responseBuilder := NewResponseBuilder(k8stypes.UID(""))
+
+			got, err := handler.handleKongIngress(context.Background(), request, responseBuilder)
+			require.NoError(t, err)
+			require.True(t, got.Allowed)
+			require.Equal(t, tt.wantWarnings, got.Warnings)
+		})
+	}
+}
+
+func TestHandleService(t *testing.T) {
+	tests := []struct {
+		name         string
+		resource     corev1.Service
+		wantWarnings []string
+	}{
+		{
+			name: "has kongingress annotation",
+			resource: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					Annotations: map[string]string{
+						annotations.AnnotationPrefix + annotations.ConfigurationKey: "test",
+					},
+				},
+			},
+			wantWarnings: []string{
+				fmt.Sprintf(serviceWarning, annotations.AnnotationPrefix+annotations.ConfigurationKey,
+					kongv1beta1.KongUpstreamPolicyAnnotationKey),
+			},
+		},
+		{
+			name: "has upstream policy annotation",
+			resource: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					Annotations: map[string]string{
+						annotations.AnnotationPrefix + kongv1beta1.KongUpstreamPolicyAnnotationKey: "test",
+					},
+				},
+			},
+			wantWarnings: nil,
+		},
+	}
+	for _, tt := range tests {
+		resource := tt.resource
+		t.Run(tt.name, func(t *testing.T) {
+			validator := KongHTTPValidator{}
+			raw, err := json.Marshal(tt.resource)
+			require.NoError(t, err)
+			request := admissionv1.AdmissionRequest{
+				Object: runtime.RawExtension{
+					Object: &resource,
+					Raw:    raw,
+				},
+			}
+
+			handler := RequestHandler{
+				Validator: validator,
+				Logger:    logr.Discard(),
+			}
+
+			responseBuilder := NewResponseBuilder(k8stypes.UID(""))
+
+			got, err := handler.handleService(context.Background(), request, responseBuilder)
+			require.NoError(t, err)
+			require.True(t, got.Allowed)
+			require.Equal(t, tt.wantWarnings, got.Warnings)
+		})
+	}
+}

internal/dataplane/kongstate/kongstate.go

@@ -18,6 +18,7 @@ import (
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/failures"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/store"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
+	kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1"
 )
 
 // KongState holds the configuration that should be applied to Kong.
@@ -203,10 +204,14 @@ func (ks *KongState) FillOverrides(
 		}
 	}
 
-	ks.FillUpstreamOverrides(s, failuresCollector)
+	ks.FillUpstreamOverrides(s, logger, failuresCollector)
 }
 
-func (ks *KongState) FillUpstreamOverrides(s store.Storer, failuresCollector *failures.ResourceFailuresCollector) {
+func (ks *KongState) FillUpstreamOverrides(
+	s store.Storer,
+	logger logr.Logger,
+	failuresCollector *failures.ResourceFailuresCollector,
+) {
 	for i := 0; i < len(ks.Upstreams); i++ {
 		servicesGroup := lo.Values(ks.Upstreams[i].Service.K8sServices)
 		servicesAsObjects := func(svc *corev1.Service, _ int) client.Object { return svc }
@@ -217,6 +222,11 @@ func (ks *KongState) FillUpstreamOverrides(s store.Storer, failuresCollector *fa
 		} else {
 			for _, svc := range ks.Upstreams[i].Service.K8sServices {
 				ks.Upstreams[i].override(kongIngress, svc)
+				logger.Error(nil, fmt.Sprintf(
+					"Service uses deprecated %s annotation and KongIngress, migrate to %s and KongUpstreamPolicy",
+					annotations.AnnotationPrefix+annotations.ConfigurationKey,
+					kongv1beta1.KongUpstreamPolicyAnnotationKey),
+					"namespace", svc.Namespace, "name", svc.Name)
 			}
 		}
 
@@ -225,6 +235,18 @@ func (ks *KongState) FillUpstreamOverrides(s store.Storer, failuresCollector *fa
 			failuresCollector.PushResourceFailure(err.Error(), lo.Map(servicesGroup, servicesAsObjects)...)
 		} else {
 			if kongUpstreamPolicy != nil {
+				if kongIngress != nil {
+					for _, svc := range servicesGroup {
+						logger.Error(nil,
+							fmt.Sprintf("Service uses both %s and %s annotations, should use only %s annotation. Settings "+
+								"from %s will take precedence",
+								annotations.AnnotationPrefix+annotations.ConfigurationKey,
+								kongv1beta1.KongUpstreamPolicyAnnotationKey,
+								kongv1beta1.KongUpstreamPolicyAnnotationKey,
+								kongv1beta1.KongUpstreamPolicyAnnotationKey),
+							"namespace", svc.Namespace, "name", svc.Name)
+					}
+				}
 				ks.Upstreams[i].overrideByKongUpstreamPolicy(kongUpstreamPolicy)
 			}
 		}

internal/dataplane/kongstate/kongstate_test.go

@@ -1134,7 +1134,7 @@ func TestKongState_FillUpstreamOverrides(t *testing.T) {
 			failuresCollector := failures.NewResourceFailuresCollector(logr.Discard())
 
 			kongState := KongState{Upstreams: []Upstream{tc.upstream}}
-			kongState.FillUpstreamOverrides(s, failuresCollector)
+			kongState.FillUpstreamOverrides(s, logr.Discard(), failuresCollector)
 			require.Equal(t, tc.expectedUpstream, kongState.Upstreams[0].Upstream)
 			require.ElementsMatch(t, tc.expectedFailures, failuresCollector.PopResourceFailures())
 		})

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() {