issue-439, Support batch of users for OpenSearch cluster CRD #471
Conversation
worryg0d
force-pushed
the
issue-439-opensearch-users-batch-support
branch
2 times, most recently
from
6835c7e
to
398e351
Compare
| DeletedEvent = "deleted" | ||
| GenericEvent = "generic" | ||
| SecretEvent = "secret" | ||
| ClusterDeletingEvent = "cluster deleting" |
There was a problem hiding this comment.
Why don't use deleting event?
There was a problem hiding this comment.
Because DeletingEvent indicates that it deletes a user from the specified cluster by its username. But ClusterDeletingEvent indicates that the operator deletes cluster, so it hasn't to make an API call to Instaclustr API to delete the user. but we still need to delete finalizers from the user CRD.
Maybe the name of the variable is not appropriate. But if you have any idea how to call or enhance one you are welcome
| } | ||
|
|
||
| patch := user.NewPatch() | ||
| user.Status.State = models.Created | ||
|
|
||
| if controllerutil.AddFinalizer(user, models.DeletionUserFinalizer+clusterID) { |
There was a problem hiding this comment.
Lets use generator for finalizer as you did before
| err, | ||
| ) | ||
| return models.ReconcileRequeue, nil | ||
| errorOccurred := false |
There was a problem hiding this comment.
Let's remove this variable
You can check the error existence and if it exists, re-run the reconciler
There was a problem hiding this comment.
The variable indicates that error has occurred in at least one of user creating or deleting events, so we have to re-run the reconciler. If we check the error existence and immediately re-run the reconciler then it stops processing other events until the next reconciliation.
| patch = user.NewPatch() | ||
|
|
||
| user.Status.ClustersEvents[clusterID] = models.Created | ||
|
|
There was a problem hiding this comment.
Let's remove spaces from here also, check all similar cases, please
| patch := user.NewPatch() | ||
| user.Status.State = models.Created | ||
|
|
||
| if controllerutil.AddFinalizer(user, models.DeletionUserFinalizer+clusterID) { |
There was a problem hiding this comment.
Let's remove the space here
| ) | ||
| r.EventRecorder.Eventf( | ||
| user, models.Normal, models.Deleted, | ||
| "OpenSearchUser resource has been deleted deleted", | ||
| "OpenSearchUser resource has been deleted deleted from the cluster (clusterID: %v)", clusterID, |
There was a problem hiding this comment.
Remove one of deleted words, please
| @@ -306,13 +306,19 @@ func (r *OpenSearchReconciler) HandleUpdateCluster( | |||
| } | |||
| } | |||
|
|
|||
| if o.Spec.UserRef != nil { | |||
| err = r.handleCreateUser(ctx, o, logger) | |||
| errorOccurred := false | |||
There was a problem hiding this comment.
Let's remove this variable and just check the error
There was a problem hiding this comment.
I've answered this in the comment above
worryg0d
force-pushed
the
issue-439-opensearch-users-batch-support
branch
3 times, most recently
from
1190d32
to
3e53c67
Compare
| if o.Spec.UserRefs != nil { | ||
| patch := o.NewPatch() | ||
| o.Annotations[models.ResourceStateAnnotation] = models.UpdatingEvent | ||
| err = r.Patch(ctx, o, patch) |
There was a problem hiding this comment.
I suggest you refactor this logic, as I did. Please take a look at this in the Cassandra controller.
There was a problem hiding this comment.
you may do this in another PR
| errorOccurred := false | ||
|
|
||
| for _, ref := range o.Spec.UserRefs { | ||
| err = r.createUser(ctx, logger, o, ref) | ||
| if err != nil { | ||
| return models.ReconcileRequeue | ||
| errorOccurred = true | ||
| } | ||
| } | ||
|
|
||
| if errorOccurred { | ||
| return models.ReconcileRequeue | ||
| } |
There was a problem hiding this comment.
It's better to remove it all and just keep user creation/deletion logic in the predicate
| err := r.Get(ctx, req, u) | ||
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| l.Error(err, "OpenSearch user is not found", "request", req) | ||
| r.EventRecorder.Eventf(c, models.Warning, models.NotFound, | ||
| "User is not found, create a new one or provide correct userRef."+ | ||
| "Current provided reference: %v", uRef) | ||
| return err | ||
| } |
There was a problem hiding this comment.
if it's a delete event, there's no reason to create a new user for you to delete. Is that make sense?
There was a problem hiding this comment.
Yes, it makes sense. Fixed
| if k8serrors.IsNotFound(err) { | ||
| l.Error(err, "OpenSearch user is not found", "request", req) | ||
| r.EventRecorder.Eventf(c, models.Warning, models.NotFound, | ||
| "User is not found, create a new one or provide correct userRef."+ |
There was a problem hiding this comment.
The same goes for a detach event.
worryg0d
force-pushed
the
issue-439-opensearch-users-batch-support
branch
from
3e53c67
to
bde8496
Compare
Batch of users support
Added support for a batch of users for OpenSearch CRD.
Creation flow
To create a user on the specified cluster a customer should add a reference to a OpenSearchUser CRD to the cluster spec.
Deletion flow
To delete a user from a specified cluster a customer should delete a reference for the OpenSearchUser CRD from the cluster spec.
A customer cannot delete OpenSearchUser CRD until at least one cluster references the user CRD.
Cluster deletion
If a cluster is deleted, the cluster resource sets to the CRD
ClusterDeletingEventthat indicates OpenSearchUser CRD to delete the reference to the deleted cluster. Also, if the event happened, OpenSearchUser reconciler doesn't make an API call to Instaclurt API.