diff --git a/CHANGELOG.md b/CHANGELOG.md index c35e6424d7..9f94d04fbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,6 +121,13 @@ Adding a new version? You'll need three changes: It's nearly identical to `deploy/single/all-in-one-dbless-k4k8s-enterprise.yaml` which is used in the official docs. [#4873](https://github.com/Kong/kubernetes-ingress-controller/pull/4873) +- Credentials now use a `konghq.com/credential` label to indicate + credential type instead of the `kongCredType` field. This allows controller + compontents to avoid caching unnecessary Secrets. The `kongCredType` field is + still supported but is now deprecated. A script to generate commands to + update Secrets is available at https://github.com/Kong/kubernetes-ingress-controller/issues/2502#issuecomment-1758213596 + [#4825](https://github.com/Kong/kubernetes-ingress-controller/pull/4825) + ### Fixed diff --git a/internal/admission/handler.go b/internal/admission/handler.go index b1bde689cb..533e76bbe8 100644 --- a/internal/admission/handler.go +++ b/internal/admission/handler.go @@ -14,6 +14,7 @@ import ( gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/kong/kubernetes-ingress-controller/v2/internal/gatewayapi" + "github.com/kong/kubernetes-ingress-controller/v2/internal/util" kongv1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1beta1" ) @@ -242,7 +243,11 @@ func (h RequestHandler) handleSecret( if err != nil { return nil, err } - if _, ok := secret.Data["kongCredType"]; !ok { + // 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 } diff --git a/internal/admission/validation/consumers/credentials/validation.go b/internal/admission/validation/consumers/credentials/validation.go index 276de4348d..522662223f 100644 --- a/internal/admission/validation/consumers/credentials/validation.go +++ b/internal/admission/validation/consumers/credentials/validation.go @@ -6,6 +6,9 @@ import ( "strings" corev1 "k8s.io/api/core/v1" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/labels" + "github.com/kong/kubernetes-ingress-controller/v2/internal/util" ) // ----------------------------------------------------------------------------- @@ -15,21 +18,17 @@ import ( // ValidateCredentials performs basic validation on a credential secret given // the Kubernetes secret which contains credentials data. func ValidateCredentials(secret *corev1.Secret) error { - // the indication of credential type is required to be present on all credentials. - credentialTypeB, ok := secret.Data[TypeKey] - if !ok { - return fmt.Errorf("missing required key %s", TypeKey) + credentialType, credentialSource := util.ExtractKongCredentialType(secret) + + if credentialSource == util.CredentialTypeAbsent { + // this shouldn't occur, since we check this earlier in the admission controller's handleSecret function, but + // checking here also in case a refactor removes that + return fmt.Errorf("secret has no credential type, add a %s label", labels.LabelPrefix+labels.CredentialKey) } - credentialType := string(credentialTypeB) // verify that the credential type provided is valid if !SupportedTypes.Has(credentialType) { - return fmt.Errorf("invalid credential type %s", secret.Data[TypeKey]) - } - - // it's not valid to have a secret that ONLY has a type - if len(secret.Data) == 1 { - return fmt.Errorf("invalid credentials secret, no data present") + return fmt.Errorf("invalid credential type %s", credentialType) } // verify that all required fields are present diff --git a/internal/admission/validation/consumers/credentials/validation_test.go b/internal/admission/validation/consumers/credentials/validation_test.go index 4407b0cc57..d3ce3304fd 100644 --- a/internal/admission/validation/consumers/credentials/validation_test.go +++ b/internal/admission/validation/consumers/credentials/validation_test.go @@ -1,10 +1,15 @@ package credentials import ( + "fmt" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/labels" ) func TestUniqueConstraintsValidation(t *testing.T) { @@ -59,3 +64,110 @@ func TestUniqueConstraintsValidation(t *testing.T) { t.Log("verifying that unconstrained keys for types with constraints don't flag as violated") assert.False(t, IsKeyUniqueConstrained("basic-auth", "unconstrained-key")) } + +func TestValidateCredentials(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantErr error + }{ + { + name: "valid credential", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + wantErr: nil, + }, + { + // TODO https://github.com/Kong/kubernetes-ingress-controller/issues/4853 to be removed after deprecation window + name: "valid credential with deprectated field", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + "kongCredType": []byte("key-auth"), + }, + }, + wantErr: nil, + }, + { + name: "invalid credential type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "bee-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + wantErr: fmt.Errorf("invalid credential type bee-auth"), + }, + { + name: "no credential type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + wantErr: fmt.Errorf("secret has no credential type, add a %s label", labels.LabelPrefix+labels.CredentialKey), + }, + { + name: "missing required field", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "bee": []byte("little-rabbits-be-good"), + }, + }, + wantErr: fmt.Errorf("missing required field(s): key"), + }, + { + name: "empty required field", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte(""), + }, + }, + wantErr: fmt.Errorf("some fields were invalid due to missing data: key"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateCredentials(tt.secret) + require.Equal(t, tt.wantErr, err) + }) + } +} diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go index 5b4e162a0a..80905ca48b 100644 --- a/internal/dataplane/kongstate/kongstate.go +++ b/internal/dataplane/kongstate/kongstate.go @@ -59,6 +59,7 @@ func (ks *KongState) SanitizedCopy() *KongState { } func (ks *KongState) FillConsumersAndCredentials( + logger logr.Logger, s store.Storer, failuresCollector *failures.ResourceFailuresCollector, ) { @@ -102,6 +103,19 @@ func (ks *KongState) FillConsumersAndCredentials( continue } credConfig := map[string]interface{}{} + // try the label first. if it's present, no need to check the field + credType, credTypeSource := util.ExtractKongCredentialType(secret) + if credTypeSource == util.CredentialTypeFromField { + logger.Error(nil, + fmt.Sprintf("Secret uses deprecated kongCredType field, needs konghq.com/credential=%s label", credType), + "namesapce", secret.Namespace, "name", secret.Name) + } + if !credentials.SupportedTypes.Has(credType) { + pushCredentialResourceFailures( + fmt.Sprintf("failed to provision credential: unsupported credential type: %q", credType), + ) + continue + } for k, v := range secret.Data { // TODO populate these based on schema from Kong // and remove this workaround @@ -132,7 +146,7 @@ func (ks *KongState) FillConsumersAndCredentials( if err != nil { // add a translation error here to tell that parsing TTL failed. pushCredentialResourceFailures( - fmt.Sprintf("faield to parse ttl to int: %v, skipfilling the fiedl", err), + fmt.Sprintf("failed to parse ttl to int: %v, skipfilling the field", err), ) } else { credConfig[k] = intVal @@ -141,25 +155,6 @@ func (ks *KongState) FillConsumersAndCredentials( } credConfig[k] = string(v) } - credType, ok := credConfig["kongCredType"].(string) - if !ok { - pushCredentialResourceFailures( - fmt.Sprintf("failed to provision credential: invalid kongCredType: type '%T' not string", credType), - ) - continue - } - if !credentials.SupportedTypes.Has(credType) { - pushCredentialResourceFailures( - fmt.Sprintf("failed to provision credential: unsupported kongCredType: %q", credType), - ) - continue - } - if len(credConfig) <= 1 { // 1 key of credType itself - pushCredentialResourceFailures( - "failed to provision credential: empty secret", - ) - continue - } credTags := util.GenerateTagsForObject(secret) if err := c.SetCredential(credType, credConfig, credTags); err != nil { pushCredentialResourceFailures( diff --git a/internal/dataplane/kongstate/kongstate_test.go b/internal/dataplane/kongstate/kongstate_test.go index 4c4789b393..999cb84be9 100644 --- a/internal/dataplane/kongstate/kongstate_test.go +++ b/internal/dataplane/kongstate/kongstate_test.go @@ -20,6 +20,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v2/internal/annotations" "github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/failures" + "github.com/kong/kubernetes-ingress-controller/v2/internal/labels" "github.com/kong/kubernetes-ingress-controller/v2/internal/store" "github.com/kong/kubernetes-ingress-controller/v2/internal/util" kongv1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1" @@ -478,6 +479,43 @@ func TestFillConsumersAndCredentials(t *testing.T) { "foo": []byte("bar"), }, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "labeledSecret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "labeledSecretWithCredField", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "kongCredType": []byte("key-auth"), + "key": []byte("little-rabbits-be-good"), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "badTypeLabeledSecret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "bee-auth", + }, + }, + Data: map[string][]byte{ + "foo": []byte("bar"), + }, + }, } testCases := []struct { @@ -613,7 +651,104 @@ func TestFillConsumersAndCredentials(t *testing.T) { }, }, expectedTranslationFailureMessages: map[k8stypes.NamespacedName]string{ - {Namespace: "default", Name: "foo"}: fmt.Sprintf("failed to provision credential: unsupported kongCredType: %q", "unsupported"), + {Namespace: "default", Name: "foo"}: fmt.Sprintf("failed to provision credential: unsupported credential type: %q", "unsupported"), + }, + }, + { + name: "referring to secret with unsupported credential label", + k8sConsumers: []*kongv1.KongConsumer{ + { + TypeMeta: kongConsumerTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": annotations.DefaultIngressClass, + }, + }, + Username: "foo", + Credentials: []string{ + "badTypeLabeledSecret", + }, + }, + }, + expectedKongStateConsumers: []Consumer{ + { + Consumer: kong.Consumer{ + Username: kong.String("foo"), + }, + }, + }, + expectedTranslationFailureMessages: map[k8stypes.NamespacedName]string{ + {Namespace: "default", Name: "foo"}: fmt.Sprintf("failed to provision credential: unsupported credential type: %q", "bee-auth"), + }, + }, + { + name: "KongConsumer with key-auth from label secret", + k8sConsumers: []*kongv1.KongConsumer{ + { + TypeMeta: kongConsumerTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": annotations.DefaultIngressClass, + }, + }, + Username: "foo", + CustomID: "foo", + Credentials: []string{ + "labeledSecret", + }, + }, + }, + expectedKongStateConsumers: []Consumer{ + { + Consumer: kong.Consumer{ + Username: kong.String("foo"), + CustomID: kong.String("foo"), + }, + KeyAuths: []*KeyAuth{{kong.KeyAuth{ + Key: kong.String("little-rabbits-be-good"), + Tags: util.GenerateTagsForObject(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "labeledSecret"}, + }), + }}}, + }, + }, + }, + { + name: "KongConsumer with key-auth from label secret with the old cred field", + k8sConsumers: []*kongv1.KongConsumer{ + { + TypeMeta: kongConsumerTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": annotations.DefaultIngressClass, + }, + }, + Username: "foo", + CustomID: "foo", + Credentials: []string{ + "labeledSecretWithCredField", + }, + }, + }, + expectedKongStateConsumers: []Consumer{ + { + Consumer: kong.Consumer{ + Username: kong.String("foo"), + CustomID: kong.String("foo"), + }, + KeyAuths: []*KeyAuth{{kong.KeyAuth{ + Key: kong.String("little-rabbits-be-good"), + Tags: util.GenerateTagsForObject(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "labeledSecretWithCredField"}, + }), + }}}, + }, }, }, } @@ -630,7 +765,7 @@ func TestFillConsumersAndCredentials(t *testing.T) { failureCollector := failures.NewResourceFailuresCollector(logger) state := KongState{} - state.FillConsumersAndCredentials(store, failureCollector) + state.FillConsumersAndCredentials(logger, store, failureCollector) // compare translated consumers. require.Len(t, state.Consumers, len(tc.expectedKongStateConsumers)) // compare fields. Since we only test for translating a single consumer, we only compare the first one if exists. diff --git a/internal/dataplane/parser/parser.go b/internal/dataplane/parser/parser.go index caa5c69731..01cebe9c88 100644 --- a/internal/dataplane/parser/parser.go +++ b/internal/dataplane/parser/parser.go @@ -193,7 +193,7 @@ func (p *Parser) BuildKongConfig() KongConfigBuildingResult { result.FillOverrides(p.logger, p.storer) // generate consumers and credentials - result.FillConsumersAndCredentials(p.storer, p.failuresCollector) + result.FillConsumersAndCredentials(p.logger, p.storer, p.failuresCollector) for i := range result.Consumers { p.registerSuccessfullyParsedObject(&result.Consumers[i].K8sKongConsumer) } diff --git a/internal/labels/labels.go b/internal/labels/labels.go new file mode 100644 index 0000000000..13b0b32d47 --- /dev/null +++ b/internal/labels/labels.go @@ -0,0 +1,11 @@ +package labels + +import "github.com/kong/kubernetes-ingress-controller/v2/internal/annotations" + +const ( + // LabelPrefix is the string used at the beginning of KIC-specific labels. + LabelPrefix = annotations.AnnotationPrefix + + // CredentialKey is the key used to indicate a Secret's credential type. + CredentialKey = "/credential" //nolint:gosec +) diff --git a/internal/util/credential.go b/internal/util/credential.go new file mode 100644 index 0000000000..f82a998eee --- /dev/null +++ b/internal/util/credential.go @@ -0,0 +1,36 @@ +package util + +import ( + corev1 "k8s.io/api/core/v1" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/labels" +) + +// TODO https://github.com/Kong/kubernetes-ingress-controller/issues/4853 remove field handling when no longer supported. + +// CredentialTypeSource indicates the source of credential type information (or lack thereof) in a Secret. +type CredentialTypeSource int + +const ( + // CredentialTypeAbsent indicates that no credential information is present in a Secret. + CredentialTypeAbsent CredentialTypeSource = iota + // CredentialTypeFromLabel indicates that a Secret's credential type was determined from a label. + CredentialTypeFromLabel + // CredentialTypeFromField indicates that a Secret's credential type was determined from a data field. + CredentialTypeFromField +) + +// ExtractKongCredentialType returns the credential type of a Secret and a code indicating whether the credential type +// was obtained from a label, field, or not at all. Labels take precedence over fields if both are present. +func ExtractKongCredentialType(secret *corev1.Secret) (string, CredentialTypeSource) { + credType, labelOk := secret.Labels[labels.LabelPrefix+labels.CredentialKey] + if !labelOk { + // if no label, fall back to the deprecated field + credBytes, fieldOk := secret.Data["kongCredType"] + if !fieldOk { + return "", CredentialTypeAbsent + } + return string(credBytes), CredentialTypeFromField + } + return credType, CredentialTypeFromLabel +} diff --git a/internal/util/credential_test.go b/internal/util/credential_test.go new file mode 100644 index 0000000000..c50c21cc2f --- /dev/null +++ b/internal/util/credential_test.go @@ -0,0 +1,94 @@ +package util + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/labels" +) + +func TestExtractKongCredentialType(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + credType string + credTypeSource CredentialTypeSource + }{ + { + name: "labeled credential", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + credType: "key-auth", + credTypeSource: CredentialTypeFromLabel, + }, + { + // TODO https://github.com/Kong/kubernetes-ingress-controller/issues/4853 to be removed after deprecation window + name: "kongCredType field credential", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + "kongCredType": []byte("key-auth"), + }, + }, + credType: "key-auth", + credTypeSource: CredentialTypeFromField, + }, + { + // TODO https://github.com/Kong/kubernetes-ingress-controller/issues/4853 to be removed after deprecation window + name: "kongCredType field and labeled credential, label takes precedence", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + Labels: map[string]string{ + labels.LabelPrefix + labels.CredentialKey: "key-auth", + }, + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + "kongCredType": []byte("bee-auth"), + }, + }, + credType: "key-auth", + credTypeSource: CredentialTypeFromLabel, + }, + { + name: "no credential type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "key": []byte("little-rabbits-be-good"), + }, + }, + credType: "", + credTypeSource: CredentialTypeAbsent, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credType, credTypeSource := ExtractKongCredentialType(tt.secret) + require.Equal(t, tt.credType, credType) + require.Equal(t, tt.credTypeSource, credTypeSource) + }) + } +} diff --git a/renovate.json b/renovate.json index 02ef9f7a97..06eb8d1bfe 100644 --- a/renovate.json +++ b/renovate.json @@ -1,7 +1,7 @@ { "$schema": "https://docs.renovatebot.com/renovate-schema.json", "configMigration": true, - "enabledManagers": ["regex"], + "enabledManagers": ["regex", "kustomize"], "automerge": false, "separateMinorPatch": true, "labels": ["dependencies"], diff --git a/test/envtest/kongstate_consumer_failures_test.go b/test/envtest/kongstate_consumer_failures_test.go index f1cf916b00..816a0c91db 100644 --- a/test/envtest/kongstate_consumer_failures_test.go +++ b/test/envtest/kongstate_consumer_failures_test.go @@ -98,7 +98,7 @@ func TestKongStateFillConsumersAndCredentialsFailure(t *testing.T) { // KongConsumer name -> event message kongConsumerTranslationFailureMessages := map[string]string{ - "consumer-empty-cred": `credential "empty-cred" failure: failed to provision credential: empty secret`, + "consumer-empty-cred": `credential "empty-cred" failure: failed to provision credential: key-auth is invalid: no key`, "consumer-no-username": `no username or custom_id specified`, }