issue-528, handle the user errors #547
Conversation
OleksiienkoMykyta
requested review from
ribaraka,
testisnullus and
tengu-alt
| @@ -134,7 +136,7 @@ func (r *RedisUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( | |||
| for clusterID, event := range user.Status.ClustersEvents { | |||
| if event == models.CreatingEvent { | |||
| _, err = r.API.CreateRedisUser(user.Spec.ToInstAPI(password, clusterID, username)) | |||
| if err != nil { | |||
| if err != nil && !strings.Contains(fmt.Sprint(err), "username already in use") { | |||
There was a problem hiding this comment.
!strings.Contains(fmt.Sprint(err) - doesn't seem like the best approach for error handling. If you don't get the HTTP status code AlreadyExist, I recommend implementing the following alternative strategy:
- Retrieve a list of users from the Instaclustr API.
- Iterate through this list and check for any duplicate users.
- Only proceed with sending a request for user creation if no duplicates are found in the user list.
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
2 times, most recently
from
0ddcd3e to
6f03bf7
Compare
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
from
6f03bf7 to
e960266
Compare
| @@ -339,6 +368,8 @@ func (r *RedisUserReconciler) handleDeleteUser( | |||
| return err | |||
| } | |||
|
|
|||
| l.Info("CR Redis User was deleted") | |||
There was a problem hiding this comment.
l.Info("Redis User resource/CR was deleted")
| if event == models.CreatingEvent { | ||
| _, err = r.API.CreateRedisUser(user.Spec.ToInstAPI(password, clusterID, username)) | ||
| if err != nil { | ||
| err = r.API.GetRedisUser(userID) |
There was a problem hiding this comment.
It's strange that the Get method returns nothing. Let's return the user from the GetRedisUser method and validate that usernames are the same, wdyt?
There was a problem hiding this comment.
I think that we should return entity, but name validation unnecessary, because user ID contains user name, we cant get user if its not equal
user ID example: "id": "c1af59c6-ba0e-4cc2-a0f3-65cee17a5f37_myRedisUser"
User ID equals clusterID + "_" + username
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
from
e960266 to
b730143
Compare
| name: "Username-redis" | ||
| name: "Username-test" | ||
| version: "7.0.12" | ||
| slaTier: "NON_PRODUCTION" | ||
| clientEncryption: false | ||
| passwordAndUserAuth: true | ||
| privateNetworkCluster: false | ||
| userRefs: | ||
| # - name: redisuser-sample-1 | ||
| # namespace: default | ||
| # - name: redisuser-sample-2 | ||
| # namespace: default | ||
| # - name: redisuser-sample-3 | ||
| # namespace: default | ||
| - name: redisuser-sample-1 | ||
| namespace: default | ||
| - name: redisuser-sample-2 | ||
| namespace: default | ||
| - name: redisuser-sample-3 | ||
| namespace: default | ||
| # twoFactorDelete: |
There was a problem hiding this comment.
please leave it unchanged
| _, err = r.API.GetRedisUser(userID) | ||
|
|
||
| if err != nil && !errors.Is(err, instaclustr.NotFound) { | ||
| l.Error(err, "Cannot create a user for the Redis cluster", |
There was a problem hiding this comment.
"Cannot create a user..." -> "Cannot get a user..."
There was a problem hiding this comment.
I thought that it would be strange when customer try to create entity and got message that they cannot get user from the cluster, wdyt?
There was a problem hiding this comment.
In this case, write a more detailed error message.
There was a problem hiding this comment.
"Cannot create a user for the Redis cluster" --> "Cannot check if the user already exists on the cluster"
| err = r.API.UpdateRedisUser(user.ToInstAPIUpdate(password, userID)) | ||
| if err != nil { | ||
| if err != nil && !errors.Is(instaclustr.NotFound, err) { |
There was a problem hiding this comment.
please switch the parameters in !errors.Is. Fix in similar cases
| if errors.Is(instaclustr.NotFound, err) { | ||
| l.Info("Cannot update redis user password", | ||
| "secret name", user.Spec.SecretRef.Name, | ||
| "secret namespace", user.Spec.SecretRef.Namespace, | ||
| "username", username, | ||
| "cluster ID", clusterID, | ||
| "user ID", userID, | ||
| ) | ||
|
|
||
| r.EventRecorder.Eventf(user, models.Warning, models.UpdateFailed, | ||
| "Given user doesn`t exist on the cluster ID: %v", clusterID) | ||
| } |
There was a problem hiding this comment.
Maybe you should verbose these logs a bit because they are the same as if there is no NotFound error.
Like:
Cannot update redis user password -> Cannot update redis user password, the user doesn't exist on the given cluster
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
2 times, most recently
from
09f7de9 to
fbc2eb7
Compare
| @@ -339,6 +368,8 @@ func (r *RedisUserReconciler) handleDeleteUser( | |||
| return err | |||
| } | |||
|
|
|||
| l.Info("Redis User resource/CR was deleted") | |||
There was a problem hiding this comment.
l.Info("Redis User resource/CR was deleted") -> l.Info("Redis User resource was deleted")
I meant that you can choose between 2 options and use only one of them
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
from
fbc2eb7 to
c207c5b
Compare
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
from
c207c5b to
ca16854
Compare
OleksiienkoMykyta
force-pushed
the
issue-528-handle-the-user-errors
branch
from
ca16854 to
a938766
Compare
No description provided.