Skip to content

Commit

Permalink
Handle KongIngress deprecation in admission webhook and in `KongIng…
Browse files Browse the repository at this point in the history
…ress` validation rules (#5022)

Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
  • Loading branch information
3 people committed Oct 31, 2023
1 parent 0eb998f commit 2fccacb
Show file tree
Hide file tree
Showing 18 changed files with 311 additions and 55 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions config/crd/bases/configuration.konghq.com_kongingresses.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 2 additions & 34 deletions go.sum

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions hack/deploy-admission-controller.sh
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ webhooks:
- UPDATE
resources:
- secrets
- services
- apiGroups:
- networking.k8s.io
apiVersions:
Expand Down
50 changes: 46 additions & 4 deletions internal/admission/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) (
Expand All @@ -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:
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}

Expand Down
147 changes: 147 additions & 0 deletions internal/admission/handler_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
26 changes: 24 additions & 2 deletions internal/dataplane/kongstate/kongstate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 }
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/dataplane/kongstate/kongstate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
Expand Down
5 changes: 2 additions & 3 deletions pkg/apis/configuration/v1/kongingress_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/manifests/all-in-one-dbless-k4k8s-enterprise.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions test/e2e/manifests/all-in-one-dbless-konnect-enterprise.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 2fccacb

Please sign in to comment.