From 49f32e5bba610947da648483d407135f4fe9c928 Mon Sep 17 00:00:00 2001 From: Sergiy Kulanov Date: Tue, 30 Jun 2026 16:24:30 +0300 Subject: [PATCH] EPMDEDP-17146: fix: garbage-collect GitServer credentials Secret on deletion The GitServer reconciler now sets the GitServer as the controller owner of its credentials Secret (spec.nameSshKeySecret). Kubernetes then garbage-collects the Secret whenever the GitServer is deleted, regardless of the deletion path (portal UI, kubectl, or CLI). Previously the native K8s Secret was orphaned, which made recreating a Git integration with the same name fail with "resource already exists". Ownership is skipped when: - the Secret does not exist yet (set on a later reconcile, not a failure); - the Secret is already controlled by another resource (e.g. an ExternalSecret managed by the External Secrets Operator), or by this GitServer already; - the Secret is referenced by more than one GitServer, so deleting one does not garbage-collect credentials still in use by the others. Signed-off-by: Sergiy Kulanov --- controllers/gitserver/gitserver_controller.go | 66 +++++- .../gitserver/gitserver_controller_test.go | 218 +++++++++++++++++- 2 files changed, 280 insertions(+), 4 deletions(-) diff --git a/controllers/gitserver/gitserver_controller.go b/controllers/gitserver/gitserver_controller.go index 51349507..2722d463 100644 --- a/controllers/gitserver/gitserver_controller.go +++ b/controllers/gitserver/gitserver_controller.go @@ -7,10 +7,12 @@ import ( coreV1 "k8s.io/api/core/v1" 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/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" codebaseApi "github.com/epam/edp-codebase-operator/v2/api/v1" @@ -49,7 +51,7 @@ func (r *ReconcileGitServer) SetupWithManager(mgr ctrl.Manager) error { // +kubebuilder:rbac:groups=v2.edp.epam.com,namespace=placeholder,resources=gitservers,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=v2.edp.epam.com,namespace=placeholder,resources=gitservers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=v2.edp.epam.com,namespace=placeholder,resources=gitservers/finalizers,verbs=update -// +kubebuilder:rbac:groups="",namespace=placeholder,resources=secrets,verbs=get;list;watch +// +kubebuilder:rbac:groups="",namespace=placeholder,resources=secrets,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups="networking.k8s.io",namespace=placeholder,resources=ingresses,verbs=get;list;watch;create // +kubebuilder:rbac:groups="route.openshift.io",namespace=placeholder,resources=routes,verbs=get;list;watch;create @@ -70,6 +72,19 @@ func (r *ReconcileGitServer) Reconcile(ctx context.Context, request reconcile.Re oldStatus := instance.Status gitServer := model.ConvertToGitServer(instance) + if err := r.ensureSecretOwnership(ctx, instance); err != nil { + instance.Status.SetFailed(err.Error()) + instance.Status.Connected = false + + if statusErr := r.updateGitServerStatus(ctx, instance, oldStatus); statusErr != nil { + return reconcile.Result{}, statusErr + } + + log.Error(err, "Failed to set owner reference on Git Server secret") + + return reconcile.Result{RequeueAfter: defaultRequeueTime}, nil + } + if err := r.checkConnectionToGitServer(ctx, gitServer); err != nil { instance.Status.SetFailed(err.Error()) instance.Status.Connected = false @@ -110,6 +125,55 @@ func (r *ReconcileGitServer) Reconcile(ctx context.Context, request reconcile.Re }, nil } +// ensureSecretOwnership makes the GitServer the controller owner of its credentials Secret so +// that Kubernetes garbage-collects the Secret when the GitServer is deleted, regardless of the +// deletion path (portal UI, kubectl, CLI). +func (r *ReconcileGitServer) ensureSecretOwnership(ctx context.Context, gitServer *codebaseApi.GitServer) error { + secret := &coreV1.Secret{} + + if err := r.client.Get(ctx, types.NamespacedName{ + Namespace: gitServer.Namespace, + Name: gitServer.Spec.NameSshKeySecret, + }, secret); err != nil { + if k8sErrors.IsNotFound(err) { + // The secret hasn't been provisioned yet; a later reconcile will set ownership. + return nil + } + + return fmt.Errorf("failed to get secret %s: %w", gitServer.Spec.NameSshKeySecret, err) + } + + // Leave the secret untouched if it already has a controller owner: either this GitServer + // (idempotent re-reconcile) or another resource such as an ExternalSecret. + if controllerRef := metaV1.GetControllerOf(secret); controllerRef != nil { + return nil + } + + // Don't take exclusive ownership of a secret referenced by several GitServers, otherwise + // deleting one of them would garbage-collect credentials still in use by the others. + gitServers := &codebaseApi.GitServerList{} + if err := r.client.List(ctx, gitServers, client.InNamespace(gitServer.Namespace)); err != nil { + return fmt.Errorf("failed to list GitServers in namespace %s: %w", gitServer.Namespace, err) + } + + for i := range gitServers.Items { + if gitServers.Items[i].Name != gitServer.Name && + gitServers.Items[i].Spec.NameSshKeySecret == gitServer.Spec.NameSshKeySecret { + return nil + } + } + + if err := controllerutil.SetControllerReference(gitServer, secret, r.client.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference on secret %s: %w", secret.Name, err) + } + + if err := r.client.Update(ctx, secret); err != nil { + return fmt.Errorf("failed to update secret %s with owner reference: %w", secret.Name, err) + } + + return nil +} + func (r *ReconcileGitServer) checkConnectionToGitServer(ctx context.Context, gitServer *model.GitServer) error { log := ctrl.LoggerFrom(ctx) log.Info("Start CheckConnectionToGitServer method", "host", gitServer.GitHost) diff --git a/controllers/gitserver/gitserver_controller_test.go b/controllers/gitserver/gitserver_controller_test.go index fd5bcfee..86a29968 100644 --- a/controllers/gitserver/gitserver_controller_test.go +++ b/controllers/gitserver/gitserver_controller_test.go @@ -2,6 +2,7 @@ package gitserver import ( "context" + "errors" "strings" "testing" "time" @@ -16,6 +17,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" codebaseApi "github.com/epam/edp-codebase-operator/v2/api/v1" @@ -231,9 +233,8 @@ func TestReconcileGitServer_ServerUnavailable(t *testing.T) { }, } scheme := runtime.NewScheme() - scheme.AddKnownTypes(codebaseApi.GroupVersion, gs) - err := corev1.AddToScheme(scheme) - assert.NoError(t, err) + require.NoError(t, codebaseApi.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) secret := corev1.Secret{ ObjectMeta: metaV1.ObjectMeta{Name: "ssh-secret", Namespace: gs.Namespace}, @@ -393,6 +394,217 @@ func TestReconcileGitServer_EmptySSHKey(t *testing.T) { assert.True(t, gotGitServer.Status.IsSuccess()) } +func TestReconcileGitServer_EnsureSecretOwnership(t *testing.T) { + t.Parallel() + + const ( + ns = "namespace" + targetName = "git-server" + ) + + gitServer := func(name, secretName string) *codebaseApi.GitServer { + return &codebaseApi.GitServer{ + ObjectMeta: metaV1.ObjectMeta{Name: name, Namespace: ns, UID: types.UID(name + "-uid")}, + Spec: codebaseApi.GitServerSpec{GitHost: "g-host", NameSshKeySecret: secretName}, + } + } + secret := func(name string, owners ...metaV1.OwnerReference) *corev1.Secret { + return &corev1.Secret{ObjectMeta: metaV1.ObjectMeta{Name: name, Namespace: ns, OwnerReferences: owners}} + } + gsOwner := func(name string) metaV1.OwnerReference { + controller := true + return metaV1.OwnerReference{ + APIVersion: codebaseApi.GroupVersion.String(), + Kind: "GitServer", + Name: name, + UID: types.UID(name + "-uid"), + Controller: &controller, + } + } + extOwner := func() metaV1.OwnerReference { + controller := true + return metaV1.OwnerReference{ + APIVersion: "external-secrets.io/v1beta1", + Kind: "ExternalSecret", + Name: "ssh-secret", + UID: "external-secret-uid", + Controller: &controller, + } + } + + tests := []struct { + name string + secretRef string + // objects returns the fresh fixtures for the subtest, including the target GitServer. + objects func() []client.Object + interceptor interceptor.Funcs + wantErr bool + verify func(t *testing.T, secret *corev1.Secret, getErr error) + }{ + { + name: "adopts an unowned secret", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret")} }, + verify: func(t *testing.T, s *corev1.Secret, getErr error) { + require.NoError(t, getErr) + require.Len(t, s.OwnerReferences, 1) + assert.Equal(t, "GitServer", s.OwnerReferences[0].Kind) + assert.Equal(t, targetName, s.OwnerReferences[0].Name) + require.NotNil(t, s.OwnerReferences[0].Controller) + assert.True(t, *s.OwnerReferences[0].Controller) + }, + }, + { + name: "leaves a secret controlled by another resource untouched", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret", extOwner())} }, + verify: func(t *testing.T, s *corev1.Secret, getErr error) { + require.NoError(t, getErr) + require.Len(t, s.OwnerReferences, 1) + assert.Equal(t, "ExternalSecret", s.OwnerReferences[0].Kind) + }, + }, + { + name: "is idempotent when already owned by this GitServer", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret", gsOwner(targetName))} }, + verify: func(t *testing.T, s *corev1.Secret, getErr error) { + require.NoError(t, getErr) + require.Len(t, s.OwnerReferences, 1) + assert.Equal(t, targetName, s.OwnerReferences[0].Name) + }, + }, + { + name: "treats a not-yet-provisioned secret as a no-op", + secretRef: "missing-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "missing-secret")} }, + verify: func(t *testing.T, _ *corev1.Secret, getErr error) { + require.Error(t, getErr) // the secret was never created + }, + }, + { + name: "does not exclusively own a secret shared by multiple GitServers", + secretRef: "ssh-secret", + objects: func() []client.Object { + return []client.Object{gitServer(targetName, "ssh-secret"), gitServer("other-git-server", "ssh-secret"), secret("ssh-secret")} + }, + verify: func(t *testing.T, s *corev1.Secret, getErr error) { + require.NoError(t, getErr) + assert.Empty(t, s.OwnerReferences) + }, + }, + { + name: "returns an error when reading the secret fails", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret")} }, + interceptor: interceptor.Funcs{ + Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return errors.New("get failed") + } + return nil + }, + }, + wantErr: true, + }, + { + name: "returns an error when listing GitServers fails", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret")} }, + interceptor: interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return errors.New("list failed") + }, + }, + wantErr: true, + }, + { + name: "returns an error when updating the secret fails", + secretRef: "ssh-secret", + objects: func() []client.Object { return []client.Object{gitServer(targetName, "ssh-secret"), secret("ssh-secret")} }, + interceptor: interceptor.Funcs{ + Update: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.UpdateOption) error { + return errors.New("update failed") + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, codebaseApi.AddToScheme(scheme)) + + objects := tt.objects() + target := objects[0].(*codebaseApi.GitServer) + + fakeCl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + WithInterceptorFuncs(tt.interceptor). + Build() + r := ReconcileGitServer{client: fakeCl} + + err := r.ensureSecretOwnership(context.Background(), target) + + if tt.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + + gotSecret := &corev1.Secret{} + getErr := fakeCl.Get(context.Background(), types.NamespacedName{Name: tt.secretRef, Namespace: ns}, gotSecret) + tt.verify(t, gotSecret, getErr) + }) + } +} + +func TestReconcileGitServer_Reconcile_SecretOwnershipFailureSetsFailedStatus(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, codebaseApi.AddToScheme(scheme)) + + gs := &codebaseApi.GitServer{ + ObjectMeta: metaV1.ObjectMeta{Name: "git-server", Namespace: "namespace"}, + Spec: codebaseApi.GitServerSpec{GitHost: "g-host", NameSshKeySecret: "ssh-secret"}, + Status: codebaseApi.GitServerStatus{Connected: true}, + } + secret := &corev1.Secret{ObjectMeta: metaV1.ObjectMeta{Name: "ssh-secret", Namespace: gs.Namespace}} + + fakeCl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(gs, secret). + WithStatusSubresource(gs). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return errors.New("list failed") + }, + }). + Build() + + r := ReconcileGitServer{client: fakeCl} + + res, err := r.Reconcile(ctrl.LoggerInto(context.Background(), logr.Discard()), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: gs.Name, Namespace: gs.Namespace}, + }) + + require.NoError(t, err) + assert.Equal(t, defaultRequeueTime, res.RequeueAfter) + + gotGitServer := &codebaseApi.GitServer{} + require.NoError(t, fakeCl.Get(context.Background(), types.NamespacedName{Name: gs.Name, Namespace: gs.Namespace}, gotGitServer)) + assert.Equal(t, "failed", gotGitServer.Status.Status) + assert.False(t, gotGitServer.Status.Connected) +} + func TestNewReconcileGitServer(t *testing.T) { t.Parallel()