From 525106717e0e5aa6ddf107b85efc361e13bb37eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Thu, 26 Sep 2024 14:34:34 +0200 Subject: [PATCH 1/5] feat(konnect): add KongKey - KongKeySet binding --- config/samples/konnect_kongkeyset.yaml | 61 +++++ controller/konnect/conditions/conditions.go | 14 + controller/konnect/ops/ops_kongkey_test.go | 166 ++++++++--- controller/konnect/reconciler_generic.go | 288 ++++++++++++++++++++ test/envtest/konnect_entities_key_test.go | 174 +++++++++--- test/envtest/update_status.go | 18 ++ test/helpers/deploy/deploy_resources.go | 6 +- 7 files changed, 640 insertions(+), 87 deletions(-) diff --git a/config/samples/konnect_kongkeyset.yaml b/config/samples/konnect_kongkeyset.yaml index 8ca4bc0b6..89c50e3c3 100644 --- a/config/samples/konnect_kongkeyset.yaml +++ b/config/samples/konnect_kongkeyset.yaml @@ -37,3 +37,64 @@ spec: name: key-set-1 tags: - production +--- +kind: KongKey +apiVersion: configuration.konghq.com/v1alpha1 +metadata: + name: key-1 + namespace: default + annotations: + konghq.com/tags: "infra" +spec: + controlPlaneRef: + type: konnectNamespacedRef + konnectNamespacedRef: + name: test1 + keySetRef: + type: namespacedRef + namespacedRef: + name: key-set-1 + tags: + - production + kid: kid + name: key-1 + pem: + private_key: | + -----BEGIN PRIVATE KEY----- + MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCdLY9b3Pb/Fsxo + cIJAKxZQKEtqTU8k72BOvMBtzcIN9ekGFrUYB8GQgIJsPIUw4+c4XK99YNh0tVBE + /9W8OdyXwopzqNn+nRfrhXpxDu+BVvjQ/AENAHKqg8pJKhNTd4W6dAzxelLO/t7y + rlXbjGX/Ry/3ednPq6PpDcxvqgc+v7Rcmh+5dEKdIoIrppjUc2X38+LXcy9xOuML + FtxNtx+NB+5bNq31eooT9OKk3r7mA0gX4Su2DnIL+SLsdTIb0dnCBIydUpbLdYfd + dew1UGy2XtlWsxux3zoXjGe+RBtndUzPBvyb/k6g2QFAaIEwndPbwQ4fi9y4FrB7 + hqjQa+OLAgMBAAECggEAMkWruCydHarLl04BAwgk+19f+7Cdc0lTCuwJOjWY70Er + loR1yKlWamMIFBlpWmFSh67xfE8Y/H8vnNodITZ6jVmuUd78VpklWPHY30dxKHPK + YoFvzppJkqtTbIJWKxir/551s1i2GrnfUkybbnzh9Lvuph9loKwb4YNF06NU7OcA + tgCk78oA/JpVa01PCJYmVy8zI4UERt/2mBzuummk8kJhPl+A7K9gVkNz6KSeQDGM + QUZ6gtiYtyg7nT+kI1H6LfwokxCljQ+MBuB62eehUsie7EmpgmJqbzesqnWfdbFp + IjCDn174R45o0FUD1QpcbQWxa39cdo4f6oP4My8szQKBgQDJT8Z7yfYAXeAyVeRD + tTrOWhXqKzj3DOO65n+Evwen9O4NlWKtbC6LeaogcrlJSuHhYlAShdgrBy6DLWi8 + DEwozbK5YvpKbQ8u03rJYnfM6nN57gvm49SgsaoUPO4FlZMt1V3VC6kG2K4YbP8Y + OWy5FCdYPRlOtPp4CsFQ4xzbjQKBgQDH4IIMBT667V+7fWC/YyvUqJoIimuZcVzP + zmxICWVP9u4VKCHw46sbqukCw56bMYD8X7zu16Sbkkc3YzeOP6n4NGcLUzIFkweq + nzKdxZ6wj00x+mHT0/i/B8IZDYSkRFHF7ISV3Z8B9FuJXfsk5xGHVc47jVOTyKPb + XuLzcAlpdwKBgAsij37/X80LZEBEgfjAyHzrfLTUKTV5EAuhfkIwctL2eEhmD+w5 + xKVQWHms/tSwAKh/0KAFqTxQDGGTHGzyXTAQmKcqc1+0gpd7eRo0iR3bhgGjiiL+ + TR+KVDcEW8IRUO/DEoqbN4E6cP7G4KFNY9ck5zw5PPIejpAfQCwiM9FtAoGAW8Kn + EWurA9gMFiAWNWcK7UNGC9u4UCZqDIDg1yVxHIfpf08AXf23RSludbVm8CqG49Xz + /9aCHGXIShZDoAt8NZWhJOLZ2RNJ9rvFWgcqtjXjo6kmFkB/NvwR0LyTA3LV876E + k+S9pgEPsP2zWZq3QmFTH6XfE76N8x0ZpdbuizsCgYBBDNh8AfKbaEdo90bQi8No + sNqbHFAc12H6qxqnRl/pDvoY34wBVeZP3QEfb/XeOO2BVrcx6tGvIosy2lkOJtrh + ckY/QO1OLvcDtDgMA6qOr1rAROP/aWhuhJg1Aw50vCuy3z96CfUVSJBG+r0v7HvO + ZNgrh9kB0qmomKcjwwJlKQ== + -----END PRIVATE KEY----- + public_key: | + -----BEGIN PUBLIC KEY----- + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnS2PW9z2/xbMaHCCQCsW + UChLak1PJO9gTrzAbc3CDfXpBha1GAfBkICCbDyFMOPnOFyvfWDYdLVQRP/VvDnc + l8KKc6jZ/p0X64V6cQ7vgVb40PwBDQByqoPKSSoTU3eFunQM8XpSzv7e8q5V24xl + /0cv93nZz6uj6Q3Mb6oHPr+0XJofuXRCnSKCK6aY1HNl9/Pi13MvcTrjCxbcTbcf + jQfuWzat9XqKE/TipN6+5gNIF+Ertg5yC/ki7HUyG9HZwgSMnVKWy3WH3XXsNVBs + tl7ZVrMbsd86F4xnvkQbZ3VMzwb8m/5OoNkBQGiBMJ3T28EOH4vcuBawe4ao0Gvj + iwIDAQAB + -----END PUBLIC KEY----- diff --git a/controller/konnect/conditions/conditions.go b/controller/konnect/conditions/conditions.go index 7452bcdd7..532809f09 100644 --- a/controller/konnect/conditions/conditions.go +++ b/controller/konnect/conditions/conditions.go @@ -128,6 +128,20 @@ const ( KongUpstreamRefReasonInvalid = "Invalid" ) +const ( + // KeySetRefValidConditionType is the type of the condition that indicates + // whether the KeySet reference is valid and points to an existing + // KeySet. + KeySetRefValidConditionType = "KeySetRefValid" + + // KeySetRefReasonValid is the reason used with the KeySetRefValid + // condition type indicating that the KeySet reference is valid. + KeySetRefReasonValid = "Valid" + // KeySetRefReasonInvalid is the reason used with the KeySetRefValid + // condition type indicating that the KeySet reference is invalid. + KeySetRefReasonInvalid = "Invalid" +) + const ( // KongCertificateRefValidConditionType is the type of the condition that indicates // whether the KongCertificate reference is valid and points to an existing KongCertificate diff --git a/controller/konnect/ops/ops_kongkey_test.go b/controller/konnect/ops/ops_kongkey_test.go index 5852c6657..08a19f4cb 100644 --- a/controller/konnect/ops/ops_kongkey_test.go +++ b/controller/konnect/ops/ops_kongkey_test.go @@ -1,9 +1,10 @@ package ops import ( + "sort" "testing" - "github.com/google/uuid" + sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" "github.com/samber/lo" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,55 +13,138 @@ import ( konnectconsts "github.com/kong/gateway-operator/controller/konnect/consts" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" + konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" ) func TestKongKeyToKeyInput(t *testing.T) { - key := &configurationv1alpha1.KongKey{ - TypeMeta: metav1.TypeMeta{ - Kind: "KongKey", - APIVersion: "configuration.konghq.com/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "key-1", - Namespace: "default", - Generation: 2, - UID: k8stypes.UID(uuid.NewString()), - Annotations: map[string]string{ - konnectconsts.AnnotationTags: "tag1,tag2,duplicate", + testCases := []struct { + name string + key *configurationv1alpha1.KongKey + expectedOutput sdkkonnectcomp.KeyInput + }{ + { + name: "kong key with all fields set without key set", + key: &configurationv1alpha1.KongKey{ + TypeMeta: metav1.TypeMeta{ + Kind: "KongKey", + APIVersion: "configuration.konghq.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "key-1", + Namespace: "default", + Generation: 2, + UID: k8stypes.UID("key-uid"), + Annotations: map[string]string{ + konnectconsts.AnnotationTags: "tag1,tag2,duplicate", + }, + }, + Spec: configurationv1alpha1.KongKeySpec{ + KongKeyAPISpec: configurationv1alpha1.KongKeyAPISpec{ + KID: "kid", + Name: lo.ToPtr("name"), + JWK: lo.ToPtr("jwk"), + PEM: &configurationv1alpha1.PEMKeyPair{ + PublicKey: "public", + PrivateKey: "private", + }, + Tags: []string{"tag3", "tag4", "duplicate"}, + }, + }, + }, + expectedOutput: sdkkonnectcomp.KeyInput{ + Kid: "kid", + Name: lo.ToPtr("name"), + Jwk: lo.ToPtr("jwk"), + Pem: &sdkkonnectcomp.Pem{ + PublicKey: lo.ToPtr("public"), + PrivateKey: lo.ToPtr("private"), + }, + Tags: []string{ + "duplicate", + "k8s-generation:2", + "k8s-group:configuration.konghq.com", + "k8s-kind:KongKey", + "k8s-name:key-1", + "k8s-namespace:default", + "k8s-uid:key-uid", + "k8s-version:v1alpha1", + "tag1", + "tag2", + "tag3", + "tag4", + }, }, }, - Spec: configurationv1alpha1.KongKeySpec{ - KongKeyAPISpec: configurationv1alpha1.KongKeyAPISpec{ - KID: "kid", + { + name: "kong key with all fields set with key set", + key: &configurationv1alpha1.KongKey{ + TypeMeta: metav1.TypeMeta{ + Kind: "KongKey", + APIVersion: "configuration.konghq.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "key-1", + Namespace: "default", + Generation: 2, + UID: k8stypes.UID("key-uid"), + Annotations: map[string]string{ + konnectconsts.AnnotationTags: "tag1,tag2,duplicate", + }, + }, + Spec: configurationv1alpha1.KongKeySpec{ + KongKeyAPISpec: configurationv1alpha1.KongKeyAPISpec{ + KID: "kid", + Name: lo.ToPtr("name"), + JWK: lo.ToPtr("jwk"), + PEM: &configurationv1alpha1.PEMKeyPair{ + PublicKey: "public", + PrivateKey: "private", + }, + Tags: []string{"tag3", "tag4", "duplicate"}, + }, + }, + Status: configurationv1alpha1.KongKeyStatus{ + Konnect: &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{ + KeySetID: "key-set-id", + }, + }, + }, + expectedOutput: sdkkonnectcomp.KeyInput{ + Kid: "kid", Name: lo.ToPtr("name"), - JWK: lo.ToPtr("jwk"), - PEM: &configurationv1alpha1.PEMKeyPair{ - PublicKey: "public", - PrivateKey: "private", + Jwk: lo.ToPtr("jwk"), + Pem: &sdkkonnectcomp.Pem{ + PublicKey: lo.ToPtr("public"), + PrivateKey: lo.ToPtr("private"), + }, + Set: &sdkkonnectcomp.Set{ + ID: lo.ToPtr("key-set-id"), + }, + Tags: []string{ + "duplicate", + "k8s-generation:2", + "k8s-group:configuration.konghq.com", + "k8s-kind:KongKey", + "k8s-name:key-1", + "k8s-namespace:default", + "k8s-uid:key-uid", + "k8s-version:v1alpha1", + "tag1", + "tag2", + "tag3", + "tag4", }, - Tags: []string{"tag3", "tag4", "duplicate"}, }, }, } - output := kongKeyToKeyInput(key) - expectedTags := []string{ - "k8s-generation:2", - "k8s-kind:KongKey", - "k8s-name:key-1", - "k8s-uid:" + string(key.GetUID()), - "k8s-version:v1alpha1", - "k8s-group:configuration.konghq.com", - "k8s-namespace:default", - "tag1", - "tag2", - "tag3", - "tag4", - "duplicate", + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := kongKeyToKeyInput(tc.key) + + // Tags order is not guaranteed, so we need to sort them before comparing. + sort.Strings(output.Tags) + require.Equal(t, tc.expectedOutput, output) + }) } - require.ElementsMatch(t, expectedTags, output.Tags) - require.Equal(t, "kid", output.Kid) - require.Equal(t, "name", *output.Name) - require.Equal(t, "jwk", *output.Jwk) - require.Equal(t, "public", *output.Pem.PublicKey) - require.Equal(t, "private", *output.Pem.PrivateKey) } diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go index ec1493ba7..0941f0bb9 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -288,6 +288,12 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( return res, nil } + // If a type has a KongKeySet ref, handle it. + res, err = handleKongKeySetRef(ctx, r.Client, ent) + if err != nil || !res.IsZero() { + return res, err + } + apiAuthRef, err := getAPIAuthRefNN(ctx, r.Client, ent) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to get APIAuth ref for %s: %w", client.ObjectKeyFromObject(ent), err) @@ -725,6 +731,189 @@ func getConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constraints.E } } +func getServiceRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + e TEnt, +) mo.Option[configurationv1alpha1.ServiceRef] { + switch e := any(e).(type) { + case *configurationv1alpha1.KongRoute: + if e.Spec.ServiceRef == nil { + return mo.None[configurationv1alpha1.ServiceRef]() + } + return mo.Some(*e.Spec.ServiceRef) + default: + return mo.None[configurationv1alpha1.ServiceRef]() + } +} + +func getKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + e TEnt, +) mo.Option[configurationv1alpha1.KeySetRef] { + switch e := any(e).(type) { + case *configurationv1alpha1.KongKey: + if e.Spec.KeySetRef == nil { + return mo.None[configurationv1alpha1.KeySetRef]() + } + return mo.Some(*e.Spec.KeySetRef) + default: + return mo.None[configurationv1alpha1.KeySetRef]() + } +} + +// handleKongServiceRef handles the ServiceRef for the given entity. +// It sets the owner reference to the referenced KongService and updates the +// status of the entity based on the referenced KongService status. +func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + ctx context.Context, + cl client.Client, + ent TEnt, +) (ctrl.Result, error) { + kongServiceRef, ok := getServiceRef(ent).Get() + if !ok { + return ctrl.Result{}, nil + } + switch kongServiceRef.Type { + case configurationv1alpha1.ServiceRefNamespacedRef: + svc := configurationv1alpha1.KongService{} + nn := types.NamespacedName{ + Name: kongServiceRef.NamespacedRef.Name, + // TODO: handle cross namespace refs + Namespace: ent.GetNamespace(), + } + + if err := cl.Get(ctx, nn, &svc); err != nil { + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KongServiceRefValidConditionType, + metav1.ConditionFalse, + conditions.KongServiceRefReasonInvalid, + err.Error(), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + return ctrl.Result{}, fmt.Errorf("can't get the referenced KongService %s: %w", nn, err) + } + + // If referenced KongService is being deleted, return an error so that we + // can remove the entity from Konnect first. + if delTimestamp := svc.GetDeletionTimestamp(); !delTimestamp.IsZero() { + return ctrl.Result{}, ReferencedKongServiceIsBeingDeleted{ + Reference: nn, + } + } + + cond, ok := k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, &svc) + if !ok || cond.Status != metav1.ConditionTrue { + ent.SetKonnectID("") + if res, err := updateStatusWithCondition( + ctx, cl, ent, + conditions.KongServiceRefValidConditionType, + metav1.ConditionFalse, + conditions.KongServiceRefReasonInvalid, + fmt.Sprintf("Referenced KongService %s is not programmed yet", nn), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } + return ctrl.Result{Requeue: true}, nil + } + + old := ent.DeepCopyObject().(TEnt) + if err := controllerutil.SetOwnerReference(&svc, ent, cl.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to set owner reference: %w", err) + } + if err := cl.Patch(ctx, ent, client.MergeFrom(old)); err != nil { + if k8serrors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + + // TODO(pmalek): make this generic. + // Service ID is not stored in KonnectEntityStatus because not all entities + // have a ServiceRef, hence the type constraints in the reconciler can't be used. + if route, ok := any(ent).(*configurationv1alpha1.KongRoute); ok { + if route.Status.Konnect == nil { + route.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndServiceRefs{} + } + route.Status.Konnect.ServiceID = svc.Status.Konnect.GetKonnectID() + } + + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KongServiceRefValidConditionType, + metav1.ConditionTrue, + conditions.KongServiceRefReasonValid, + fmt.Sprintf("Referenced KongService %s programmed", nn), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + cpRef, ok := getControlPlaneRef(&svc).Get() + if !ok { + return ctrl.Result{}, fmt.Errorf( + "KongRoute references a KongService %s which does not have a ControlPlane ref", + client.ObjectKeyFromObject(&svc), + ) + } + cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) + if err != nil { + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.ControlPlaneRefValidConditionType, + metav1.ConditionFalse, + conditions.ControlPlaneRefReasonInvalid, + err.Error(), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + if k8serrors.IsNotFound(err) { + return ctrl.Result{}, ReferencedControlPlaneDoesNotExistError{ + Reference: nn, + Err: err, + } + } + return ctrl.Result{}, err + } + + cond, ok = k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, cp) + if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.ControlPlaneRefValidConditionType, + metav1.ConditionFalse, + conditions.ControlPlaneRefReasonInvalid, + fmt.Sprintf("Referenced ControlPlane %s is not programmed yet", nn), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + return ctrl.Result{Requeue: true}, nil + } + + // TODO(pmalek): make this generic. + // CP ID is not stored in KonnectEntityStatus because not all entities + // have a ControlPlaneRef, hence the type constraints in the reconciler can't be used. + if resource, ok := any(ent).(EntityWithControlPlaneRef); ok { + resource.SetControlPlaneID(cp.Status.ID) + } + + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.ControlPlaneRefValidConditionType, + metav1.ConditionTrue, + conditions.ControlPlaneRefReasonValid, + fmt.Sprintf("Referenced ControlPlane %s is programmed", nn), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + default: + return ctrl.Result{}, fmt.Errorf("unimplemented KongService ref type %q", kongServiceRef.Type) + } + + return ctrl.Result{}, nil +} + // handleKongConsumerRef handles the ConsumerRef for the given entity. // It sets the owner reference to the referenced KongConsumer and updates the // status of the entity based on the referenced KongConsumer status. @@ -1056,6 +1245,105 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr } } +// handleKongKeySetRef handles the KeySetRef for the given entity. +func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + ctx context.Context, + cl client.Client, + ent TEnt, +) (ctrl.Result, error) { + keySetRef, ok := getKeySetRef(ent).Get() + if !ok { + if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { + // If the entity has a resolved reference, but the spec has changed, we need to adjust the status. + if key.Status.Konnect != nil && key.Status.Konnect.GetKeySetID() != "" { + key.Status.Konnect.KeySetID = "" + if res, err := updateStatusWithCondition(ctx, cl, key, + conditions.KeySetRefValidConditionType, + metav1.ConditionTrue, + conditions.KeySetRefReasonValid, + "KeySetRef is nil", + ); err != nil || !res.IsZero() { + return res, err + } + } + } + return ctrl.Result{}, nil + } + + if keySetRef.Type != configurationv1alpha1.KeySetRefNamespacedRef { + return ctrl.Result{}, fmt.Errorf("unsupported KeySet ref type %q", keySetRef.Type) + } + + keySet := configurationv1alpha1.KongKeySet{} + nn := types.NamespacedName{ + Name: keySetRef.NamespacedRef.Name, + Namespace: ent.GetNamespace(), + } + + if err := cl.Get(ctx, nn, &keySet); err != nil { + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionFalse, + conditions.KeySetRefReasonInvalid, + err.Error(), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + // If it was not found, let's requeue. + if k8serrors.IsNotFound(err) { + return ctrl.Result{Requeue: true}, nil + } + + return ctrl.Result{}, fmt.Errorf("failed getting KongKeySet %s: %w", nn, err) + } + + // If referenced KongKeySet is being deleted, requeue. + if delTimestamp := keySet.GetDeletionTimestamp(); !delTimestamp.IsZero() { + return ctrl.Result{ + RequeueAfter: time.Until(delTimestamp.Time), + }, nil + } + + // Verify that the KongKeySet is programmed. + cond, ok := k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, &keySet) + if !ok || cond.Status != metav1.ConditionTrue { + if res, err := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionFalse, + conditions.KeySetRefReasonInvalid, + fmt.Sprintf("Referenced KongKeySet %s is not programmed yet", nn), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } + return ctrl.Result{Requeue: true}, nil + } + + // TODO: make this generic. + // KongKeySet ID is not stored in KonnectEntityStatus because not all entities + // have a KeySetRef, hence the type constraints in the reconciler can't be used. + if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { + if key.Status.Konnect == nil { + key.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{} + } + key.Status.Konnect.KeySetID = keySet.Status.Konnect.GetKonnectID() + } + + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionTrue, + conditions.KeySetRefReasonValid, + fmt.Sprintf("Referenced KongKeySet %s programmed", nn), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + return ctrl.Result{}, nil +} + func conditionMessageReferenceKonnectAPIAuthConfigurationInvalid(apiAuthRef types.NamespacedName) string { return fmt.Sprintf("referenced KonnectAPIAuthConfiguration %s is invalid", apiAuthRef) } diff --git a/test/envtest/konnect_entities_key_test.go b/test/envtest/konnect_entities_key_test.go index 1e0635c8f..b953657ac 100644 --- a/test/envtest/konnect_entities_key_test.go +++ b/test/envtest/konnect_entities_key_test.go @@ -28,8 +28,10 @@ func TestKongKey(t *testing.T) { keyKid = "key-kid" keyName = "key-name" keyID = "key-id" - ) + keySetName = "key-set-name" + keySetID = "key-set-id" + ) t.Parallel() ctx, cancel := Context(t, context.Background()) defer cancel() @@ -71,50 +73,132 @@ func TestKongKey(t *testing.T) { t.Log("Setting up a watch for KongKey events") w := setupWatch[configurationv1alpha1.KongKeyList](t, ctx, cl, client.InNamespace(ns.Name)) - t.Log("Creating KongKey") - createdKey := deploy.KongKeyAttachedToCP(t, ctx, clientNamespaced, keyKid, keyName, cp) + t.Run("without KongKeySet", func(t *testing.T) { + t.Log("Creating KongKey") + createdKey := deploy.KongKeyAttachedToCP(t, ctx, clientNamespaced, keyKid, keyName, cp) + + t.Log("Waiting for KongKey to be programmed") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { + if c.GetName() != createdKey.GetName() { + return false + } + return lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == conditions.KonnectEntityProgrammedConditionType && + condition.Status == metav1.ConditionTrue + }) + }, "KongKey's Programmed condition should be true eventually") + + t.Log("Waiting for KongKey to be created in the SDK") + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on KongKey update") + sdk.KeysSDK.EXPECT().UpsertKey(mock.Anything, mock.MatchedBy(func(r sdkkonnectops.UpsertKeyRequest) bool { + return r.KeyID == keyID && + lo.Contains(r.Key.Tags, "addedTag") + })).Return(&sdkkonnectops.UpsertKeyResponse{}, nil) + + t.Log("Patching KongKey") + certToPatch := createdKey.DeepCopy() + certToPatch.Spec.Tags = append(certToPatch.Spec.Tags, "addedTag") + require.NoError(t, clientNamespaced.Patch(ctx, certToPatch, client.MergeFrom(createdKey))) + + t.Log("Waiting for KongKey to be updated in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on KongKey deletion") + sdk.KeysSDK.EXPECT().DeleteKey(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), keyID). + Return(&sdkkonnectops.DeleteKeyResponse{}, nil) + + t.Log("Deleting KongKey") + require.NoError(t, cl.Delete(ctx, createdKey)) + + t.Log("Waiting for KongKey to be deleted in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) + }, waitTime, tickTime) + }) - t.Log("Waiting for KongKey to be programmed") - watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { - if c.GetName() != createdKey.GetName() { - return false + t.Run("with KongKeySet", func(t *testing.T) { + t.Log("Creating KongKey") + withKeySetRef := func(key *configurationv1alpha1.KongKey) { + key.Spec.KeySetRef = &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: lo.ToPtr(configurationv1alpha1.KeySetNamespacedRef{ + Name: keySetName, + }), + } } - return lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { - return condition.Type == conditions.KonnectEntityProgrammedConditionType && - condition.Status == metav1.ConditionTrue - }) - }, "KongKey's Programmed condition should be true eventually") - - t.Log("Waiting for KongKey to be created in the SDK") - require.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) - }, waitTime, tickTime) - - t.Log("Setting up SDK expectations on KongKey update") - sdk.KeysSDK.EXPECT().UpsertKey(mock.Anything, mock.MatchedBy(func(r sdkkonnectops.UpsertKeyRequest) bool { - return r.KeyID == keyID && - lo.Contains(r.Key.Tags, "addedTag") - })).Return(&sdkkonnectops.UpsertKeyResponse{}, nil) - - t.Log("Patching KongKey") - certToPatch := createdKey.DeepCopy() - certToPatch.Spec.Tags = append(certToPatch.Spec.Tags, "addedTag") - require.NoError(t, clientNamespaced.Patch(ctx, certToPatch, client.MergeFrom(createdKey))) - - t.Log("Waiting for KongKey to be updated in the SDK") - assert.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) - }, waitTime, tickTime) - - t.Log("Setting up SDK expectations on KongKey deletion") - sdk.KeysSDK.EXPECT().DeleteKey(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), keyID). - Return(&sdkkonnectops.DeleteKeyResponse{}, nil) - - t.Log("Deleting KongKey") - require.NoError(t, cl.Delete(ctx, createdKey)) - - t.Log("Waiting for KongKey to be deleted in the SDK") - assert.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) - }, waitTime, tickTime) + createdKey := deployKongKeyAttachedToCP(t, ctx, clientNamespaced, keyKid, keyName, cp, withKeySetRef) + + t.Log("Waiting for KeySetRefValid condition to be false") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { + if c.GetName() != createdKey.GetName() { + return false + } + return lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == conditions.KeySetRefValidConditionType && + condition.Status == metav1.ConditionFalse + }) + }, "KongKey's KeySetRefValid condition should be false eventually as the KongKeySet is not created yet") + + t.Log("Setting up SDK expectations on KongKey creation with KeySetRef") + sdk.KeysSDK.EXPECT().CreateKey(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), + mock.MatchedBy(func(input sdkkonnectcomp.KeyInput) bool { + return input.Kid == keyKid && + input.Name != nil && *input.Name == keyName && + input.Set != nil && input.Set.GetID() != nil && *input.Set.GetID() == keySetID + }), + ).Return(&sdkkonnectops.CreateKeyResponse{ + Key: &sdkkonnectcomp.Key{ + ID: lo.ToPtr(keyID), + }, + }, nil) + + t.Log("Creating KongKeySet") + keySet := deployKongKeySetAttachedToCP(t, ctx, clientNamespaced, keySetName, cp) + updateKongKeySetStatusWithProgrammed(t, ctx, clientNamespaced, keySet, keySetID, cp.GetKonnectStatus().GetKonnectID()) + + t.Log("Waiting for KongKey to be programmed and associated with KongKeySet") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { + if c.GetName() != createdKey.GetName() { + return false + } + programmed := lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == conditions.KonnectEntityProgrammedConditionType && + condition.Status == metav1.ConditionTrue + }) + associated := lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == conditions.KeySetRefValidConditionType && + condition.Status == metav1.ConditionTrue + }) + keySetIDPopulated := c.Status.Konnect != nil && c.Status.Konnect.KeySetID != "" + + return programmed && associated && keySetIDPopulated + }, "KongKey's Programmed and KeySetRefValid conditions should be true eventually") + + t.Log("Waiting for KongKey to be created in the SDK") + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on KongKeySet deattachment") + sdk.KeysSDK.EXPECT().UpsertKey(mock.Anything, mock.MatchedBy(func(r sdkkonnectops.UpsertKeyRequest) bool { + return r.KeyID == keyID && + r.Key.Set == nil + })).Return(&sdkkonnectops.UpsertKeyResponse{}, nil) + + t.Log("Patching KongKey to deattach from KongKeySet") + keyToPatch := createdKey.DeepCopy() + keyToPatch.Spec.KeySetRef = nil + require.NoError(t, clientNamespaced.Patch(ctx, keyToPatch, client.MergeFrom(createdKey))) + + t.Log("Waiting for KongKey to be deattached from KongKeySet in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) + }, waitTime, tickTime) + }) } diff --git a/test/envtest/update_status.go b/test/envtest/update_status.go index 3034b14c6..2dbde5af7 100644 --- a/test/envtest/update_status.go +++ b/test/envtest/update_status.go @@ -89,6 +89,24 @@ func updateKongRouteStatusWithProgrammed( require.NoError(t, cl.Status().Update(ctx, obj)) } +func updateKongKeySetStatusWithProgrammed( + t *testing.T, + ctx context.Context, + cl client.Client, + obj *configurationv1alpha1.KongKeySet, + id, cpID string, +) { + obj.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneRef{ + ControlPlaneID: cpID, + KonnectEntityStatus: konnectEntityStatus(id), + } + obj.Status.Conditions = []metav1.Condition{ + programmedCondition(obj.GetGeneration()), + } + + require.NoError(t, cl.Status().Update(ctx, obj)) +} + func konnectEntityStatus(id string) konnectv1alpha1.KonnectEntityStatus { return konnectv1alpha1.KonnectEntityStatus{ ID: id, diff --git a/test/helpers/deploy/deploy_resources.go b/test/helpers/deploy/deploy_resources.go index 902bec157..3e4ba88b2 100644 --- a/test/helpers/deploy/deploy_resources.go +++ b/test/helpers/deploy/deploy_resources.go @@ -634,6 +634,7 @@ func KongKeyAttachedToCP( cl client.Client, kid, name string, cp *konnectv1alpha1.KonnectGatewayControlPlane, + opts ...objOption, ) *configurationv1alpha1.KongKey { t.Helper() @@ -655,6 +656,9 @@ func KongKeyAttachedToCP( }, }, } + for _, opt := range opts { + opt(key) + } require.NoError(t, cl.Create(ctx, key)) t.Logf("deployed new KongKey %s", client.ObjectKeyFromObject(key)) return key @@ -718,7 +722,7 @@ func KongKeySetAttachedToCP( keySet := &configurationv1alpha1.KongKeySet{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: "key-set-", + Name: name, }, Spec: configurationv1alpha1.KongKeySetSpec{ ControlPlaneRef: &configurationv1alpha1.ControlPlaneRef{ From 6dedfd6fac79c0a4d6279bc8028226adb0e63d18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Wed, 2 Oct 2024 14:48:03 +0200 Subject: [PATCH 2/5] handle edge cases --- controller/konnect/errors.go | 25 ++ controller/konnect/reconciler_generic.go | 325 ++------------- controller/konnect/reconciler_keysetref.go | 186 +++++++++ .../konnect/reconciler_keysetref_test.go | 370 ++++++++++++++++++ controller/konnect/watch_kongkey.go | 48 +++ test/envtest/konnect_entities_key_test.go | 19 +- test/helpers/deploy/deploy_resources.go | 4 +- 7 files changed, 689 insertions(+), 288 deletions(-) create mode 100644 controller/konnect/reconciler_keysetref.go create mode 100644 controller/konnect/reconciler_keysetref_test.go diff --git a/controller/konnect/errors.go b/controller/konnect/errors.go index 133cf8ee7..b5c5d15d4 100644 --- a/controller/konnect/errors.go +++ b/controller/konnect/errors.go @@ -111,3 +111,28 @@ type ReferencedKongCertificateDoesNotExist struct { func (e ReferencedKongCertificateDoesNotExist) Error() string { return fmt.Sprintf("referenced Kong Certificate %s does not exist: %v", e.Reference, e.Err) } + +// ReferencedKongKeySetDoesNotExist is an error type that is returned when +// a Konnect entity references a KongKeySet which does not exist. +type ReferencedKongKeySetDoesNotExist struct { + Reference types.NamespacedName + Err error +} + +// Error implements the error interface. +func (e ReferencedKongKeySetDoesNotExist) Error() string { + return fmt.Sprintf("referenced KongKeySet %s does not exist: %v", e.Reference, e.Err) +} + +// ReferencedKongKeySetIsBeingDeleted is an error type that is returned when +// a Konnect entity references a KongKeySet which is being deleted. +type ReferencedKongKeySetIsBeingDeleted struct { + Reference types.NamespacedName + DeletionTimestamp time.Time +} + +// Error implements the error interface. +func (e ReferencedKongKeySetIsBeingDeleted) Error() string { + return fmt.Sprintf("referenced KongKeySet %s is being deleted (deletion timestamp: %s)", + e.Reference, e.DeletionTimestamp) +} diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go index 0941f0bb9..1e17d4199 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -291,6 +291,38 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( // If a type has a KongKeySet ref, handle it. res, err = handleKongKeySetRef(ctx, r.Client, ent) if err != nil || !res.IsZero() { + // If the referenced KongKeySet is being deleted and the object + // is not being deleted yet then requeue until it will + // get the deletion timestamp set due to having the owner set to KongKeySet. + if errDel := (&ReferencedKongKeySetIsBeingDeleted{}); errors.As(err, errDel) && + ent.GetDeletionTimestamp().IsZero() { + return ctrl.Result{ + RequeueAfter: time.Until(errDel.DeletionTimestamp), + }, nil + } + + // If the referenced KongKeySet is not found or is being deleted + // and the object is being deleted, remove the finalizer and let the + // deletion proceed without trying to delete the entity from Konnect + // as the KongKeySet deletion will take care of it on the Konnect side. + if errors.As(err, &ReferencedKongKeySetIsBeingDeleted{}) || + errors.As(err, &ReferencedKongKeySetDoesNotExist{}) { + if !ent.GetDeletionTimestamp().IsZero() { + if controllerutil.RemoveFinalizer(ent, KonnectCleanupFinalizer) { + if err := r.Client.Update(ctx, ent); err != nil { + if k8serrors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to remove finalizer %s: %w", KonnectCleanupFinalizer, err) + } + log.Debug(logger, "finalizer removed as the owning KongKeySet is being deleted or is already gone", ent, + "finalizer", KonnectCleanupFinalizer, + ) + return ctrl.Result{}, nil + } + } + } + return res, err } @@ -731,189 +763,6 @@ func getConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constraints.E } } -func getServiceRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( - e TEnt, -) mo.Option[configurationv1alpha1.ServiceRef] { - switch e := any(e).(type) { - case *configurationv1alpha1.KongRoute: - if e.Spec.ServiceRef == nil { - return mo.None[configurationv1alpha1.ServiceRef]() - } - return mo.Some(*e.Spec.ServiceRef) - default: - return mo.None[configurationv1alpha1.ServiceRef]() - } -} - -func getKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( - e TEnt, -) mo.Option[configurationv1alpha1.KeySetRef] { - switch e := any(e).(type) { - case *configurationv1alpha1.KongKey: - if e.Spec.KeySetRef == nil { - return mo.None[configurationv1alpha1.KeySetRef]() - } - return mo.Some(*e.Spec.KeySetRef) - default: - return mo.None[configurationv1alpha1.KeySetRef]() - } -} - -// handleKongServiceRef handles the ServiceRef for the given entity. -// It sets the owner reference to the referenced KongService and updates the -// status of the entity based on the referenced KongService status. -func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( - ctx context.Context, - cl client.Client, - ent TEnt, -) (ctrl.Result, error) { - kongServiceRef, ok := getServiceRef(ent).Get() - if !ok { - return ctrl.Result{}, nil - } - switch kongServiceRef.Type { - case configurationv1alpha1.ServiceRefNamespacedRef: - svc := configurationv1alpha1.KongService{} - nn := types.NamespacedName{ - Name: kongServiceRef.NamespacedRef.Name, - // TODO: handle cross namespace refs - Namespace: ent.GetNamespace(), - } - - if err := cl.Get(ctx, nn, &svc); err != nil { - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.KongServiceRefValidConditionType, - metav1.ConditionFalse, - conditions.KongServiceRefReasonInvalid, - err.Error(), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - return ctrl.Result{}, fmt.Errorf("can't get the referenced KongService %s: %w", nn, err) - } - - // If referenced KongService is being deleted, return an error so that we - // can remove the entity from Konnect first. - if delTimestamp := svc.GetDeletionTimestamp(); !delTimestamp.IsZero() { - return ctrl.Result{}, ReferencedKongServiceIsBeingDeleted{ - Reference: nn, - } - } - - cond, ok := k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, &svc) - if !ok || cond.Status != metav1.ConditionTrue { - ent.SetKonnectID("") - if res, err := updateStatusWithCondition( - ctx, cl, ent, - conditions.KongServiceRefValidConditionType, - metav1.ConditionFalse, - conditions.KongServiceRefReasonInvalid, - fmt.Sprintf("Referenced KongService %s is not programmed yet", nn), - ); err != nil || res.Requeue { - return ctrl.Result{}, err - } - return ctrl.Result{Requeue: true}, nil - } - - old := ent.DeepCopyObject().(TEnt) - if err := controllerutil.SetOwnerReference(&svc, ent, cl.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set owner reference: %w", err) - } - if err := cl.Patch(ctx, ent, client.MergeFrom(old)); err != nil { - if k8serrors.IsConflict(err) { - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) - } - - // TODO(pmalek): make this generic. - // Service ID is not stored in KonnectEntityStatus because not all entities - // have a ServiceRef, hence the type constraints in the reconciler can't be used. - if route, ok := any(ent).(*configurationv1alpha1.KongRoute); ok { - if route.Status.Konnect == nil { - route.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndServiceRefs{} - } - route.Status.Konnect.ServiceID = svc.Status.Konnect.GetKonnectID() - } - - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.KongServiceRefValidConditionType, - metav1.ConditionTrue, - conditions.KongServiceRefReasonValid, - fmt.Sprintf("Referenced KongService %s programmed", nn), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - cpRef, ok := getControlPlaneRef(&svc).Get() - if !ok { - return ctrl.Result{}, fmt.Errorf( - "KongRoute references a KongService %s which does not have a ControlPlane ref", - client.ObjectKeyFromObject(&svc), - ) - } - cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) - if err != nil { - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.ControlPlaneRefValidConditionType, - metav1.ConditionFalse, - conditions.ControlPlaneRefReasonInvalid, - err.Error(), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - if k8serrors.IsNotFound(err) { - return ctrl.Result{}, ReferencedControlPlaneDoesNotExistError{ - Reference: nn, - Err: err, - } - } - return ctrl.Result{}, err - } - - cond, ok = k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, cp) - if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.ControlPlaneRefValidConditionType, - metav1.ConditionFalse, - conditions.ControlPlaneRefReasonInvalid, - fmt.Sprintf("Referenced ControlPlane %s is not programmed yet", nn), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - return ctrl.Result{Requeue: true}, nil - } - - // TODO(pmalek): make this generic. - // CP ID is not stored in KonnectEntityStatus because not all entities - // have a ControlPlaneRef, hence the type constraints in the reconciler can't be used. - if resource, ok := any(ent).(EntityWithControlPlaneRef); ok { - resource.SetControlPlaneID(cp.Status.ID) - } - - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.ControlPlaneRefValidConditionType, - metav1.ConditionTrue, - conditions.ControlPlaneRefReasonValid, - fmt.Sprintf("Referenced ControlPlane %s is programmed", nn), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - default: - return ctrl.Result{}, fmt.Errorf("unimplemented KongService ref type %q", kongServiceRef.Type) - } - - return ctrl.Result{}, nil -} - // handleKongConsumerRef handles the ConsumerRef for the given entity. // It sets the owner reference to the referenced KongConsumer and updates the // status of the entity based on the referenced KongConsumer status. @@ -1205,11 +1054,18 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr return ctrl.Result{Requeue: true}, nil } - old := ent.DeepCopyObject().(TEnt) - if ent.GetNamespace() != "" { + var ( + old = ent.DeepCopyObject().(TEnt) + // A cluster scoped object cannot set a namespaced object as its owner, and also we cannot set cross namespaced owner reference. // So we skip setting owner reference for cluster scoped resources (KongVault). // TODO: handle cross namespace refs + isNamespaceScoped = ent.GetNamespace() != "" + + // If an entity has another owner, we should not set the owner reference as that would prevent the entity from being deleted. + hasNoOwners = len(ent.GetOwnerReferences()) == 0 + ) + if isNamespaceScoped && hasNoOwners { if err := controllerutil.SetOwnerReference(&cp, ent, cl.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set owner reference: %w", err) } @@ -1245,105 +1101,6 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr } } -// handleKongKeySetRef handles the KeySetRef for the given entity. -func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( - ctx context.Context, - cl client.Client, - ent TEnt, -) (ctrl.Result, error) { - keySetRef, ok := getKeySetRef(ent).Get() - if !ok { - if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { - // If the entity has a resolved reference, but the spec has changed, we need to adjust the status. - if key.Status.Konnect != nil && key.Status.Konnect.GetKeySetID() != "" { - key.Status.Konnect.KeySetID = "" - if res, err := updateStatusWithCondition(ctx, cl, key, - conditions.KeySetRefValidConditionType, - metav1.ConditionTrue, - conditions.KeySetRefReasonValid, - "KeySetRef is nil", - ); err != nil || !res.IsZero() { - return res, err - } - } - } - return ctrl.Result{}, nil - } - - if keySetRef.Type != configurationv1alpha1.KeySetRefNamespacedRef { - return ctrl.Result{}, fmt.Errorf("unsupported KeySet ref type %q", keySetRef.Type) - } - - keySet := configurationv1alpha1.KongKeySet{} - nn := types.NamespacedName{ - Name: keySetRef.NamespacedRef.Name, - Namespace: ent.GetNamespace(), - } - - if err := cl.Get(ctx, nn, &keySet); err != nil { - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.KeySetRefValidConditionType, - metav1.ConditionFalse, - conditions.KeySetRefReasonInvalid, - err.Error(), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - // If it was not found, let's requeue. - if k8serrors.IsNotFound(err) { - return ctrl.Result{Requeue: true}, nil - } - - return ctrl.Result{}, fmt.Errorf("failed getting KongKeySet %s: %w", nn, err) - } - - // If referenced KongKeySet is being deleted, requeue. - if delTimestamp := keySet.GetDeletionTimestamp(); !delTimestamp.IsZero() { - return ctrl.Result{ - RequeueAfter: time.Until(delTimestamp.Time), - }, nil - } - - // Verify that the KongKeySet is programmed. - cond, ok := k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, &keySet) - if !ok || cond.Status != metav1.ConditionTrue { - if res, err := updateStatusWithCondition( - ctx, cl, ent, - conditions.KeySetRefValidConditionType, - metav1.ConditionFalse, - conditions.KeySetRefReasonInvalid, - fmt.Sprintf("Referenced KongKeySet %s is not programmed yet", nn), - ); err != nil || res.Requeue { - return ctrl.Result{}, err - } - return ctrl.Result{Requeue: true}, nil - } - - // TODO: make this generic. - // KongKeySet ID is not stored in KonnectEntityStatus because not all entities - // have a KeySetRef, hence the type constraints in the reconciler can't be used. - if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { - if key.Status.Konnect == nil { - key.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{} - } - key.Status.Konnect.KeySetID = keySet.Status.Konnect.GetKonnectID() - } - - if res, errStatus := updateStatusWithCondition( - ctx, cl, ent, - conditions.KeySetRefValidConditionType, - metav1.ConditionTrue, - conditions.KeySetRefReasonValid, - fmt.Sprintf("Referenced KongKeySet %s programmed", nn), - ); errStatus != nil || res.Requeue { - return res, errStatus - } - - return ctrl.Result{}, nil -} - func conditionMessageReferenceKonnectAPIAuthConfigurationInvalid(apiAuthRef types.NamespacedName) string { return fmt.Sprintf("referenced KonnectAPIAuthConfiguration %s is invalid", apiAuthRef) } diff --git a/controller/konnect/reconciler_keysetref.go b/controller/konnect/reconciler_keysetref.go new file mode 100644 index 000000000..eea444e0c --- /dev/null +++ b/controller/konnect/reconciler_keysetref.go @@ -0,0 +1,186 @@ +package konnect + +import ( + "context" + "fmt" + + "github.com/samber/mo" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + ctrllog "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/kong/gateway-operator/controller/konnect/conditions" + "github.com/kong/gateway-operator/controller/konnect/constraints" + k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" + + configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" + konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" +) + +// handleKongKeySetRef handles the KeySetRef for the given entity. +func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + ctx context.Context, + cl client.Client, + ent TEnt, +) (ctrl.Result, error) { + keySetRef, ok := getKeySetRef(ent).Get() + if !ok { + if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { + // If the entity has a resolved reference, but the spec has changed, we need to adjust the status + // and transfer the ownership back from the KeySet to the ControlPlane. + if key.Status.Konnect != nil && key.Status.Konnect.GetKeySetID() != "" { + // Reset the KeySetID in the status and set the condition to True. + key.Status.Konnect.KeySetID = "" + if res, err := updateStatusWithCondition(ctx, cl, key, + conditions.KeySetRefValidConditionType, + metav1.ConditionTrue, + conditions.KeySetRefReasonValid, + "KeySetRef is nil", + ); err != nil || !res.IsZero() { + return res, fmt.Errorf("failed to update status: %w", err) + } + + // Transfer the ownership back to the ControlPlane if it's resolved. + cpRef, hasCPRef := getControlPlaneRef(ent).Get() + if hasCPRef { + cp, err := getCPForRef(ctx, cl, cpRef, key.GetNamespace()) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to get ControlPlane: %w", err) + } + if res, err := passOwnershipExclusivelyTo(ctx, cl, key, cp); err != nil || !res.IsZero() { + return res, fmt.Errorf("failed to transfer ownership to ControlPlane: %w", err) + } + } + } + } + return ctrl.Result{}, nil + } + + if keySetRef.Type != configurationv1alpha1.KeySetRefNamespacedRef { + ctrllog.FromContext(ctx).Error(fmt.Errorf("unsupported KeySet ref type %q", keySetRef.Type), "entity", ent) + return ctrl.Result{}, nil + } + + keySet := configurationv1alpha1.KongKeySet{} + nn := types.NamespacedName{ + Name: keySetRef.NamespacedRef.Name, + Namespace: ent.GetNamespace(), + } + if err := cl.Get(ctx, nn, &keySet); err != nil { + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionFalse, + conditions.KeySetRefReasonInvalid, + err.Error(), + ); errStatus != nil || !res.IsZero() { + return res, errStatus + } + + // If the KongKeySet is not found, we don't want to requeue. + if k8serrors.IsNotFound(err) { + return ctrl.Result{}, ReferencedKongKeySetDoesNotExist{ + Reference: nn, + Err: err, + } + } + + return ctrl.Result{}, fmt.Errorf("failed getting KongKeySet %s: %w", nn, err) + } + + // If referenced KongKeySet is being deleted, return an error so that we can remove the entity from Konnect first. + if delTimestamp := keySet.GetDeletionTimestamp(); !delTimestamp.IsZero() { + return ctrl.Result{}, ReferencedKongKeySetIsBeingDeleted{ + Reference: nn, + DeletionTimestamp: delTimestamp.Time, + } + } + + // Verify that the KongKeySet is programmed. + cond, ok := k8sutils.GetCondition(conditions.KonnectEntityProgrammedConditionType, &keySet) + if !ok || cond.Status != metav1.ConditionTrue { + if res, err := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionFalse, + conditions.KeySetRefReasonInvalid, + fmt.Sprintf("Referenced KongKeySet %s is not programmed yet", nn), + ); err != nil || !res.IsZero() { + return ctrl.Result{}, err + } + return ctrl.Result{Requeue: true}, nil + } + + if res, err := passOwnershipExclusivelyTo(ctx, cl, ent, &keySet); err != nil || !res.IsZero() { + return res, err + } + + // TODO: make this generic. + // KongKeySet ID is not stored in KonnectEntityStatus because not all entities + // have a KeySetRef, hence the type constraints in the reconciler can't be used. + if key, ok := any(ent).(*configurationv1alpha1.KongKey); ok { + if key.Status.Konnect == nil { + key.Status.Konnect = &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{} + } + key.Status.Konnect.KeySetID = keySet.Status.Konnect.GetKonnectID() + } + + if res, errStatus := updateStatusWithCondition( + ctx, cl, ent, + conditions.KeySetRefValidConditionType, + metav1.ConditionTrue, + conditions.KeySetRefReasonValid, + fmt.Sprintf("Referenced KongKeySet %s programmed", nn), + ); errStatus != nil || res.Requeue { + return res, errStatus + } + + return ctrl.Result{}, nil +} + +func getKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + e TEnt, +) mo.Option[configurationv1alpha1.KeySetRef] { + switch e := any(e).(type) { + case *configurationv1alpha1.KongKey: + if e.Spec.KeySetRef == nil { + return mo.None[configurationv1alpha1.KeySetRef]() + } + return mo.Some(*e.Spec.KeySetRef) + default: + return mo.None[configurationv1alpha1.KeySetRef]() + } +} + +// passOwnershipExclusivelyTo transfers the ownership of the entity exclusively to the given owner, removing all other +// owner references. +func passOwnershipExclusivelyTo[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + ctx context.Context, + cl client.Client, + ent TEnt, + to metav1.Object, +) (ctrl.Result, error) { + old := ent.DeepCopyObject().(TEnt) + + // Cleanup the old owner references. + ent.SetOwnerReferences(nil) + + // Set the owner reference. + if err := controllerutil.SetOwnerReference(to, ent, cl.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to set owner reference: %w", err) + } + + // Patch the entity. + if err := cl.Patch(ctx, ent, client.MergeFrom(old)); err != nil { + if k8serrors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to patch owner references: %w", err) + } + + return ctrl.Result{}, nil +} diff --git a/controller/konnect/reconciler_keysetref_test.go b/controller/konnect/reconciler_keysetref_test.go new file mode 100644 index 000000000..e55dddda1 --- /dev/null +++ b/controller/konnect/reconciler_keysetref_test.go @@ -0,0 +1,370 @@ +package konnect + +import ( + "context" + "fmt" + "reflect" + "testing" + + "github.com/samber/lo" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/kong/gateway-operator/controller/konnect/conditions" + "github.com/kong/gateway-operator/controller/konnect/constraints" + + configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" + konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" +) + +type handleKeySetRefTestCase[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]] struct { + name string + ent TEnt + objects []client.Object + expectResult ctrl.Result + expectErrorContains string + // Returns true if the updated entity satisfy the assertion. + // Returns false and error message if entity fails to satisfy it. + updatedEntAssertions []func(TEnt) (ok bool, message string) +} + +func TestHandleKeySetRef(t *testing.T) { + // Test objects definitions. + var ( + commonKeyMeta = metav1.ObjectMeta{ + Name: "key-1", + Namespace: "ns", + } + + cp = &konnectv1alpha1.KonnectGatewayControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cp-1", + Namespace: "ns", + }, + } + cpRef = &configurationv1alpha1.ControlPlaneRef{ + Type: configurationv1alpha1.ControlPlaneRefKonnectNamespacedRef, + KonnectNamespacedRef: &configurationv1alpha1.KonnectNamespacedRef{ + Name: "cp-1", + }, + } + + notProgrammedKeySet = &configurationv1alpha1.KongKeySet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "key-set-1", + Namespace: "ns", + }, + Spec: configurationv1alpha1.KongKeySetSpec{ + ControlPlaneRef: cpRef, + KongKeySetAPISpec: configurationv1alpha1.KongKeySetAPISpec{ + Name: "key-set-1", + }, + }, + } + programmedKeySet = &configurationv1alpha1.KongKeySet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "key-set-2", + Namespace: "ns", + }, + Spec: configurationv1alpha1.KongKeySetSpec{ + ControlPlaneRef: cpRef, + KongKeySetAPISpec: configurationv1alpha1.KongKeySetAPISpec{ + Name: "key-set-2", + }, + }, + Status: configurationv1alpha1.KongKeySetStatus{ + Konnect: &konnectv1alpha1.KonnectEntityStatusWithControlPlaneRef{ + KonnectEntityStatus: konnectv1alpha1.KonnectEntityStatus{ + ID: "key-set-id", + }, + ControlPlaneID: "cp-id", + }, + Conditions: []metav1.Condition{ + { + Type: conditions.KonnectEntityProgrammedConditionType, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }, + }, + }, + } + keySetDuringDeletion = &configurationv1alpha1.KongKeySet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "key-set-3", + Namespace: "ns", + DeletionTimestamp: lo.ToPtr(metav1.Now()), + Finalizers: []string{ + KonnectCleanupFinalizer, + }, + }, + } + ) + + // Common assertions. + var ( + keySetIDIsEmpty = func(key *configurationv1alpha1.KongKey) (bool, string) { + if key.Status.Konnect != nil && key.Status.Konnect.KeySetID != "" { + return false, "KeySetID should be empty" + } + return true, "" + } + keySetIDIs = func(expectedID string) func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + return func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + if key.Status.Konnect == nil || key.Status.Konnect.KeySetID != expectedID { + return false, fmt.Sprintf("KeySetID should be %s", expectedID) + } + return true, "" + } + } + keySetRefConditionIs = func(expectedStatus metav1.ConditionStatus) func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + return func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + containsCondition := lo.ContainsBy(key.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == conditions.KeySetRefValidConditionType && + condition.Status == expectedStatus + }) + if !containsCondition { + return false, fmt.Sprintf("KeySetRefValid condition should be %s", expectedStatus) + } + return true, "" + } + } + hasExactlyOneOwnerReferenceTo = func(obj client.Object) func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + return func(key *configurationv1alpha1.KongKey) (ok bool, message string) { + if len(key.GetOwnerReferences()) != 1 { + return false, "KongKey should have exactly one owner reference" + } + + hasOwnerRef := lo.ContainsBy(key.GetOwnerReferences(), func(owner metav1.OwnerReference) bool { + return owner.Name == obj.GetName() && + reflect.TypeOf(obj).Elem().Name() == owner.Kind && + owner.BlockOwnerDeletion != nil && *owner.BlockOwnerDeletion + }) + if !hasOwnerRef { + return false, fmt.Sprintf("KongKey should have owner reference to %s", client.ObjectKeyFromObject(obj)) + } + return true, "" + } + } + ) + + testCases := []handleKeySetRefTestCase[configurationv1alpha1.KongKey, *configurationv1alpha1.KongKey]{ + { + name: "key set ref is nil", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: nil, + }, + }, + expectResult: ctrl.Result{}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIsEmpty, + }, + }, + { + name: "key set ref is nil but Konnect ID in status is set", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: nil, + }, + Status: configurationv1alpha1.KongKeyStatus{ + Konnect: &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{ + ControlPlaneID: "cp-id", + }, + }, + }, + expectResult: ctrl.Result{}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIsEmpty, + }, + }, + { + name: "key set ref points to non-existing key set", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: &configurationv1alpha1.KeySetNamespacedRef{ + Name: "key-set-1", + }, + }, + }, + }, + expectResult: ctrl.Result{}, + expectErrorContains: "keysets.configuration.konghq.com \"key-set-1\" not found", + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetRefConditionIs(metav1.ConditionFalse), + keySetIDIsEmpty, + }, + }, + { + name: "key set ref points to a key set during deletion", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: &configurationv1alpha1.KeySetNamespacedRef{ + Name: keySetDuringDeletion.Name, + }, + }, + }, + }, + objects: []client.Object{keySetDuringDeletion}, + expectResult: ctrl.Result{}, + expectErrorContains: "referenced KongKeySet ns/key-set-3 is being deleted", + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIsEmpty, + }, + }, + { + name: "key set ref points to a key set that is not programmed yet", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: &configurationv1alpha1.KeySetNamespacedRef{ + Name: notProgrammedKeySet.Name, + }, + }, + }, + }, + objects: []client.Object{notProgrammedKeySet}, + expectResult: ctrl.Result{Requeue: true}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIsEmpty, + keySetRefConditionIs(metav1.ConditionFalse), + }, + }, + { + name: "key set ref points to a programmed key set", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: &configurationv1alpha1.KeySetNamespacedRef{ + Name: programmedKeySet.Name, + }, + }, + }, + }, + objects: []client.Object{programmedKeySet}, + expectResult: ctrl.Result{}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetRefConditionIs(metav1.ConditionTrue), + keySetIDIs(programmedKeySet.Status.Konnect.ID), + hasExactlyOneOwnerReferenceTo(programmedKeySet), + }, + }, + { + name: "key set ref in spec changed to nil after resolving ref", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: commonKeyMeta, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: nil, + }, + Status: configurationv1alpha1.KongKeyStatus{ + Konnect: &konnectv1alpha1.KonnectEntityStatusWithControlPlaneAndKeySetRef{ + ControlPlaneID: "cp-id", + KeySetID: "key-set-id", + }, + }, + }, + expectResult: ctrl.Result{}, + objects: []client.Object{cp}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIsEmpty, + keySetRefConditionIs(metav1.ConditionTrue), + hasExactlyOneOwnerReferenceTo(&konnectv1alpha1.KonnectGatewayControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: cpRef.KonnectNamespacedRef.Name, + }, + }), + }, + }, + { + name: "when entity has owning reference to control plane and refers key set, the ownership should be transferred to key set", + ent: &configurationv1alpha1.KongKey{ + ObjectMeta: metav1.ObjectMeta{ + Name: "key-1", + Namespace: "ns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: konnectv1alpha1.GroupVersion.String(), + Kind: "KonnectGatewayControlPlane", + Name: cpRef.KonnectNamespacedRef.Name, + }, + }, + }, + Spec: configurationv1alpha1.KongKeySpec{ + ControlPlaneRef: cpRef, + KeySetRef: &configurationv1alpha1.KeySetRef{ + Type: configurationv1alpha1.KeySetRefNamespacedRef, + NamespacedRef: &configurationv1alpha1.KeySetNamespacedRef{ + Name: programmedKeySet.Name, + }, + }, + }, + }, + objects: []client.Object{programmedKeySet}, + expectResult: ctrl.Result{}, + updatedEntAssertions: []func(*configurationv1alpha1.KongKey) (ok bool, message string){ + keySetIDIs(programmedKeySet.Status.Konnect.ID), + keySetRefConditionIs(metav1.ConditionTrue), + hasExactlyOneOwnerReferenceTo(programmedKeySet), + }, + }, + } + testHandleKeySetRef(t, testCases) +} + +func testHandleKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constraints.EntityType[T]]( + t *testing.T, testCases []handleKeySetRefTestCase[T, TEnt]) { + t.Helper() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, configurationv1alpha1.AddToScheme(scheme)) + require.NoError(t, konnectv1alpha1.AddToScheme(scheme)) + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(tc.ent).WithObjects(tc.objects...). + // WithStatusSubresource is required for updating status of handled entity. + WithStatusSubresource(tc.ent).Build() + require.NoError(t, fakeClient.SubResource("status").Update(context.Background(), tc.ent)) + + res, err := handleKongKeySetRef(context.Background(), fakeClient, tc.ent) + + var updatedEnt = tc.ent.DeepCopyObject().(TEnt) + require.NoError(t, fakeClient.Get(context.Background(), client.ObjectKeyFromObject(tc.ent), updatedEnt)) + for _, assertion := range tc.updatedEntAssertions { + ok, msg := assertion(updatedEnt) + require.True(t, ok, msg) + } + + if len(tc.expectErrorContains) > 0 { + require.ErrorContains(t, err, tc.expectErrorContains) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expectResult, res) + }) + } +} diff --git a/controller/konnect/watch_kongkey.go b/controller/konnect/watch_kongkey.go index 5d7ea117c..fd4ef1e01 100644 --- a/controller/konnect/watch_kongkey.go +++ b/controller/konnect/watch_kongkey.go @@ -29,6 +29,14 @@ func KongKeyReconciliationWatchOptions(cl client.Client) []func(*ctrl.Builder) * ), ) }, + func(b *ctrl.Builder) *ctrl.Builder { + return b.Watches( + &configurationv1alpha1.KongKeySet{}, + handler.EnqueueRequestsFromMapFunc( + enqueueKongKeyForKongKeySet(cl), + ), + ) + }, func(b *ctrl.Builder) *ctrl.Builder { return b.Watches( &konnectv1alpha1.KonnectAPIAuthConfiguration{}, @@ -176,3 +184,43 @@ func enqueueKongKeyForKonnectControlPlane( return ret } } + +func enqueueKongKeyForKongKeySet(cl client.Client) handler.MapFunc { + return func(ctx context.Context, obj client.Object) []reconcile.Request { + keySet, ok := obj.(*configurationv1alpha1.KongKeySet) + if !ok { + return nil + } + var l configurationv1alpha1.KongKeyList + if err := cl.List(ctx, &l, &client.ListOptions{}); err != nil { + return nil + } + + var ret []reconcile.Request + for _, key := range l.Items { + keySetRef := getKeySetRef(&key) + ref, ok := keySetRef.Get() + if !ok { + continue + } + if ref.Type != configurationv1alpha1.KeySetRefNamespacedRef { + ctrllog.FromContext(ctx).V(logging.DebugLevel.Value()).Info( + "unsupported KongKeySetRef for KongKey", + "KongKey", key, "refType", ref.Type, + ) + continue + } + if ref.NamespacedRef == nil || ref.NamespacedRef.Name != keySet.Name { + continue + } + ret = append(ret, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: key.Namespace, + Name: key.Name, + }, + }) + } + + return ret + } +} diff --git a/test/envtest/konnect_entities_key_test.go b/test/envtest/konnect_entities_key_test.go index b953657ac..af150d414 100644 --- a/test/envtest/konnect_entities_key_test.go +++ b/test/envtest/konnect_entities_key_test.go @@ -132,7 +132,7 @@ func TestKongKey(t *testing.T) { }), } } - createdKey := deployKongKeyAttachedToCP(t, ctx, clientNamespaced, keyKid, keyName, cp, withKeySetRef) + createdKey := deploy.KongKeyAttachedToCP(t, ctx, clientNamespaced, keyKid, keyName, cp, withKeySetRef) t.Log("Waiting for KeySetRefValid condition to be false") watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { @@ -159,7 +159,7 @@ func TestKongKey(t *testing.T) { }, nil) t.Log("Creating KongKeySet") - keySet := deployKongKeySetAttachedToCP(t, ctx, clientNamespaced, keySetName, cp) + keySet := deploy.KongKeySetAttachedToCP(t, ctx, clientNamespaced, keySetName, cp) updateKongKeySetStatusWithProgrammed(t, ctx, clientNamespaced, keySet, keySetID, cp.GetKonnectStatus().GetKonnectID()) t.Log("Waiting for KongKey to be programmed and associated with KongKeySet") @@ -176,8 +176,10 @@ func TestKongKey(t *testing.T) { condition.Status == metav1.ConditionTrue }) keySetIDPopulated := c.Status.Konnect != nil && c.Status.Konnect.KeySetID != "" + exactlyOneOwnerReference := len(c.GetOwnerReferences()) == 1 + hasOwnerRefToKeySet := c.GetOwnerReferences()[0].Name == keySet.GetName() - return programmed && associated && keySetIDPopulated + return programmed && associated && keySetIDPopulated && exactlyOneOwnerReference && hasOwnerRefToKeySet }, "KongKey's Programmed and KeySetRefValid conditions should be true eventually") t.Log("Waiting for KongKey to be created in the SDK") @@ -196,6 +198,17 @@ func TestKongKey(t *testing.T) { keyToPatch.Spec.KeySetRef = nil require.NoError(t, clientNamespaced.Patch(ctx, keyToPatch, client.MergeFrom(createdKey))) + t.Log("Waiting for KongKey to be deattached from KongKeySet") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongKey) bool { + if c.GetName() != createdKey.GetName() { + return false + } + exactlyOneOwnerReference := len(c.GetOwnerReferences()) == 1 + hasOwnerReferenceToCP := c.GetOwnerReferences()[0].Name == cp.GetName() + + return exactlyOneOwnerReference && hasOwnerReferenceToCP + }, "KongKey should be deattached from KongKeySet eventually") + t.Log("Waiting for KongKey to be deattached from KongKeySet in the SDK") assert.EventuallyWithT(t, func(c *assert.CollectT) { assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) diff --git a/test/helpers/deploy/deploy_resources.go b/test/helpers/deploy/deploy_resources.go index 3e4ba88b2..ee5cdb705 100644 --- a/test/helpers/deploy/deploy_resources.go +++ b/test/helpers/deploy/deploy_resources.go @@ -627,6 +627,8 @@ func KongVaultAttachedToCP( return vault } +type kongKeyOption func(*configurationv1alpha1.KongKey) + // KongKeyAttachedToCP deploys a KongKey resource attached to a CP and returns the resource. func KongKeyAttachedToCP( t *testing.T, @@ -634,7 +636,7 @@ func KongKeyAttachedToCP( cl client.Client, kid, name string, cp *konnectv1alpha1.KonnectGatewayControlPlane, - opts ...objOption, + opts ...kongKeyOption, ) *configurationv1alpha1.KongKey { t.Helper() From 8647af29a5ec3ba2b0d851cd7b6e2d96dec2671a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Wed, 2 Oct 2024 16:16:40 +0200 Subject: [PATCH 3/5] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Patryk Małek --- controller/konnect/reconciler_keysetref.go | 2 +- test/envtest/konnect_entities_key_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/konnect/reconciler_keysetref.go b/controller/konnect/reconciler_keysetref.go index eea444e0c..f3631ecbe 100644 --- a/controller/konnect/reconciler_keysetref.go +++ b/controller/konnect/reconciler_keysetref.go @@ -39,7 +39,7 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai conditions.KeySetRefValidConditionType, metav1.ConditionTrue, conditions.KeySetRefReasonValid, - "KeySetRef is nil", + "KeySetRef is unset", ); err != nil || !res.IsZero() { return res, fmt.Errorf("failed to update status: %w", err) } diff --git a/test/envtest/konnect_entities_key_test.go b/test/envtest/konnect_entities_key_test.go index af150d414..908f699b1 100644 --- a/test/envtest/konnect_entities_key_test.go +++ b/test/envtest/konnect_entities_key_test.go @@ -88,7 +88,7 @@ func TestKongKey(t *testing.T) { }) }, "KongKey's Programmed condition should be true eventually") - t.Log("Waiting for KongKey to be created in the SDK") + t.Log("Checking SDK KongKey operations") require.EventuallyWithT(t, func(c *assert.CollectT) { assert.True(c, factory.SDK.KeysSDK.AssertExpectations(t)) }, waitTime, tickTime) From bac05c78bd68fb2dfc99875b08f4d1574b891258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Wed, 2 Oct 2024 16:50:48 +0200 Subject: [PATCH 4/5] use indices --- controller/konnect/index_kongkey.go | 56 ++++++++++++++++++++ controller/konnect/watch_kongkey.go | 81 +++++------------------------ modules/manager/controller_setup.go | 4 ++ 3 files changed, 74 insertions(+), 67 deletions(-) create mode 100644 controller/konnect/index_kongkey.go diff --git a/controller/konnect/index_kongkey.go b/controller/konnect/index_kongkey.go new file mode 100644 index 000000000..3e0e81fbb --- /dev/null +++ b/controller/konnect/index_kongkey.go @@ -0,0 +1,56 @@ +package konnect + +import ( + "sigs.k8s.io/controller-runtime/pkg/client" + + configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" +) + +const ( + // IndexFieldKongKeyOnKongKeySetReference is the index field for KongKey-> KongKeySet. + IndexFieldKongKeyOnKongKeySetReference = "kongKeySetRef" + + // IndexFieldKongKeyOnKonnectGatewayControlPlane is the index field for KongKey -> KonnectGatewayControlPlane. + IndexFieldKongKeyOnKonnectGatewayControlPlane = "kongKeyKonnectGatewayControlPlaneRef" +) + +// IndexOptionsForKongKey returns required Index options for KongKey reconclier. +func IndexOptionsForKongKey() []ReconciliationIndexOption { + return []ReconciliationIndexOption{ + { + IndexObject: &configurationv1alpha1.KongKey{}, + IndexField: IndexFieldKongKeyOnKongKeySetReference, + ExtractValue: kongKeySetRefFromKongKey, + }, + { + IndexObject: &configurationv1alpha1.KongKey{}, + IndexField: IndexFieldKongKeyOnKonnectGatewayControlPlane, + ExtractValue: konnectGatewayControlPlaneRefFromKongKey, + }, + } +} + +// kongKeySetRefFromKongKey returns namespace/name of referenced KongKeySet in KongKey spec. +func kongKeySetRefFromKongKey(obj client.Object) []string { + key, ok := obj.(*configurationv1alpha1.KongKey) + if !ok { + return nil + } + + if key.Spec.KeySetRef == nil || + key.Spec.KeySetRef.Type != configurationv1alpha1.KeySetRefNamespacedRef || + key.Spec.KeySetRef.NamespacedRef == nil { + return nil + } + + return []string{key.GetNamespace() + "/" + key.Spec.KeySetRef.NamespacedRef.Name} +} + +// kongPluginReferencesFromKongKey returns namespace/name of referenced KonnectGatewayControlPlane in KongKey spec. +func konnectGatewayControlPlaneRefFromKongKey(obj client.Object) []string { + key, ok := obj.(*configurationv1alpha1.KongKey) + if !ok { + return nil + } + return controlPlaneKonnectNamespacedRefAsSlice(key) +} diff --git a/controller/konnect/watch_kongkey.go b/controller/konnect/watch_kongkey.go index fd4ef1e01..91e1c03bb 100644 --- a/controller/konnect/watch_kongkey.go +++ b/controller/konnect/watch_kongkey.go @@ -138,50 +138,17 @@ func enqueueKongKeyForKonnectControlPlane( return nil } var l configurationv1alpha1.KongKeyList - if err := cl.List(ctx, &l, &client.ListOptions{ + if err := cl.List(ctx, &l, // TODO: change this when cross namespace refs are allowed. - Namespace: cp.GetNamespace(), - }); err != nil { + client.InNamespace(cp.GetNamespace()), + client.MatchingFields{ + IndexFieldKongKeyOnKonnectGatewayControlPlane: cp.GetNamespace() + "/" + cp.GetName(), + }, + ); err != nil { return nil } - var ret []reconcile.Request - for _, key := range l.Items { - cpRef, ok := getControlPlaneRef(&key).Get() - if !ok { - continue - } - switch cpRef.Type { - case configurationv1alpha1.ControlPlaneRefKonnectNamespacedRef: - // TODO: change this when cross namespace refs are allowed. - if cpRef.KonnectNamespacedRef.Name != cp.Name { - continue - } - - ret = append(ret, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: key.Namespace, - Name: key.Name, - }, - }) - - case configurationv1alpha1.ControlPlaneRefKonnectID: - ctrllog.FromContext(ctx).Error( - fmt.Errorf("unimplemented ControlPlaneRef type %q", cpRef.Type), - "unimplemented ControlPlaneRef for KongKey", - "KongKey", key, "refType", cpRef.Type, - ) - continue - - default: - ctrllog.FromContext(ctx).V(logging.DebugLevel.Value()).Info( - "unsupported ControlPlaneRef for KongKey", - "KongKey", key, "refType", cpRef.Type, - ) - continue - } - } - return ret + return objectListToReconcileRequests(l.Items) } } @@ -192,35 +159,15 @@ func enqueueKongKeyForKongKeySet(cl client.Client) handler.MapFunc { return nil } var l configurationv1alpha1.KongKeyList - if err := cl.List(ctx, &l, &client.ListOptions{}); err != nil { + if err := cl.List(ctx, &l, + client.InNamespace(keySet.GetNamespace()), + client.MatchingFields{ + IndexFieldKongKeyOnKongKeySetReference: keySet.GetNamespace() + "/" + keySet.GetName(), + }, + ); err != nil { return nil } - var ret []reconcile.Request - for _, key := range l.Items { - keySetRef := getKeySetRef(&key) - ref, ok := keySetRef.Get() - if !ok { - continue - } - if ref.Type != configurationv1alpha1.KeySetRefNamespacedRef { - ctrllog.FromContext(ctx).V(logging.DebugLevel.Value()).Info( - "unsupported KongKeySetRef for KongKey", - "KongKey", key, "refType", ref.Type, - ) - continue - } - if ref.NamespacedRef == nil || ref.NamespacedRef.Name != keySet.Name { - continue - } - ret = append(ret, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: key.Namespace, - Name: key.Name, - }, - }) - } - - return ret + return objectListToReconcileRequests(l.Items) } } diff --git a/modules/manager/controller_setup.go b/modules/manager/controller_setup.go index f343ac93e..8905beb99 100644 --- a/modules/manager/controller_setup.go +++ b/modules/manager/controller_setup.go @@ -616,6 +616,10 @@ func SetupCacheIndicesForKonnectTypes(ctx context.Context, mgr manager.Manager, Object: &configurationv1alpha1.KongSNI{}, IndexOptions: konnect.IndexOptionsForKongSNI(), }, + { + Object: &configurationv1alpha1.KongKey{}, + IndexOptions: konnect.IndexOptionsForKongKey(), + }, } for _, t := range types { From 643d89a7f22e212d0f63ce0fc5c933cc7159a3d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Wed, 2 Oct 2024 16:55:27 +0200 Subject: [PATCH 5/5] add comment --- controller/konnect/reconciler_keysetref.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/controller/konnect/reconciler_keysetref.go b/controller/konnect/reconciler_keysetref.go index f3631ecbe..35414852c 100644 --- a/controller/konnect/reconciler_keysetref.go +++ b/controller/konnect/reconciler_keysetref.go @@ -115,6 +115,11 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai return ctrl.Result{Requeue: true}, nil } + // Transfer the ownership of the entity exclusively to the KongKeySet to make sure it will get garbage collected + // when the KongKeySet is deleted. This is to follow the behavior on the Konnect API that deletes KongKeys associated + // with a KongKeySet once it's deleted. + // The ownership needs to be transferred *exclusively* to the KongKeySet because a Kubernetes object gets garbage + // collected only when all its owner references are removed. if res, err := passOwnershipExclusivelyTo(ctx, cl, ent, &keySet); err != nil || !res.IsZero() { return res, err }