diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cc0adba4c..3677d7a3d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,12 +104,19 @@ Adding a new version? You'll need three changes: - Added support for GRPC over HTTP (without TLS) in Gateway API. [#5128](https://github.com/Kong/kubernetes-ingress-controller/pull/5128) [#5283](https://github.com/Kong/kubernetes-ingress-controller/pull/5283) -- Added `-init-cache-sync-duration` CLI flag. This flag configures how long the controller waits for Kubernetes resources to populate at startup before generating the initial Kong configuration. It also fixes a bug that removed the default 5 second wait period. +- Added `--init-cache-sync-duration` CLI flag. This flag configures how long the + controller waits for Kubernetes resources to populate at startup before + generating the initial Kong configuration. It also fixes a bug that removed + the default 5 second wait period. [#5238](https://github.com/Kong/kubernetes-ingress-controller/pull/5238) - Added `--emit-kubernetes-events` CLI flag to disable the creation of events in translating and applying configurations to Kong. [#5296](https://github.com/Kong/kubernetes-ingress-controller/pull/5296) [#5299](https://github.com/Kong/kubernetes-ingress-controller/pull/5299) + - Added validation on `Secret`s to reject the change if it will generate + invalid confiugration of plugins for `KongPlugin`s or `KongClusterPlugin`s + referencing to the secret. + [#5203](https://github.com/Kong/kubernetes-ingress-controller/pull/5203) ### Fixed diff --git a/internal/admission/handler.go b/internal/admission/handler.go index 017f40af22..bb1b6b35ae 100644 --- a/internal/admission/handler.go +++ b/internal/admission/handler.go @@ -13,18 +13,28 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kong/kubernetes-ingress-controller/v3/internal/annotations" + ctrlref "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/reference" "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" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1" ) +const ( + KindKongPlugin = "KongPlugin" + KindKongClusterPlugin = "KongClusterPlugin" +) + // RequestHandler is an HTTP server that can validate Kong Ingress Controllers' // Custom Resources using Kubernetes Admission Webhooks. type RequestHandler struct { // Validator validates the entities that the k8s API-server asks // it the server to validate. Validator KongValidator + // ReferenceIndexers gets the resources (KongPlugin and KongClusterPlugin) + // referring the validated resource (Secret) to check the changes on + // referred Secret will produce invalid configuration of the plugins. + ReferenceIndexers ctrlref.CacheIndexers Logger logr.Logger } @@ -203,7 +213,7 @@ func (h RequestHandler) handleKongPlugin( return nil, err } - ok, message, err := h.Validator.ValidatePlugin(ctx, plugin) + ok, message, err := h.Validator.ValidatePlugin(ctx, plugin, nil) if err != nil { return nil, err } @@ -222,7 +232,7 @@ func (h RequestHandler) handleKongClusterPlugin( return nil, err } - ok, message, err := h.Validator.ValidateClusterPlugin(ctx, plugin) + ok, message, err := h.Validator.ValidateClusterPlugin(ctx, plugin, nil) if err != nil { return nil, err } @@ -240,24 +250,77 @@ func (h RequestHandler) handleSecret( if err != nil { return nil, err } - // TODO so long as we still handle the deprecated field, this has to remain - // once the deprecated field is removed, we must replace this with a label filter in the webhook itself - // https://github.com/Kong/kubernetes-ingress-controller/issues/4853 - - if _, credentialTypeSource := util.ExtractKongCredentialType(&secret); credentialTypeSource == util.CredentialTypeAbsent { - // secret does not look like a credential resource in Kong - return responseBuilder.Allowed(true).Build(), nil - } switch request.Operation { case admissionv1.Update, admissionv1.Create: - ok, message := h.Validator.ValidateCredential(ctx, secret) - return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil + // TODO so long as we still handle the deprecated field, this has to remain + // once the deprecated field is removed, we must replace this with a label filter in the webhook itself + // https://github.com/Kong/kubernetes-ingress-controller/issues/4853 + if _, credentialTypeSource := util.ExtractKongCredentialType(&secret); credentialTypeSource != util.CredentialTypeAbsent { + ok, message := h.Validator.ValidateCredential(ctx, secret) + if !ok { + return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil + } + } + + ok, message, err := h.checkReferrersOfSecret(ctx, &secret) + if err != nil { + return responseBuilder.Allowed(false).WithMessage(fmt.Sprintf("failed to validate other objects referencing the secret: %v", err)).Build(), err + } + if !ok { + return responseBuilder.Allowed(false).WithMessage(message).Build(), nil + } + + return responseBuilder.Allowed(true).Build(), nil + default: return nil, fmt.Errorf("unknown operation %q", string(request.Operation)) } } +// checkReferrersOfSecret validates all referrers (KongPlugins and KongClusterPlugins) of the secret +// and rejects the secret if it generates invalid configurations for any of the referrers. +func (h RequestHandler) checkReferrersOfSecret(ctx context.Context, secret *corev1.Secret) (bool, string, error) { + referrers, err := h.ReferenceIndexers.ListReferrerObjectsByReferent(secret) + if err != nil { + return false, "", fmt.Errorf("failed to list referrers of secret: %w", err) + } + + for _, obj := range referrers { + gvk := obj.GetObjectKind().GroupVersionKind() + if gvk.Group == kongv1.GroupVersion.Group && gvk.Version == kongv1.GroupVersion.Version && gvk.Kind == KindKongPlugin { + plugin := obj.(*kongv1.KongPlugin) + ok, message, err := h.Validator.ValidatePlugin(ctx, *plugin, []*corev1.Secret{secret}) + if err != nil { + return false, "", fmt.Errorf("failed to run validation on KongPlugin %s/%s: %w", + plugin.Namespace, plugin.Name, err, + ) + } + if !ok { + return false, + fmt.Sprintf("Change on secret will generate invalid configuration for KongPlugin %s/%s: %s", + plugin.Namespace, plugin.Name, message, + ), nil + } + } + if gvk.Group == kongv1.GroupVersion.Group && gvk.Version == kongv1.GroupVersion.Version && gvk.Kind == KindKongClusterPlugin { + plugin := obj.(*kongv1.KongClusterPlugin) + ok, message, err := h.Validator.ValidateClusterPlugin(ctx, *plugin, []*corev1.Secret{secret}) + if err != nil { + return false, "", fmt.Errorf("failed to run validation on KongClusterPlugin %s: %w", + plugin.Name, err, + ) + } + if !ok { + return false, fmt.Sprintf("Change on secret will generate invalid configuration for KongClusterPlugin %s: %s", + plugin.Name, message, + ), nil + } + } + } + return true, "", nil +} + func (h RequestHandler) handleGateway( ctx context.Context, request admissionv1.AdmissionRequest, diff --git a/internal/admission/handler_test.go b/internal/admission/handler_test.go index c99d8e6583..83caf9478b 100644 --- a/internal/admission/handler_test.go +++ b/internal/admission/handler_test.go @@ -4,21 +4,41 @@ import ( "context" "encoding/json" "fmt" + "net/http" "testing" "github.com/go-logr/logr" + "github.com/go-logr/logr/testr" "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" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kong/kubernetes-ingress-controller/v3/internal/annotations" + ctrlref "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/reference" + "github.com/kong/kubernetes-ingress-controller/v3/internal/util" kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1" ) +var ( + secretTypeMeta = metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Secret", + } + kongPluginTypeMeta = metav1.TypeMeta{ + APIVersion: kongv1.GroupVersion.String(), + Kind: "KongPlugin", + } + kongClusterPluginTypeMeta = metav1.TypeMeta{ + APIVersion: kongv1.GroupVersion.String(), + Kind: "KongClusterPlugin", + } +) + func TestHandleKongIngress(t *testing.T) { tests := []struct { name string @@ -130,7 +150,6 @@ func TestHandleService(t *testing.T) { Raw: raw, }, } - handler := RequestHandler{ Validator: validator, Logger: logr.Discard(), @@ -145,3 +164,178 @@ func TestHandleService(t *testing.T) { }) } } + +func TestHandleSecret(t *testing.T) { + testCases := []struct { + name string + secret *corev1.Secret + referrers []client.Object + validatorOK bool + validatorMessage string + validatorError error + expectAllowed bool + expectStatusCode int32 + expectMessage string + expectError bool + }{ + { + name: "secret used as a credential and passes the validation of credential", + secret: &corev1.Secret{ + TypeMeta: secretTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "credential-0", + Labels: map[string]string{ + "konghq.com/credential": "true", + }, + }, + Data: map[string][]byte{}, + }, + validatorOK: true, + expectAllowed: true, + }, + { + name: "secret used as credential and fails the validation of credential", + secret: &corev1.Secret{ + TypeMeta: secretTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "credential-1", + Labels: map[string]string{ + "konghq.com/credential": "true", + }, + }, + Data: map[string][]byte{}, + }, + validatorOK: false, + validatorMessage: "invalid credential", + expectAllowed: false, + expectStatusCode: http.StatusBadRequest, + expectMessage: "invalid credential", + }, + { + name: "secret used as KongPlugin config and KongClusterPlugin and passes validation of both CRDs", + secret: &corev1.Secret{ + TypeMeta: secretTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "plugin-conf", + }, + }, + referrers: []client.Object{ + &kongv1.KongPlugin{ + TypeMeta: kongPluginTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "plugin-0", + }, + PluginName: "test-plugin", + }, + &kongv1.KongClusterPlugin{ + TypeMeta: kongClusterPluginTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-plugin-0", + }, + PluginName: "test-plugin", + }, + }, + validatorOK: true, + expectAllowed: true, + }, + { + name: "secret used as KongPlugin config and fails validation of KongPlugin", + secret: &corev1.Secret{ + TypeMeta: secretTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "plugin-conf", + }, + }, + referrers: []client.Object{ + &kongv1.KongPlugin{ + TypeMeta: kongPluginTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "plugin-0", + }, + PluginName: "test-plugin", + }, + }, + validatorOK: false, + validatorMessage: "invalid KongPlugin", + expectAllowed: false, + expectStatusCode: http.StatusBadRequest, + expectMessage: "Change on secret will generate invalid configuration for KongPlugin default/plugin-0: invalid KongPlugin", + }, + { + name: "secret used as KongClusterPlugin config and fails validation of KongClusterPlugin", + secret: &corev1.Secret{ + TypeMeta: secretTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "plugin-conf", + }, + }, + referrers: []client.Object{ + &kongv1.KongClusterPlugin{ + TypeMeta: kongClusterPluginTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-plugin-0", + }, + PluginName: "test-plugin", + }, + }, + validatorOK: false, + validatorMessage: "invalid KongClusterPlugin", + expectAllowed: false, + expectStatusCode: http.StatusBadRequest, + expectMessage: "Change on secret will generate invalid configuration for KongClusterPlugin cluster-plugin-0: invalid KongClusterPlugin", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + validator := KongFakeValidator{ + Result: tc.validatorOK, + Message: tc.validatorMessage, + Error: tc.validatorError, + } + raw, err := json.Marshal(tc.secret) + require.NoError(t, err) + request := admissionv1.AdmissionRequest{ + Object: runtime.RawExtension{ + Object: tc.secret, + Raw: raw, + }, + Operation: admissionv1.Update, + } + + logger := testr.NewWithOptions(t, testr.Options{Verbosity: util.DebugLevel}) + referenceIndexer := ctrlref.NewCacheIndexers(logger) + + handler := RequestHandler{ + Validator: validator, + Logger: logger, + ReferenceIndexers: referenceIndexer, + } + for _, obj := range tc.referrers { + err := handler.ReferenceIndexers.SetObjectReference(obj, tc.secret) + require.NoError(t, err) + } + + responseBuilder := NewResponseBuilder(k8stypes.UID("")) + got, err := handler.handleSecret(context.Background(), request, responseBuilder) + if tc.expectError { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.expectAllowed, got.Allowed, "should return expected result of allowed") + require.Equal(t, tc.expectStatusCode, got.Result.Code) + if len(tc.expectMessage) > 0 { + require.Contains(t, got.Result.Message, tc.expectMessage) + } + }) + } +} diff --git a/internal/admission/server_test.go b/internal/admission/server_test.go index 97e776a59c..f927ffce8f 100644 --- a/internal/admission/server_test.go +++ b/internal/admission/server_test.go @@ -48,6 +48,7 @@ func (v KongFakeValidator) ValidateConsumerGroup( func (v KongFakeValidator) ValidatePlugin( _ context.Context, _ kongv1.KongPlugin, + _ []*corev1.Secret, ) (bool, string, error) { return v.Result, v.Message, v.Error } @@ -55,6 +56,7 @@ func (v KongFakeValidator) ValidatePlugin( func (v KongFakeValidator) ValidateClusterPlugin( _ context.Context, _ kongv1.KongClusterPlugin, + _ []*corev1.Secret, ) (bool, string, error) { return v.Result, v.Message, v.Error } diff --git a/internal/admission/validator.go b/internal/admission/validator.go index 2726e26f71..4c6bcb5b37 100644 --- a/internal/admission/validator.go +++ b/internal/admission/validator.go @@ -7,10 +7,12 @@ import ( "github.com/go-logr/logr" "github.com/kong/go-kong/kong" + "github.com/samber/lo" corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" credsvalidation "github.com/kong/kubernetes-ingress-controller/v3/internal/admission/validation/consumers/credentials" @@ -31,8 +33,8 @@ import ( type KongValidator interface { ValidateConsumer(ctx context.Context, consumer kongv1.KongConsumer) (bool, string, error) ValidateConsumerGroup(ctx context.Context, consumerGroup kongv1beta1.KongConsumerGroup) (bool, string, error) - ValidatePlugin(ctx context.Context, plugin kongv1.KongPlugin) (bool, string, error) - ValidateClusterPlugin(ctx context.Context, plugin kongv1.KongClusterPlugin) (bool, string, error) + ValidatePlugin(ctx context.Context, plugin kongv1.KongPlugin, overrideSecrets []*corev1.Secret) (bool, string, error) + ValidateClusterPlugin(ctx context.Context, plugin kongv1.KongClusterPlugin, overrideSecrets []*corev1.Secret) (bool, string, error) ValidateCredential(ctx context.Context, secret corev1.Secret) (bool, string) ValidateGateway(ctx context.Context, gateway gatewayapi.Gateway) (bool, string, error) ValidateHTTPRoute(ctx context.Context, httproute gatewayapi.HTTPRoute) (bool, string, error) @@ -54,6 +56,43 @@ type ConsumerGetter interface { ListAllConsumers(ctx context.Context) ([]kongv1.KongConsumer, error) } +// SecretGetterWithOverride returns the override secrets in the list if the namespace and name matches, +// or use the nested secretGetter to fetch the secret otherwise. +// Used for validating changes of secrets to override existing the one in cache with the one to be updated. +type SecretGetterWithOverride struct { + overrideSecrets map[k8stypes.NamespacedName]*corev1.Secret + secretGetter kongstate.SecretGetter +} + +var _ kongstate.SecretGetter = &SecretGetterWithOverride{} + +func (s *SecretGetterWithOverride) GetSecret(namespace, name string) (*corev1.Secret, error) { + nsName := k8stypes.NamespacedName{ + Namespace: namespace, + Name: name, + } + overrideSecret, ok := s.overrideSecrets[nsName] + if ok { + return overrideSecret, nil + } + + return s.secretGetter.GetSecret(namespace, name) +} + +// NewSecretGetterWithOverride returns a secret getter with given override secrets. +func NewSecretGetterWithOverride(s kongstate.SecretGetter, overrideSecrets []*corev1.Secret) *SecretGetterWithOverride { + overrideSecretMap := lo.SliceToMap(overrideSecrets, func(secret *corev1.Secret) (k8stypes.NamespacedName, *corev1.Secret) { + return k8stypes.NamespacedName{ + Namespace: secret.Namespace, + Name: secret.Name, + }, secret.DeepCopy() + }) + return &SecretGetterWithOverride{ + overrideSecrets: overrideSecretMap, + secretGetter: s, + } +} + // KongHTTPValidator implements KongValidator interface to validate Kong // entities using the Admin API of Kong. type KongHTTPValidator struct { @@ -284,13 +323,16 @@ func (validator KongHTTPValidator) ValidateCredential(ctx context.Context, secre func (validator KongHTTPValidator) ValidatePlugin( ctx context.Context, k8sPlugin kongv1.KongPlugin, + overrideSecrets []*corev1.Secret, ) (bool, string, error) { var plugin kong.Plugin plugin.Name = kong.String(k8sPlugin.PluginName) var err error + secretGetter := NewSecretGetterWithOverride(validator.SecretGetter, overrideSecrets) + plugin.Config, err = kongstate.RawConfigurationWithPatchesToConfiguration( - validator.SecretGetter, + secretGetter, k8sPlugin.Namespace, k8sPlugin.Config, k8sPlugin.ConfigPatches, @@ -299,7 +341,7 @@ func (validator KongHTTPValidator) ValidatePlugin( return false, fmt.Sprintf("%s: %s", ErrTextPluginConfigInvalid, err), nil } if k8sPlugin.ConfigFrom != nil { - config, err := kongstate.SecretToConfiguration(validator.SecretGetter, (*k8sPlugin.ConfigFrom).SecretValue, k8sPlugin.Namespace) + config, err := kongstate.SecretToConfiguration(secretGetter, (*k8sPlugin.ConfigFrom).SecretValue, k8sPlugin.Namespace) if err != nil { return false, fmt.Sprintf("%s: %s", ErrTextPluginSecretConfigUnretrievable, err), nil } @@ -328,13 +370,15 @@ func (validator KongHTTPValidator) ValidatePlugin( func (validator KongHTTPValidator) ValidateClusterPlugin( ctx context.Context, k8sPlugin kongv1.KongClusterPlugin, + overrideSecrets []*corev1.Secret, ) (bool, string, error) { var plugin kong.Plugin plugin.Name = kong.String(k8sPlugin.PluginName) var err error + secretGetter := NewSecretGetterWithOverride(validator.SecretGetter, overrideSecrets) plugin.Config, err = kongstate.RawConfigurationWithNamespacedPatchesToConfiguration( - validator.SecretGetter, + secretGetter, k8sPlugin.Config, k8sPlugin.ConfigPatches, ) @@ -343,11 +387,16 @@ func (validator KongHTTPValidator) ValidateClusterPlugin( } if k8sPlugin.ConfigFrom != nil { - config, err := kongstate.NamespacedSecretToConfiguration(validator.SecretGetter, k8sPlugin.ConfigFrom.SecretValue) + config, err := kongstate.NamespacedSecretToConfiguration(secretGetter, k8sPlugin.ConfigFrom.SecretValue) if err != nil { return false, fmt.Sprintf("%s: %s", ErrTextPluginSecretConfigUnretrievable, err), nil } plugin.Config = config + validator.Logger.V(util.DebugLevel).Info("Got config from secret", + "secret", k8sPlugin.ConfigFrom.SecretValue.Namespace+"/"+k8sPlugin.ConfigFrom.SecretValue.Secret, + "key", k8sPlugin.ConfigFrom.SecretValue.Key, + "config", config, + ) } if k8sPlugin.RunOn != "" { plugin.RunOn = kong.String(k8sPlugin.RunOn) diff --git a/internal/admission/validator_test.go b/internal/admission/validator_test.go index 389576255d..93ca1175d2 100644 --- a/internal/admission/validator_test.go +++ b/internal/admission/validator_test.go @@ -114,7 +114,8 @@ func TestKongHTTPValidator_ValidatePlugin(t *testing.T) { }, }) type args struct { - plugin kongv1.KongPlugin + plugin kongv1.KongPlugin + overrideSecrets []*corev1.Secret } tests := []struct { name string @@ -239,6 +240,34 @@ func TestKongHTTPValidator_ValidatePlugin(t *testing.T) { wantMessage: ErrTextPluginConfigValidationFailed, wantErr: true, }, + { + name: "validate from override secret which generates valid configuration", + PluginSvc: &fakePluginSvc{valid: true}, + args: args{ + plugin: kongv1.KongPlugin{ + PluginName: "key-auth", + ConfigFrom: &kongv1.ConfigSource{ + SecretValue: kongv1.SecretValueFromSource{ + Key: "valid-conf", + Secret: "another-conf-secret", + }, + }, + }, + overrideSecrets: []*corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "", + Name: "another-conf-secret", + }, + Data: map[string][]byte{ + "valid-conf": []byte(`{"foo":"bar"}`), + "invalid-conf": []byte(`{"foo":"baz}`), + }, + }, + }, + }, + wantOK: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -249,7 +278,7 @@ func TestKongHTTPValidator_ValidatePlugin(t *testing.T) { }, ingressClassMatcher: fakeClassMatcher, } - gotOK, gotMessage, err := validator.ValidatePlugin(context.Background(), tt.args.plugin) + gotOK, gotMessage, err := validator.ValidatePlugin(context.Background(), tt.args.plugin, tt.args.overrideSecrets) assert.Equalf(t, tt.wantOK, gotOK, "KongHTTPValidator.ValidatePlugin() want OK: %v, got OK: %v", tt.wantOK, gotOK, @@ -284,7 +313,8 @@ func TestKongHTTPValidator_ValidateClusterPlugin(t *testing.T) { }, }) type args struct { - plugin kongv1.KongClusterPlugin + plugin kongv1.KongClusterPlugin + overrideSecrets []*corev1.Secret } tests := []struct { name string @@ -412,6 +442,35 @@ func TestKongHTTPValidator_ValidateClusterPlugin(t *testing.T) { wantMessage: ErrTextPluginConfigValidationFailed, wantErr: true, }, + { + name: "validate from override secret which generates valid configuration", + PluginSvc: &fakePluginSvc{valid: true}, + args: args{ + plugin: kongv1.KongClusterPlugin{ + PluginName: "key-auth", + ConfigFrom: &kongv1.NamespacedConfigSource{ + SecretValue: kongv1.NamespacedSecretValueFromSource{ + Namespace: "default", + Key: "valid-conf", + Secret: "another-conf-secret", + }, + }, + }, + overrideSecrets: []*corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "another-conf-secret", + }, + Data: map[string][]byte{ + "valid-conf": []byte(`{"foo":"bar"}`), + "invalid-conf": []byte(`{"foo":"baz}`), + }, + }, + }, + }, + wantOK: true, + }, { name: "no gateway was available at the time of validation", PluginSvc: nil, // no plugin service is available as there's no gateways @@ -433,7 +492,7 @@ func TestKongHTTPValidator_ValidateClusterPlugin(t *testing.T) { ingressClassMatcher: fakeClassMatcher, } - gotOK, gotMessage, err := validator.ValidateClusterPlugin(context.Background(), tt.args.plugin) + gotOK, gotMessage, err := validator.ValidateClusterPlugin(context.Background(), tt.args.plugin, tt.args.overrideSecrets) assert.Equalf(t, tt.wantOK, gotOK, "KongHTTPValidator.ValidateClusterPlugin() want OK: %v, got OK: %v", tt.wantOK, gotOK, diff --git a/internal/controllers/configuration/object_references.go b/internal/controllers/configuration/object_references.go index 4ce5d33680..9d188e11ec 100644 --- a/internal/controllers/configuration/object_references.go +++ b/internal/controllers/configuration/object_references.go @@ -103,7 +103,7 @@ func listKongPluginReferredSecrets(plugin *kongv1.KongPlugin) []k8stypes.Namespa } func listKongClusterPluginReferredSecrets(plugin *kongv1.KongClusterPlugin) []k8stypes.NamespacedName { - referredSecretNames := make([]k8stypes.NamespacedName, 0, 1) + referredSecretNames := make([]k8stypes.NamespacedName, 0, len(plugin.ConfigPatches)+1) if plugin.ConfigFrom != nil { nsName := k8stypes.NamespacedName{ Namespace: plugin.ConfigFrom.SecretValue.Namespace, @@ -114,7 +114,7 @@ func listKongClusterPluginReferredSecrets(plugin *kongv1.KongClusterPlugin) []k8 for _, patch := range plugin.ConfigPatches { nsName := k8stypes.NamespacedName{ - Namespace: plugin.Namespace, + Namespace: patch.ValueFrom.SecretValue.Namespace, Name: patch.ValueFrom.SecretValue.Secret, } referredSecretNames = append(referredSecretNames, nsName) diff --git a/internal/controllers/reference/indexer.go b/internal/controllers/reference/indexer.go index 3245c94ae2..1bdbdc667c 100644 --- a/internal/controllers/reference/indexer.go +++ b/internal/controllers/reference/indexer.go @@ -192,6 +192,27 @@ func (c CacheIndexers) ListReferencesByReferrer(referrer client.Object) ([]*Obje return returnRefList, nil } +// ListReferencesByReferent lists all reference records referring to the same referent. +func (c CacheIndexers) ListReferencesByReferent(referent client.Object) ([]*ObjectReference, error) { + referentKey, err := objectKeyFunc(referent) + if err != nil { + return nil, err + } + refList, err := c.indexer.ByIndex(IndexNameReferent, referentKey) + if err != nil { + return nil, err + } + returnRefList := make([]*ObjectReference, 0, len(refList)) + for _, ref := range refList { + retRef, ok := ref.(*ObjectReference) + if !ok { + return nil, ErrTypeNotObjectReference + } + returnRefList = append(returnRefList, retRef) + } + return returnRefList, nil +} + // DeleteReferencesByReferrer deletes all reference records where referrer has the same key // (GroupVersionKind+NamespacedName, that means the same k8s object). // called when a k8s object deleted in cluster, or when we do not care about it anymore. @@ -245,3 +266,16 @@ func (c CacheIndexers) ListReferredObjects(referrer client.Object) ([]client.Obj } return objs, nil } + +// ListReferrerObjectsByReferent lists all objects that refers to the same referent. +func (c CacheIndexers) ListReferrerObjectsByReferent(referent client.Object) ([]client.Object, error) { + refs, err := c.ListReferencesByReferent(referent) + if err != nil { + return nil, err + } + objs := []client.Object{} + for _, ref := range refs { + objs = append(objs, ref.Referrer) + } + return objs, nil +} diff --git a/internal/controllers/reference/indexer_test.go b/internal/controllers/reference/indexer_test.go index 1c14472142..ca817d41a6 100644 --- a/internal/controllers/reference/indexer_test.go +++ b/internal/controllers/reference/indexer_test.go @@ -262,3 +262,41 @@ func TestDeleteReferencesByReferrer(t *testing.T) { }) } } + +func TestListReferrerObjectsByReferent(t *testing.T) { + testCases := []struct { + name string + addReferrer client.Object + addReferent client.Object + checkReferent client.Object + objectNum int + }{ + { + name: "has_referring_objects", + addReferrer: testRefService1, + addReferent: testRefSecret1, + checkReferent: testRefSecret1, + objectNum: 1, + }, + { + name: "has_no_referring_objects", + addReferrer: testRefService1, + addReferent: testRefSecret1, + checkReferent: testRefSecret2, + objectNum: 0, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + c := NewCacheIndexers(logr.Discard()) + err := c.SetObjectReference(tc.addReferrer, tc.addReferent) + require.NoError(t, err, "should not return error on setting reference") + + referrers, err := c.ListReferrerObjectsByReferent(tc.checkReferent) + require.NoError(t, err) + require.Len(t, referrers, tc.objectNum) + }) + } +} diff --git a/internal/manager/controllerdef.go b/internal/manager/controllerdef.go index c2a5b9c8c7..ebb0fa881c 100644 --- a/internal/manager/controllerdef.go +++ b/internal/manager/controllerdef.go @@ -58,6 +58,7 @@ func setupControllers( ctx context.Context, mgr manager.Manager, dataplaneClient controllers.DataPlane, + referenceIndexers ctrlref.CacheIndexers, dataplaneAddressFinder *dataplane.AddressFinder, udpDataplaneAddressFinder *dataplane.AddressFinder, kubernetesStatusQueue *status.Queue, @@ -66,8 +67,6 @@ func setupControllers( kongAdminAPIEndpointsNotifier configuration.EndpointsNotifier, adminAPIsDiscoverer configuration.AdminAPIsDiscoverer, ) []ControllerDef { - referenceIndexers := ctrlref.NewCacheIndexers(ctrl.LoggerFrom(ctx).WithName("controllers").WithName("reference-indexers")) - controllers := []ControllerDef{ // --------------------------------------------------------------------------- // Kong Gateway Admin API Service discovery diff --git a/internal/manager/run.go b/internal/manager/run.go index c17c475a7e..c76c72c99b 100644 --- a/internal/manager/run.go +++ b/internal/manager/run.go @@ -22,6 +22,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/internal/adminapi" "github.com/kong/kubernetes-ingress-controller/v3/internal/clients" "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/gateway" + ctrlref "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/reference" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane" dpconf "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/config" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/configfetcher" @@ -174,6 +175,7 @@ func Run( c.UpdateStatus, ) + referenceIndexers := ctrlref.NewCacheIndexers(setupLog.WithName("reference-indexers")) cache := store.NewCacheStores() storer := store.New(cache, c.IngressClassName, logger) configTranslator, err := translator.NewTranslator( @@ -186,7 +188,7 @@ func Run( } setupLog.Info("Starting Admission Server") - if err := setupAdmissionServer(ctx, c, clientsManager, mgr.GetClient(), logger, translatorFeatureFlags, storer); err != nil { + if err := setupAdmissionServer(ctx, c, clientsManager, referenceIndexers, mgr.GetClient(), logger, translatorFeatureFlags, storer); err != nil { return err } @@ -237,6 +239,7 @@ func Run( ctx, mgr, dataplaneClient, + referenceIndexers, dataplaneAddressFinder, udpDataplaneAddressFinder, kubernetesStatusQueue, diff --git a/internal/manager/setup.go b/internal/manager/setup.go index cd61d69f80..ed31bd9e6e 100644 --- a/internal/manager/setup.go +++ b/internal/manager/setup.go @@ -25,6 +25,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/internal/adminapi" "github.com/kong/kubernetes-ingress-controller/v3/internal/admission" "github.com/kong/kubernetes-ingress-controller/v3/internal/clients" + ctrlref "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/reference" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane" dpconf "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/config" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/translator" @@ -170,6 +171,7 @@ func setupAdmissionServer( ctx context.Context, managerConfig *Config, clientsManager *clients.AdminAPIClientsManager, + referenceIndexers ctrlref.CacheIndexers, managerClient client.Client, logger logr.Logger, translatorFeatures translator.FeatureFlags, @@ -192,7 +194,8 @@ func setupAdmissionServer( translatorFeatures, storer, ), - Logger: admissionLogger, + ReferenceIndexers: referenceIndexers, + Logger: admissionLogger, }, admissionLogger) if err != nil { return err diff --git a/test/integration/webhook_test.go b/test/integration/webhook_test.go index cbf46f1360..f2d86473ca 100644 --- a/test/integration/webhook_test.go +++ b/test/integration/webhook_test.go @@ -19,6 +19,7 @@ import ( admregv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -539,6 +540,361 @@ func TestValidationWebhook(t *testing.T) { } _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, invalidJWT, metav1.CreateOptions{}) require.ErrorContains(t, err, "some fields were invalid due to missing data: rsa_public_key, key, secret") + + t.Log("verifying that the validation fails when secret generates invalid plugin configuration for KongPlugin") + for _, tt := range []struct { + name string + KongPlugin *kongv1.KongPlugin + secretBefore *corev1.Secret + secretAfter *corev1.Secret + errorOnUpdate bool + errorContains string + }{ + { + name: "should fail the validation if secret used in ConfigFrom of KongPlugin generates invalid plugin configuration", + KongPlugin: &kongv1.KongPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "rate-limiting-invalid-config-from", + }, + PluginName: "rate-limiting", + ConfigFrom: &kongv1.ConfigSource{ + SecretValue: kongv1.SecretValueFromSource{ + Secret: "conf-secret-invalid-config", + Key: "rate-limiting-config", + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-invalid-config", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":5}`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-invalid-config", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":"5"}`), + }, + }, + errorOnUpdate: true, + errorContains: "Change on secret will generate invalid configuration for KongPlugin", + }, + { + name: "should fail the validation if the secret is used in ConfigPatches of KongPlugin and generates invalid config", + KongPlugin: &kongv1.KongPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "rate-limiting-invalid-config-patches", + }, + PluginName: "rate-limiting", + Config: apiextensionsv1.JSON{ + Raw: []byte(`{"limit_by":"consumer","policy":"local"}`), + }, + ConfigPatches: []kongv1.ConfigPatch{ + { + Path: "/minute", + ValueFrom: kongv1.ConfigSource{ + SecretValue: kongv1.SecretValueFromSource{ + Secret: "conf-secret-invalid-field", + Key: "rate-limiting-config-minutes", + }, + }, + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-invalid-field", + }, + Data: map[string][]byte{ + "rate-limiting-config-minutes": []byte("10"), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-invalid-field", + }, + Data: map[string][]byte{ + "rate-limiting-config-minutes": []byte(`"10"`), + }, + }, + errorOnUpdate: true, + errorContains: "Change on secret will generate invalid configuration for KongPlugin", + }, + { + name: "should pass the validation if the secret used in ConfigPatches of KongPlugin and generates valid config", + KongPlugin: &kongv1.KongPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "rate-limiting-valid-config", + }, + PluginName: "rate-limiting", + Config: apiextensionsv1.JSON{ + Raw: []byte(`{"limit_by":"consumer","policy":"local"}`), + }, + ConfigPatches: []kongv1.ConfigPatch{ + { + Path: "/minute", + ValueFrom: kongv1.ConfigSource{ + SecretValue: kongv1.SecretValueFromSource{ + Secret: "conf-secret-valid-field", + Key: "rate-limiting-config-minutes", + }, + }, + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-valid-field", + }, + Data: map[string][]byte{ + "rate-limiting-config-minutes": []byte(`10`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "conf-secret-valid-field", + }, + Data: map[string][]byte{ + "rate-limiting-config-minutes": []byte(`15`), + }, + }, + errorOnUpdate: false, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, tt.secretBefore, metav1.CreateOptions{}) + require.NoError(t, err) + _, err = kongClient.ConfigurationV1().KongPlugins(ns.Name).Create(ctx, tt.KongPlugin, metav1.CreateOptions{}) + require.NoError(t, err) + defer func() { + err := kongClient.ConfigurationV1().KongPlugins(ns.Name).Delete(ctx, tt.KongPlugin.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }() + + _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Update(ctx, tt.secretAfter, metav1.UpdateOptions{}) + if tt.errorOnUpdate { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errorContains) + } else { + require.NoError(t, err) + } + }) + } + + t.Log("verifying that the validation fails when secret generates invalid plugin configuration for KongClusterPlugin") + for _, tt := range []struct { + name string + kongClusterPlugin *kongv1.KongClusterPlugin + secretBefore *corev1.Secret + secretAfter *corev1.Secret + errorOnUpdate bool + errorContains string + }{ + { + name: "should pass the validation if the secret used in ConfigFrom of KongClusterPlugin generates valid configuration", + kongClusterPlugin: &kongv1.KongClusterPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-rate-limiting-valid", + Annotations: map[string]string{ + annotations.IngressClassKey: consts.IngressClass, + }, + }, + PluginName: "rate-limiting", + ConfigFrom: &kongv1.NamespacedConfigSource{ + SecretValue: kongv1.NamespacedSecretValueFromSource{ + Namespace: ns.Name, + Secret: "cluster-conf-secret-valid", + Key: "rate-limiting-config", + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-valid", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":5}`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-valid", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":10}`), + }, + }, + errorOnUpdate: false, + }, + { + name: "should fail the validation if the secret in ConfigFrom of KongClusterPlugin generates invalid configuration", + kongClusterPlugin: &kongv1.KongClusterPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-rate-limiting-invalid", + Annotations: map[string]string{ + annotations.IngressClassKey: consts.IngressClass, + }, + }, + PluginName: "rate-limiting", + ConfigFrom: &kongv1.NamespacedConfigSource{ + SecretValue: kongv1.NamespacedSecretValueFromSource{ + Namespace: ns.Name, + Secret: "cluster-conf-secret-invalid", + Key: "rate-limiting-config", + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-invalid", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":5}`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-invalid", + }, + Data: map[string][]byte{ + "rate-limiting-config": []byte(`{"limit_by":"consumer","policy":"local","minute":"5"}`), + }, + }, + errorOnUpdate: true, + errorContains: "Change on secret will generate invalid configuration for KongClusterPlugin", + }, + { + name: "should pass the validation if the secret in ConfigPatches generates valid configuration", + kongClusterPlugin: &kongv1.KongClusterPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-rate-limiting-valid-config-patches", + Annotations: map[string]string{ + annotations.IngressClassKey: consts.IngressClass, + }, + }, + PluginName: "rate-limiting", + Config: apiextensionsv1.JSON{ + Raw: []byte(`{"limit_by":"consumer","policy":"local"}`), + }, + ConfigPatches: []kongv1.NamespacedConfigPatch{ + { + Path: "/minute", + ValueFrom: kongv1.NamespacedConfigSource{ + SecretValue: kongv1.NamespacedSecretValueFromSource{ + Namespace: ns.Namespace, + Secret: "cluster-conf-secret-valid-patch", + Key: "rate-limiting-minute", + }, + }, + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-valid-patch", + }, + Data: map[string][]byte{ + "rate-limiting-minute": []byte(`5`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-valid-patch", + }, + Data: map[string][]byte{ + "rate-limiting-minute": []byte(`10`), + }, + }, + errorOnUpdate: false, + }, + { + name: "should pass the validation if the secret in ConfigPatches generates invalid configuration", + kongClusterPlugin: &kongv1.KongClusterPlugin{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-rate-limiting-invalid-config-patches", + Annotations: map[string]string{ + annotations.IngressClassKey: consts.IngressClass, + }, + }, + PluginName: "rate-limiting", + Config: apiextensionsv1.JSON{ + Raw: []byte(`{"limit_by":"consumer","policy":"local"}`), + }, + ConfigPatches: []kongv1.NamespacedConfigPatch{ + { + Path: "/minute", + ValueFrom: kongv1.NamespacedConfigSource{ + SecretValue: kongv1.NamespacedSecretValueFromSource{ + Namespace: ns.Name, + Secret: "cluster-conf-secret-invalid-patch", + Key: "rate-limiting-minute", + }, + }, + }, + }, + }, + secretBefore: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-invalid-patch", + }, + Data: map[string][]byte{ + "rate-limiting-minute": []byte(`5`), + }, + }, + secretAfter: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: "cluster-conf-secret-invalid-patch", + }, + Data: map[string][]byte{ + "rate-limiting-minute": []byte(`"10"`), + }, + }, + errorOnUpdate: true, + errorContains: "Change on secret will generate invalid configuration for KongClusterPlugin", + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, tt.secretBefore, metav1.CreateOptions{}) + require.NoError(t, err) + _, err = kongClient.ConfigurationV1().KongClusterPlugins().Create(ctx, tt.kongClusterPlugin, metav1.CreateOptions{}) + require.NoError(t, err) + defer func() { + err := kongClient.ConfigurationV1().KongClusterPlugins().Delete(ctx, tt.kongClusterPlugin.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }() + + _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Update(ctx, tt.secretAfter, metav1.UpdateOptions{}) + if tt.errorOnUpdate { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errorContains) + } else { + require.NoError(t, err) + } + }) + } } func ensureWebhookService(ctx context.Context, t *testing.T, name string) {