refactor(clickhouseuser): introduce generic reconciler for ClickhouseUser

Author: ndk

.mockery.yml

@@ -0,0 +1,13 @@
+dir: controllers
+filename: mocks_test.go
+formatter: goimports
+structname: "{{.Mock}}{{.InterfaceName}}"
+pkgname: "{{.SrcPackageName}}"
+recursive: false
+require-template-schema-exists: true
+template: testify
+template-schema: "{{.Template}}.schema.json"
+packages:
+  github.com/aiven/aiven-operator/controllers:
+    config:
+      all: true

controllers/basic_controller.go

@@ -49,7 +49,6 @@ type (
 		Scheme          *runtime.Scheme
 		Recorder        record.EventRecorder
 		DefaultToken    string
-		AvGenClient     avngen.Client
 		KubeVersion     string
 		OperatorVersion string
 	}
@@ -104,6 +103,7 @@ const (
 	eventWaitingForTheInstanceToBeRunning   = "WaitingForInstanceToBeRunning"
 	eventUnableToWaitForInstanceToBeRunning = "UnableToWaitForInstanceToBeRunning"
 	eventInstanceIsRunning                  = "InstanceIsRunning"
+	eventUnableToSyncConnectionSecret       = "UnableToSyncConnectionSecret"
 	eventConnInfoSecretCreationDisabled     = "ConnInfoSecretCreationDisabled"
 )
 

controllers/clickhouseuser_controller_v2.go

@@ -0,0 +1,232 @@
+// Copyright (c) 2024 Aiven, Helsinki, Finland. https://aiven.io/
+
+package controllers
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"strconv"
+
+	avngen "github.com/aiven/go-client-codegen"
+	"github.com/aiven/go-client-codegen/handler/clickhouse"
+	"github.com/aiven/go-client-codegen/handler/service"
+	"github.com/go-logr/logr"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/aiven/aiven-operator/api/v1alpha1"
+)
+
+//+kubebuilder:rbac:groups=aiven.io,resources=clickhouseusers,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=aiven.io,resources=clickhouseusers/status,verbs=get;update;patch
+//+kubebuilder:rbac:groups=aiven.io,resources=clickhouseusers/finalizers,verbs=get;create;update
+
+// ClickhouseUserControllerV2 reconciles a ClickhouseUser object
+type ClickhouseUserControllerV2 struct {
+	client.Client
+	avnGen avngen.Client
+}
+
+func newClickhouseUserReconcilerV2(c Controller) reconcilerType {
+	return &Reconciler[*v1alpha1.ClickhouseUser]{
+		Controller:              c,
+		newAivenGeneratedClient: NewAivenGeneratedClient,
+		newObj: func() *v1alpha1.ClickhouseUser {
+			return &v1alpha1.ClickhouseUser{}
+		},
+		newController: func(avnGen avngen.Client) AivenController[*v1alpha1.ClickhouseUser] {
+			return &ClickhouseUserControllerV2{
+				Client: c.Client,
+				avnGen: avnGen,
+			}
+		},
+	}
+}
+
+func (r *ClickhouseUserControllerV2) Observe(ctx context.Context, user *v1alpha1.ClickhouseUser) (Observation, error) {
+	obs := Observation{
+		ResourceExists:   false,
+		ResourceUpToDate: false,
+		SecretDetails:    nil,
+	}
+
+	check, err := checkServiceIsOperational(ctx, r.avnGen, user.Spec.Project, user.Spec.ServiceName)
+	if err != nil {
+		if errors.Is(err, errServicePoweredOff) {
+			return obs, fmt.Errorf("%w: %w", errPreconditionNotMet, err)
+		}
+		return obs, err
+	}
+	if !check {
+		return obs, errPreconditionNotMet
+	}
+
+	list, err := r.avnGen.ServiceClickHouseUserList(ctx, user.Spec.Project, user.Spec.ServiceName)
+	if err != nil {
+		return obs, err
+	}
+	for _, u := range list {
+		if u.Name == user.GetUsername() {
+			obs.ResourceExists = true
+			user.Status.UUID = u.Uuid
+			break
+		}
+	}
+
+	if obs.ResourceExists {
+		// TODO: extend the logic with more checks if needed
+		obs.ResourceUpToDate = IsReadyToUse(user)
+	}
+
+	return obs, nil
+}
+
+// Create implements AivenClient for ClickhouseUser.
+func (r *ClickhouseUserControllerV2) Create(ctx context.Context, user *v1alpha1.ClickhouseUser) (CreateResult, error) {
+	logr.FromContextOrDiscard(ctx).Info("generation wasn't processed, creation or updating instance on aiven side")
+	a := user.GetAnnotations()
+	delete(a, processedGenerationAnnotation)
+	delete(a, instanceIsRunningAnnotation)
+
+	// Validates the secret password if it exists
+	_, err := GetPasswordFromSecret(ctx, r.Client, user)
+	if err != nil {
+		return CreateResult{}, fmt.Errorf("failed to get password from secret: %w", err)
+	}
+
+	if user.Status.UUID == "" {
+		req := clickhouse.ServiceClickHouseUserCreateIn{
+			Name: user.GetUsername(),
+		}
+		rsp, err := r.avnGen.ServiceClickHouseUserCreate(ctx, user.Spec.Project, user.Spec.ServiceName, &req)
+		if err != nil {
+			logr.FromContextOrDiscard(ctx).Info(
+				"unable to create or update %s: %s/%s, retrying: %s",
+				user.GetObjectKind().GroupVersionKind().Kind,
+				user.GetNamespace(),
+				user.GetName(),
+				err,
+			)
+			return CreateResult{}, err
+		}
+		user.Status.UUID = rsp.Uuid
+	}
+
+	logr.FromContextOrDiscard(ctx).Info(
+		"processed instance, updating annotations",
+		"generation", user.GetGeneration(),
+		"annotations", user.GetAnnotations(),
+	)
+
+	// We don't really know if an instance is created or updated because:
+	// 1. The go client's retry mechanism may create an instance successfully but receive a 5xx error,
+	//    causing the next attempt to get a conflict error
+	// 2. Remote state may be temporarily inconsistent, so GET checks can return false positives/negatives
+	// 3. Some handlers implement their own retry logic and keep trying to create instances until success
+	// Therefore, we can't rely on "exists" checks to determine the operation type
+	const reason = "CreatedOrUpdated"
+	meta.SetStatusCondition(
+		user.Conditions(),
+		getInitializedCondition(
+			reason,
+			"Successfully created or updated the instance in Aiven",
+		),
+	)
+	meta.SetStatusCondition(
+		user.Conditions(),
+		getRunningCondition(
+			metav1.ConditionUnknown,
+			reason,
+			"Successfully created or updated the instance in Aiven, status remains unknown",
+		),
+	)
+	metav1.SetMetaDataAnnotation(
+		user.GetObjectMeta(),
+		processedGenerationAnnotation,
+		strconv.FormatInt(user.GetGeneration(), formatIntBaseDecimal),
+	)
+
+	return CreateResult{}, nil
+}
+
+// Update implements AivenClient for ClickhouseUser.
+func (r *ClickhouseUserControllerV2) Update(ctx context.Context, user *v1alpha1.ClickhouseUser) (UpdateResult, error) {
+	logr.FromContextOrDiscard(ctx).Info("checking if instance is ready")
+
+	// Needs to be before o.NoSecret() check because `get` mutates the object's metadata annotations.
+	// It set the instanceIsRunningAnnotation annotation when the instance is running on Aiven's side.
+	s, err := r.avnGen.ServiceGet(ctx, user.Spec.Project, user.Spec.ServiceName, service.ServiceGetIncludeSecrets(true))
+	if err != nil {
+		return UpdateResult{}, err
+	}
+
+	// User can set password in the secret
+	secretPassword, err := GetPasswordFromSecret(ctx, r.Client, user)
+	if err != nil {
+		return UpdateResult{}, fmt.Errorf("failed to get password from secret: %w", err)
+	}
+
+	// By design, this handler can't create secret in createOrUpdate method, while the password is returned on create only.
+	// The only way to have a secret here is to reset it manually
+	req := clickhouse.ServiceClickHousePasswordResetIn{}
+	if secretPassword != "" {
+		req.Password = &secretPassword
+	}
+
+	password, err := r.avnGen.ServiceClickHousePasswordReset(ctx, user.Spec.Project, user.Spec.ServiceName, user.Status.UUID, &req)
+	if err != nil {
+		return UpdateResult{}, err
+	}
+
+	prefix := getSecretPrefix(user)
+	stringData := map[string]string{
+		prefix + "HOST":     s.ServiceUriParams["host"],
+		prefix + "PORT":     s.ServiceUriParams["port"],
+		prefix + "PASSWORD": password,
+		prefix + "USERNAME": user.GetUsername(),
+		// todo: remove in future releases
+		"HOST":     s.ServiceUriParams["host"],
+		"PORT":     s.ServiceUriParams["port"],
+		"PASSWORD": password,
+		"USERNAME": user.GetUsername(),
+	}
+
+	meta.SetStatusCondition(&user.Status.Conditions,
+		getRunningCondition(metav1.ConditionTrue, "CheckRunning",
+			"Instance is running on Aiven side"))
+	metav1.SetMetaDataAnnotation(&user.ObjectMeta, instanceIsRunningAnnotation, "true")
+
+	result := UpdateResult{
+		SecretDetails: make(map[string][]byte, len(stringData)),
+	}
+	for k, v := range stringData {
+		result.SecretDetails[k] = []byte(v)
+	}
+
+	return result, nil
+}
+
+// Delete implements AivenClient for ClickhouseUser.
+// It mirrors the current delete behaviour used in reconcile2.
+func (r *ClickhouseUserControllerV2) Delete(ctx context.Context, user *v1alpha1.ClickhouseUser) error {
+	// Not processed yet
+	if user.Status.UUID == "" {
+		return nil
+	}
+
+	// skip deletion for built-in users that cannot be deleted
+	if isBuiltInUser(user.Name) {
+		// built-in users like 'default' cannot be deleted, this is expected behavior
+		// we consider this a successful deletion since we can't and shouldn't delete built-in users
+		return nil
+	}
+
+	err := r.avnGen.ServiceClickHouseUserDelete(ctx, user.Spec.Project, user.Spec.ServiceName, user.Status.UUID)
+	if !isNotFound(err) {
+		return err
+	}
+
+	return nil
+}

pkg/managed/client.go renamed to controllers/client.go

@@ -1,14 +1,16 @@
-package managed
+package controllers
 
 import (
 	"context"
+	"errors"
 
 	"github.com/aiven/aiven-operator/api/v1alpha1"
 )
 
-// AivenClient manages the lifecycle of a resource.
+// AivenController manages the lifecycle of a resource.
 // Controllers implement this interface to define how to process their specific resource type.
-type AivenClient[T v1alpha1.AivenManagedObject] interface {
+// Implementations are expected to update status fields and annotations directly on obj.
+type AivenController[T v1alpha1.AivenManagedObject] interface {
 	// Observe the external resource and return its current state.
 	// This method should:
 	// - Check if the resource exists on Aiven side
@@ -22,18 +24,33 @@ type AivenClient[T v1alpha1.AivenManagedObject] interface {
 
 	// Create a new resource.
 	// This is called when Observe indicates the resource doesn't exist.
-	Create(ctx context.Context, obj T) error
+	// It may return optional information about the created external resource (for example, connection details).
+	Create(ctx context.Context, obj T) (CreateResult, error)
 
 	// Update an existing resource.
 	// This is called when Observe indicates the resource exists but is not up-to-date.
-	Update(ctx context.Context, obj T) error
+	Update(ctx context.Context, obj T) (UpdateResult, error)
 
 	// Delete the resource.
 	// This is called when the Kubernetes object is being deleted.
 	// If the resource is already deleted (not found), should return nil.
 	Delete(ctx context.Context, obj T) error
 }
 
+// CreateResult is returned from Create and carries optional information about the created external resource (for example, connection details).
+type CreateResult struct {
+	// SecretDetails contains secret data for the resource (credentials, endpoints, CA certs, etc.).
+	// Will be written to the connInfoSecretTarget if not nil and not empty.
+	SecretDetails map[string][]byte
+}
+
+// UpdateResult is returned from Update and carries optional information about the external resource (for example, connection details).
+type UpdateResult struct {
+	// SecretDetails contains secret data for the resource (credentials, endpoints, CA certs, etc.).
+	// Will be written to the connInfoSecretTarget if not nil and not empty.
+	SecretDetails map[string][]byte
+}
+
 // Observation is the result of observing the resource.
 // Can be extended with additional fields as needed.
 type Observation struct {
@@ -44,28 +61,11 @@ type Observation struct {
 	// Only meaningful when ResourceExists is true.
 	ResourceUpToDate bool
 
-	// ResourceReady indicates whether the resource is in a ready/running state.
-	// This is used to determine when to create the connection secret and mark the resource as ready.
-	ResourceReady bool
-
-	// IsPoweredOff indicates whether the resource is currently powered off.
-	IsPoweredOff bool
-
-	// PreconditionsMet indicates whether all preconditions for the resource are satisfied.
-	PreconditionsMet bool
-
-	// PreconditionError contains the error if preconditions are not met.
-	// This error will be set in status conditions.
-	// Should be nil if PreconditionsMet is true.
-	PreconditionError error
-
 	// SecretDetails contains secret data for the resource (credentials, endpoints, CA certs, etc.).
 	// Will be written to the connInfoSecretTarget if not nil and not empty.
 	// Keys should NOT include prefixes - the reconciler will apply the appropriate prefix.
 	// Example keys: "HOST", "PORT", "USERNAME", "PASSWORD", "CA_CERT"
 	SecretDetails map[string][]byte
-
-	// Metadata contains additional observed information that should be stored in the status.
-	// The reconciler may update status fields based on this data.
-	Metadata map[string]string
 }
+
+var errPreconditionNotMet = errors.New("preconditions are not met")

controllers/common.go

@@ -340,3 +340,26 @@ func isServerError(err error) bool {
 	}
 	return false
 }
+
+// isRetryableAivenError returns true if the error represents a transient Aiven API failure that should be retried by the reconciler.
+//
+// Current policy:
+// - 404: resource may not be visible yet (eventual consistency).
+// - 5xx: server-side issues are considered transient.
+// - 403: eventual consistency in IAM / permissions.
+func isRetryableAivenError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	switch {
+	case isNotFound(err):
+		return true
+	case isServerError(err):
+		return true
+	case isAivenError(err, http.StatusForbidden):
+		return true
+	default:
+		return false
+	}
+}

controllers/common_test.go

@@ -12,6 +12,6 @@ import (
 func TestCreateEmptyUserConfiguration(t *testing.T) {
 	var uc *kafkaconnectuserconfig.KafkaConnectUserConfig
 	m, err := CreateUserConfiguration(uc)
-	assert.Nil(t, m)
+	assert.Empty(t, m)
 	assert.NoError(t, err)
 }