WIP Add grafana service account feature to Grafana controller #1907
Conversation
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Hi, thanks for the PR! :D
As this is WIP I have mostly commented on style rather than functionality or wording.
Since you diligent with adding logs, I would like to direct you to: https://github.com/grafana/grafana-operator/blob/master/controllers/contactpoint_controller.go#L209
It's one of the few examples of debug level logs in the project by adding the .V(1)
It could be useful to you if you've been wanting to add more logs but have been holding back.
In case you have questions, feel free to leave comments!
|
|
||
| // NoMatchingInstances indicates that the instanceSelector cannot find any matching Grafana instances. | ||
| NoMatchingInstances bool `json:"NoMatchingInstances,omitempty"` |
There was a problem hiding this comment.
The plan is to deprecate the status.NoMatchingInstances in favour of using conditions.
Hence we would like to avoid it on new CRs :)
This means we won't have it as a status column either.
| // NoMatchingInstances indicates that the instanceSelector cannot find any matching Grafana instances. | |
| NoMatchingInstances bool `json:"NoMatchingInstances,omitempty"` |
There was a problem hiding this comment.
I recently (#1895) removed the grafana prefix from the controller files due to some files having the prefix and some not.
| // ResyncPeriodHasElapsed returns true if the time since last resync has exceeded the desired period. | ||
| func (in *GrafanaServiceAccount) ResyncPeriodHasElapsed() bool { | ||
| deadline := in.Status.LastResync.Add(in.Spec.ResyncPeriod.Duration) | ||
| return time.Now().After(deadline) | ||
| } |
There was a problem hiding this comment.
We have generally moved to relying on the controller.Runtime instead of checking the ResyncPeriod.
You can have a look at newer reconcilers like AlertRuleGroup which does not include this function nor the check.
| // ResyncPeriodHasElapsed returns true if the time since last resync has exceeded the desired period. | |
| func (in *GrafanaServiceAccount) ResyncPeriodHasElapsed() bool { | |
| deadline := in.Status.LastResync.Add(in.Spec.ResyncPeriod.Duration) | |
| return time.Now().After(deadline) | |
| } |
|
|
||
| // Reconcile contains the main reconciliation logic for GrafanaServiceAccount. | ||
| func (r *GrafanaServiceAccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| l := log.FromContext(ctx).WithName("GrafanaServiceAccountReconciler") |
There was a problem hiding this comment.
This does save characters but it does not conform well to the rest of the code base :)
| l := log.FromContext(ctx).WithName("GrafanaServiceAccountReconciler") | |
| log := log.FromContext(ctx).WithName("GrafanaServiceAccountReconciler") |
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| "sigs.k8s.io/controller-runtime/pkg/log" |
There was a problem hiding this comment.
To use log from above, the import needs to be changed.
| "sigs.k8s.io/controller-runtime/pkg/log" | |
| logf "sigs.k8s.io/controller-runtime/pkg/log" |
| if err := ctrl.NewControllerManagedBy(mgr). | ||
| For(&v1beta1.GrafanaServiceAccount{}). | ||
| WithEventFilter(ignoreStatusUpdates()). | ||
| Complete(r); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Embedding the initialization of err inside the if is less useful when it obscures where the body of the if starts and can in this case hurt code readability.
| // Update CR status with new tokens. | ||
| if err := r.Client.Status().Update(ctx, cr); err != nil { | ||
| log.FromContext(ctx).Error(err, "failed to update GrafanaServiceAccount status with new tokens") | ||
| return err | ||
| } |
There was a problem hiding this comment.
Is it necessary to update the status multiple times when the status is always updated after a reconcile in the defer?
| } else { | ||
| l.Info("updated permission for user", "userID") | ||
| } |
There was a problem hiding this comment.
The else body can be omitted if the error case calls continue
I've been reading Effective Go recently which has a good example of this.
| } else { | |
| l.Info("updated permission for user", "userID") | |
| } | |
| } else { | |
| l.Info("updated permission for user", "userID") | |
| } |
| if _, err := accessControlClient.SetResourcePermissionsForUser( // nolint:errcheck | ||
| access_control.NewSetResourcePermissionsForUserParamsWithContext(ctx). | ||
| WithBody(&models.SetPermissionCommand{Permission: desiredPerm}). | ||
| WithResource(resource). | ||
| WithResourceID(resourceID). | ||
| WithUserID(curr.UserID), |
There was a problem hiding this comment.
Same note on initialization hiding that it's a standard if err != nil {}
|
|
||
| // Save the service account ID in CR status and update Grafana status. | ||
| cr.Status.ID = serviceAccountID | ||
| ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("serviceAccountID", serviceAccountID)) |
There was a problem hiding this comment.
You could clean this up a bit by moving the log.FromContext(ctx)... into both the r.reconcileTokens and r.reconcilePermissions as the serviceAccountID is passed to both.
This keeps the pattern of the first thing in each function is extracting a logger from the context.
|
I found lots of issues and missed cases in my code. Lemme rework it yet. :) |
Not yet done :)
#1469
This PR is a work in progress. I based my implementation on the existing
Datasourcecontroller, and at this point the core functionality appears to be complete. However, the code still needs refactoring to improve testability, and I'm currently adding tests that will likely uncover bugs and missing pieces. As I'm new to writing Kubernetes Operators, I'm carefully reviewing tutorials and documentation to ensure that I follow best practices and don’t miss anything important. Feedback is very welcome!