diff --git a/controllers/basic_controller.go b/controllers/basic_controller.go index 1a427285..36df7647 100644 --- a/controllers/basic_controller.go +++ b/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 { diff --git a/controllers/clickhousegrant_controller.go b/controllers/clickhousegrant_controller.go index 6f5c6099..8958c8e2 100644 --- a/controllers/clickhousegrant_controller.go +++ b/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,25 +159,25 @@ 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) } } @@ -185,18 +185,31 @@ func diffClickhouseGrantSpecToApi(specPrivilegeGrants []v1alpha1.PrivilegeGrant, 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 } diff --git a/controllers/common.go b/controllers/common.go index 91731245..1a090be7 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -46,7 +46,11 @@ 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) @@ -54,11 +58,15 @@ func checkServiceIsRunning(ctx context.Context, _ *aiven.Client, avnGen avngen.C // 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 +}