Fix stale definitions files on ClusterAccess update/delete (#82)

* ensured that we have ClusterAccess CRD installed

* wait for CRD to be fully registered

* addressed comments

* removed TOCTOU race

* fix(deps): update module sigs.k8s.io/controller-runtime to v0.22.2 (#67)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module golang.org/x/text to v0.30.0 (#70)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix: relations nil pointer check (#68)

* added nil checks in relation resovling

On-behalf-of: @SAP [email protected]
Signed-off-by: Artem Shcherbatiuk <[email protected]>

* fix(deps): update golang.org/x/exp digest to d2f985d (#71)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* improved docs

* removed testing script due for the simplicity of support

* feat: cluster access definitions deletion (#69)

* feat: refine cluster access definitions deletion (#69)

* feat: add tests for cluster access definitions deletion and edit (#69)

* fix: use the observed path for path changes (#69)

* fix: fix the finalizerName value (#69)

---------

Signed-off-by: Artem Shcherbatiuk <[email protected]>
Co-authored-by: Artem Shcherbatiuk <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Author: 3 people

common/apis/v1alpha1/clusteraccess_types.go

@@ -83,6 +83,11 @@ type ClusterAccessStatus struct {
 	// Conditions represent the latest available observations of the cluster access state
 	// +optional
 	Conditions []metav1.Condition `json:"conditions,omitempty"`
+
+	// ObservedPath stores the actual path currently used to store the schema for this resource
+	// This is maintained by the controller and used for cleanup when .spec.path changes
+	// +optional
+	ObservedPath string `json:"observedPath,omitempty"`
 }
 
 type ServiceAccountRef struct {

config/crd/gateway.platform-mesh.io_clusteraccesses.yaml

@@ -197,6 +197,11 @@ spec:
                   - type
                   type: object
                 type: array
+              observedPath:
+                description: |-
+                  ObservedPath stores the actual path currently used to store the schema for this resource
+                  This is maintained by the controller and used for cleanup when .spec.path changes
+                type: string
             type: object
         type: object
     served: true

listener/reconciler/clusteraccess/subroutines.go

@@ -17,6 +17,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 )
 
+// generateSchemaSubroutine processes ClusterAccess resources and generates schemas
+const (
+	finalizerName = "gateway.platform-mesh.io/clusteraccess-finalizer"
+)
+
 // generateSchemaSubroutine processes ClusterAccess resources and generates schemas
 type generateSchemaSubroutine struct {
 	reconciler *ClusterAccessReconciler
@@ -72,12 +77,33 @@ func (s *generateSchemaSubroutine) Process(ctx context.Context, instance runtime
 		return ctrl.Result{}, commonserrors.NewOperatorError(err, false, false)
 	}
 
+	// If path changed, delete the old schema file referenced in the status
+	prevPath := clusterAccess.Status.ObservedPath
+	if prevPath != "" && prevPath != clusterName {
+		if err := s.reconciler.ioHandler.Delete(prevPath); err != nil {
+			// Log and continue; do not fail reconciliation on cleanup issues
+			s.reconciler.log.Warn().Err(err).Str("previousPath", prevPath).Str("clusterAccess", clusterAccessName).Msg("failed to delete previous schema file")
+		}
+	}
+
 	// Write schema to file using cluster name from path or resource name
 	if err := s.reconciler.ioHandler.Write(schemaWithMetadata, clusterName); err != nil {
 		s.reconciler.log.Error().Err(err).Str("clusterAccess", clusterAccessName).Msg("failed to write schema")
 		return ctrl.Result{}, commonserrors.NewOperatorError(err, false, false)
 	}
 
+	// Update status.ObservedPath to reflect the current observed path
+	if prevPath != clusterName {
+		obj := clusterAccess.DeepCopy()
+		obj.Status.ObservedPath = clusterName
+		if err := s.reconciler.opts.Client.Status().Update(ctx, obj); err != nil {
+			// Log but do not fail reconciliation; file has been written already
+			s.reconciler.log.Warn().Err(err).Str("clusterAccess", clusterAccessName).Msg("failed to update observed path in status")
+		} else {
+			clusterAccess.Status.ObservedPath = obj.Status.ObservedPath
+		}
+	}
+
 	s.reconciler.log.Info().Str("clusterAccess", clusterAccessName).Msg("successfully processed ClusterAccess resource")
 	return ctrl.Result{}, nil
 }
@@ -97,6 +123,33 @@ func (s *generateSchemaSubroutine) restMapperFromConfig(cfg *rest.Config) (meta.
 }
 
 func (s *generateSchemaSubroutine) Finalize(ctx context.Context, instance runtimeobject.RuntimeObject) (ctrl.Result, commonserrors.OperatorError) {
+	clusterAccess, ok := instance.(*gatewayv1alpha1.ClusterAccess)
+	if !ok {
+		s.reconciler.log.Error().Msg("instance is not a ClusterAccess resource in Finalize")
+		return ctrl.Result{}, commonserrors.NewOperatorError(errors.New("invalid resource type"), false, false)
+	}
+
+	// Determine current and previously used paths
+	currentPath := clusterAccess.Spec.Path
+	if currentPath == "" {
+		currentPath = clusterAccess.GetName()
+	}
+	prevPath := clusterAccess.Status.ObservedPath
+
+	// Try deleting current path file
+	if currentPath != "" {
+		if err := s.reconciler.ioHandler.Delete(currentPath); err != nil {
+			// Log and continue; do not block finalization just because file was missing or deletion failed
+			s.reconciler.log.Warn().Err(err).Str("path", currentPath).Str("clusterAccess", clusterAccess.GetName()).Msg("failed to delete schema file during finalization")
+		}
+	}
+	// If previous differs, try deleting it as well
+	if prevPath != "" && prevPath != currentPath {
+		if err := s.reconciler.ioHandler.Delete(prevPath); err != nil {
+			s.reconciler.log.Warn().Err(err).Str("path", prevPath).Str("clusterAccess", clusterAccess.GetName()).Msg("failed to delete previous schema file during finalization")
+		}
+	}
+
 	return ctrl.Result{}, nil
 }
 
@@ -105,5 +158,5 @@ func (s *generateSchemaSubroutine) GetName() string {
 }
 
 func (s *generateSchemaSubroutine) Finalizers() []string {
-	return nil
+	return []string{finalizerName}
 }

listener/reconciler/clusteraccess/subroutines_test.go

@@ -0,0 +1,122 @@
+package clusteraccess
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/rest"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/platform-mesh/golang-commons/logger"
+	gatewayv1alpha1 "github.com/platform-mesh/kubernetes-graphql-gateway/common/apis/v1alpha1"
+	workspacefile_mocks "github.com/platform-mesh/kubernetes-graphql-gateway/listener/pkg/workspacefile/mocks"
+	"github.com/platform-mesh/kubernetes-graphql-gateway/listener/reconciler"
+)
+
+func TestGenerateSchemaSubroutine_Process_InvalidResourceType(t *testing.T) {
+	mockIO := workspacefile_mocks.NewMockIOHandler(t)
+	log, _ := logger.New(logger.DefaultConfig())
+
+	r := &ClusterAccessReconciler{
+		ioHandler: mockIO,
+		log:       log,
+	}
+	s := &generateSchemaSubroutine{reconciler: r}
+
+	_, opErr := s.Process(context.Background(), &metav1.PartialObjectMetadata{})
+
+	assert.NotNil(t, opErr)
+}
+
+func TestGenerateSchemaSubroutine_Process_MissingHostReturnsError(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = gatewayv1alpha1.AddToScheme(scheme)
+
+	ca := &gatewayv1alpha1.ClusterAccess{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "test-cluster",
+			Annotations: map[string]string{},
+		},
+		Spec: gatewayv1alpha1.ClusterAccessSpec{
+			// Host is intentionally empty to trigger validation error
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(ca).Build()
+
+	mockIO := workspacefile_mocks.NewMockIOHandler(t)
+	mockIO.EXPECT().Write(mock.Anything, mock.Anything).Maybe().Return(nil)
+	mockIO.EXPECT().Delete(mock.Anything).Maybe().Return(nil)
+
+	log, _ := logger.New(logger.DefaultConfig())
+
+	r := &ClusterAccessReconciler{
+		ioHandler: mockIO,
+		log:       log,
+		opts: reconciler.ReconcilerOpts{
+			Client:      fakeClient,
+			Config:      &rest.Config{Host: "https://unit-test.invalid"},
+			ManagerOpts: ctrl.Options{Scheme: scheme},
+			Scheme:      scheme,
+		},
+	}
+	s := &generateSchemaSubroutine{reconciler: r}
+
+	res, opErr := s.Process(context.Background(), ca)
+
+	assert.NotNil(t, opErr)
+	assert.Equal(t, ctrl.Result{}, res)
+}
+
+func TestGenerateSchemaSubroutine_Finalize_DeletesCurrentAndPreviousPaths(t *testing.T) {
+	mockIO := workspacefile_mocks.NewMockIOHandler(t)
+	log, _ := logger.New(logger.DefaultConfig())
+
+	// Expect deletion of both current and previous paths
+	mockIO.EXPECT().Delete("current-path").Return(nil).Once()
+	mockIO.EXPECT().Delete("previous-path").Return(nil).Once()
+
+	r := &ClusterAccessReconciler{
+		ioHandler: mockIO,
+		log:       log,
+	}
+	s := &generateSchemaSubroutine{reconciler: r}
+
+	ca := &gatewayv1alpha1.ClusterAccess{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-resource",
+		},
+		Spec: gatewayv1alpha1.ClusterAccessSpec{
+			Path: "current-path",
+		},
+		Status: gatewayv1alpha1.ClusterAccessStatus{
+			ObservedPath: "previous-path",
+		},
+	}
+
+	res, opErr := s.Finalize(context.Background(), ca)
+
+	assert.Nil(t, opErr)
+	assert.Equal(t, ctrl.Result{}, res)
+}
+
+func TestGenerateSchemaSubroutine_restMapperFromConfig_SucceedsWithMinimalConfig(t *testing.T) {
+	mockIO := workspacefile_mocks.NewMockIOHandler(t)
+	log, _ := logger.New(logger.DefaultConfig())
+
+	r := &ClusterAccessReconciler{
+		ioHandler: mockIO,
+		log:       log,
+	}
+	s := &generateSchemaSubroutine{reconciler: r}
+
+	rm, err := s.restMapperFromConfig(&rest.Config{})
+
+	assert.NotNil(t, rm)
+	assert.NoError(t, err)
+}