fix(clickhousegrant): diff compare

controllers/basic_controller.go

@@ -16,6 +16,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -148,7 +149,11 @@ func (c *Controller) reconcileInstance(ctx context.Context, req ctrl.Request, h
 	}
 
 	requeue, err := helper.reconcile(ctx, o)
-	return ctrl.Result{Requeue: requeue, RequeueAfter: requeueTimeout}, err
+	result := ctrl.Result{Requeue: requeue}
+	if requeue {
+		result.RequeueAfter = requeueTimeout
+	}
+	return result, err
 }
 
 // a helper that closes over all instance specific fields
@@ -198,21 +203,34 @@ func (i *instanceReconcilerHelper) reconcile(ctx context.Context, o v1alpha1.Aiv
 	// First need to update the object, and then update the status.
 	// So dependent resources won't see READY before it has been updated with new values
 
-	// When updated, object status is vanished.
-	// So we waste a copy for that,
-	// while the original object must already have all the fields updated in runtime
-	clone := o.DeepCopyObject().(client.Object)
-	errUpdate := i.k8s.Update(ctx, clone)
-
 	// Now we can update the status
-	o.SetResourceVersion(clone.GetResourceVersion())
-	errStatus := i.k8s.Status().Update(ctx, o)
-	errMerged := errors.Join(err, errUpdate, errStatus)
-	if errMerged != nil {
-		return true, errMerged
-	}
+	errUpdate := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		// When updated, object status is vanished.
+		// So we waste a copy for that,
+		// while the original object must already have all the fields updated in runtime
+		// Additionally, it gets the "latest version" to resolve optimistic concurrency control conflict
+		latest := o.DeepCopyObject().(client.Object)
+		err = i.k8s.Get(ctx, types.NamespacedName{
+			Name:      latest.GetName(),
+			Namespace: latest.GetNamespace(),
+		}, latest)
+		if err != nil {
+			return err
+		}
 
-	return requeue, nil
+		updated := o.DeepCopyObject().(client.Object)
+		updated.SetResourceVersion(latest.GetResourceVersion())
+		err := i.k8s.Update(ctx, updated)
+		if err != nil {
+			return err
+		}
+
+		o.SetResourceVersion(updated.GetResourceVersion())
+		return i.k8s.Status().Update(ctx, o)
+	})
+
+	errMerged := errors.Join(err, errUpdate)
+	return requeue || errMerged != nil, errMerged
 }
 
 func (i *instanceReconcilerHelper) reconcileInstance(ctx context.Context, o v1alpha1.AivenManagedObject) (bool, error) {
@@ -247,7 +265,7 @@ func (i *instanceReconcilerHelper) reconcileInstance(ctx context.Context, o v1al
 
 	requeue, err := i.checkPreconditions(ctx, o, refs)
 	if requeue {
-		return true, err
+		return true, nil
 	}
 
 	if err != nil {

controllers/clickhousegrant_controller.go

@@ -4,10 +4,11 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
+	"net/http"
 	"slices"
 	"strconv"
-	"strings"
 
 	"github.com/aiven/aiven-go-client/v2"
 	avngen "github.com/aiven/go-client-codegen"
@@ -24,8 +25,6 @@ import (
 	chUtils "github.com/aiven/aiven-operator/utils/clickhouse"
 )
 
-const notFoundRole = "There is no role"
-
 // ClickhouseGrantReconciler reconciles a ClickhouseGrant object
 type ClickhouseGrantReconciler struct {
 	Controller
@@ -99,7 +98,8 @@ func (h *ClickhouseGrantHandler) delete(ctx context.Context, avn *aiven.Client,
 	}
 
 	_, err = g.Spec.ExecuteStatements(ctx, avnGen, chUtils.REVOKE)
-	if err == nil || strings.Contains(err.Error(), notFoundRole) {
+	if isAivenError(err, http.StatusBadRequest) {
+		// "not found in user directories", "There is no role", etc
 		return true, nil
 	}
 
@@ -159,44 +159,57 @@ func diffClickhouseGrantSpecToApi(specPrivilegeGrants []v1alpha1.PrivilegeGrant,
 	var roleGrantsToRevoke, roleGrantsToAdd []v1alpha1.RoleGrant
 
 	for _, apiGrant := range apiPrivilegeGrants {
-		if !containsPrivilegeGrant(specPrivilegeGrants, apiGrant) {
+		if !containsGrant(specPrivilegeGrants, apiGrant) {
 			privilegeGrantsToRevoke = append(privilegeGrantsToRevoke, apiGrant)
 		}
 	}
 
 	for _, specGrant := range specPrivilegeGrants {
-		if !containsPrivilegeGrant(apiPrivilegeGrants, specGrant) {
+		if !containsGrant(apiPrivilegeGrants, specGrant) {
 			privilegeGrantsToAdd = append(privilegeGrantsToAdd, specGrant)
 		}
 	}
 
 	for _, apiGrant := range apiRoleGrants {
-		if !containsRoleGrant(specRoleGrants, apiGrant) {
+		if !containsGrant(specRoleGrants, apiGrant) {
 			roleGrantsToRevoke = append(roleGrantsToRevoke, apiGrant)
 		}
 	}
 
 	for _, specGrant := range specRoleGrants {
-		if !containsRoleGrant(apiRoleGrants, specGrant) {
+		if !containsGrant(apiRoleGrants, specGrant) {
 			roleGrantsToAdd = append(roleGrantsToAdd, specGrant)
 		}
 	}
 
 	return privilegeGrantsToRevoke, privilegeGrantsToAdd, roleGrantsToRevoke, roleGrantsToAdd
 }
 
-func containsPrivilegeGrant(grants []v1alpha1.PrivilegeGrant, grant chUtils.Grant) bool {
-	for _, g := range grants {
-		if cmp.Equal(g, grant) {
-			return true
+func serializeGrant(grant any) (result map[string]any) {
+	b, _ := json.Marshal(grant)
+	_ = json.Unmarshal(b, &result)
+
+	for k, v := range result {
+		list, ok := v.([]any)
+		if !ok || len(list) == 0 {
+			continue
 		}
+
+		sorted := make([]string, len(list))
+		for i, s := range list {
+			sorted[i] = fmt.Sprintf("%v", s)
+		}
+
+		slices.Sort(sorted)
+		result[k] = sorted
 	}
-	return false
+	return
 }
 
-func containsRoleGrant(grants []v1alpha1.RoleGrant, grant v1alpha1.RoleGrant) bool {
+func containsGrant[T any](grants []T, grant T) bool {
+	s := serializeGrant(grant)
 	for _, g := range grants {
-		if cmp.Equal(g, grant) {
+		if cmp.Equal(s, serializeGrant(g)) {
 			return true
 		}
 	}
@@ -220,8 +233,8 @@ func (h *ClickhouseGrantHandler) checkPreconditions(ctx context.Context, avn *ai
 	meta.SetStatusCondition(&g.Status.Conditions,
 		getInitializedCondition("Preconditions", "Checking preconditions"))
 
-	serviceIsRunning, err := checkServiceIsRunning(ctx, avn, avnGen, g.Spec.Project, g.Spec.ServiceName)
-	if !serviceIsRunning || err != nil {
+	isRunning, err := checkServiceIsRunning(ctx, avn, avnGen, g.Spec.Project, g.Spec.ServiceName)
+	if !isRunning || err != nil {
 		return false, err
 	}
 

controllers/common.go

@@ -46,19 +46,27 @@ const (
 	errConditionCreateOrUpdate errCondition = "CreateOrUpdate"
 )
 
-var errTerminationProtectionOn = errors.New("termination protection is on")
+var (
+	errTerminationProtectionOn = errors.New("termination protection is on")
+	errServiceIsNotFound       = errors.New("service does not exist")
+	errServiceIsNotRunning     = errors.New("service is not running yet")
+)
 
 func checkServiceIsRunning(ctx context.Context, _ *aiven.Client, avnGen avngen.Client, project, serviceName string) (bool, error) {
 	s, err := avnGen.ServiceGet(ctx, project, serviceName)
 	if err != nil {
 		// if service is not found, it is not running
 		if isNotFound(err) {
 			// this will swallow an error if the project doesn't exist and object is not project
-			return false, nil
+			return false, fmt.Errorf("%w: %s/%s", errServiceIsNotFound, project, serviceName)
 		}
 		return false, err
 	}
-	return serviceIsRunning(s.State), nil
+
+	if serviceIsRunning(s.State) {
+		return true, nil
+	}
+	return false, fmt.Errorf("%w: %s/%s: %s", errServiceIsNotRunning, project, serviceName, s.State)
 }
 
 // serviceIsRunning returns "true" when a service is in operational state, i.e. "running"
@@ -253,7 +261,7 @@ func UpdateUserConfiguration(userConfig any) (map[string]any, error) {
 
 // isNotFound works both for old and new client errors
 func isNotFound(err error) bool {
-	return aiven.IsNotFound(err) || avngen.IsNotFound(err)
+	return isAivenError(err, http.StatusNotFound)
 }
 
 // isAlreadyExists works both for old and new client errors
@@ -271,3 +279,17 @@ func isDeleted(err error) (bool, error) {
 	}
 	return err == nil, err
 }
+
+func isAivenError(err error, code int) bool {
+	var oldErr aiven.Error
+	if errors.As(err, &oldErr) {
+		return oldErr.Status == code
+	}
+
+	var newErr avngen.Error
+	if errors.As(err, &newErr) {
+		return newErr.Status == code
+	}
+
+	return false
+}