-
Notifications
You must be signed in to change notification settings - Fork 433
Manage Grafana Service Accounts from the Grafana CR #2055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
48731e2
to
dc10936
Compare
5256a5e
to
ff42690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these I don't expect to have fixed and should be considered notes for further discussion.
When we're aligned on a final implementation I will review more thoroughly :)
// GenerateTokenSecret, if true, will create one default API token in a Secret if no Tokens are specified. | ||
// If false, no token is created unless explicitly listed in Tokens. | ||
// +kubebuilder:default=true | ||
GenerateTokenSecret bool `json:"generateTokenSecret,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine a scenario where a default service account is wanted as opposed to fully defining the spec?
The GrafanaServiceAccounts
struct is nice to have for potential future configuration values, if we can think of extra aside from GenerateTokenSecret
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe it's time to revise the proposal? :)
https://github.com/grafana/grafana-operator/blob/master/docs/docs/proposals/003-grafanaserviceaccount-crd.md
apiVersion: grafana.integreatly.org/v1beta1
kind: Grafana
metadata:
name: grafana-sa
namespace: grafana-namespace
spec:
grafanaServiceAccounts: #Not sure if this is the right place to place it but thats easily fixed when implementing.
createServiceAccount:
generateTokenSecret: [true/false] #Will create the k8s-secret with a default name if true. Defaults to true.
accounts: #Since its possible today to have multiple service accounts it should be a list of accounts.
- id: grafana-sa
name: grafana-service-account
roles: [Viewer/Editor/Admin]
tokens: #This is a list of the tokens that belongs to this GSA and that the operator should create k8s-secrets with tokens for with the names specified. If not specified it would default to creating a token in a k8s-secret with a default name if spec.createServiceAccount.generateTokenSecret is true.
- Name: grafana-sa-token-<name-of-GSA>
expires: <Absolute date for expiration, defaults to Never>
permissions: #This is to try and match what values can be set when creating GSA in the GUI where you can set different permissions for users and groups.
- user: <users in the cluster/root user etc>
permission: [Edit/Admin]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the proposal is just a general idea on how we thought it'd be good to implement it at the time of writing. We're not bound to implement it as-is if we find areas to improve upon
{ | ||
sar := newGrafanaServiceAccountReconciler(r.Client, r.Scheme) | ||
err := sar.reconcile(ctx, cr) | ||
if err != nil { | ||
log.Error(err, "Failed to reconcile grafana service accounts") | ||
return ctrl.Result{RequeueAfter: RequeueDelay}, nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot figure out how you rotate expiring secrets when the Grafana CR is not requeued?
From what I know, unless we explicitly return a RequeueAfter
, no reconcile will happen unless the spec is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token rotation is in TODO list yet. And yes, RequeueAfter
is the main option.
type GrafanaServiceAccountTokenSpec struct { | ||
// Name is the name of the Kubernetes Secret (and token identifier in Grafana). The secret will contain the token value. | ||
// +kubebuilder:validation:Required | ||
Name string `json:"name"` | ||
|
||
// Expires is the optional expiration time for the token. After this time, the operator may rotate the token. | ||
// +kubebuilder:validation:Optional | ||
Expires *metav1.Time `json:"expires,omitempty"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect some users would want to have secrets created outside the current namespace.
Example: allowing teams access to a Grafana instance by creating External instances with limited permissions on the GrafanaServiceAccount.
ff42690
to
10b8a27
Compare
As discussed in the weekly, I took a look at the broad strokes of this PR today. All in all, I'm happy with how it looks and the overall flow of the implementation so good job on that! One thing that stood out to me was the complexity that is introduced by keeping track of existing/removed service accounts in the The reason why we went with service accounts in the An idea on how we could get the best of both worlds could be:
This has the following benefits:
Happy to discuss this further here or in the next weekly. Would also appreciate comments on this from the other maintainers (maybe even @nissessenap as he was very involved in the original proposal 👀 ) |
Just making sure I understand your suggestions, the flow would be something like:
This would solve a lot of challenges and remove some of the uniqueness.
|
Yes, with one small trick when reconciling the difference between the accounts in the |
Hi all, sounds like an interesting idea @theSuess , thanks for the explanation of the flow @Baarsgaard . I can't remember where, but there were some arguments that we shouldn't have to be forced in to use OPA or similar tools to be able to have a "secure" grafana-operator instance. It sounds like this solution would solve the concerns raced in #1469 (comment), but still use the majority of the implementation done by @ndk in this PR. How do you feel about it @ndk? I assume we would use the So I think it sounds like a simple workaround but still having the same base functionality as discussed in the design. |
Let me know once you've settled on the final approach. I've already switched from a standalone CR to the embedded one once. :) FYI I'm a backend engineer, not a DevOps specialist, so I'm keen to learn about any pitfalls you see here but don't have a strong preference myself. |
Spent some time drafting what Gateway AllowedRoutes might look like on a apiVersion: grafana.integreatly.org/v1beta1
kind: Grafana
metadata:
name: usermirror
namespace: userspace
spec:
# allowMatcher:
# allowedResources:
# resourceMatcher:
allowedMatchers: # First match wins
- namespaces:
from: Selector # All, Selector, Same, None. No default
selector: # labelSelector
matchExpressions:
- key: kubernetes.io/metadata.name # Example from Kong ingress. Default?
operator: In # NotIn
values: ["data", "alerts"]
matchLabels: {} # Default is {}. Matches all labels
- kinds:
# group is In AllowedRoutes, should be Default or omitted entirely
# If omitted, `kind:` could be removed as well, or be kept as future proofing
- group: "grafana.integreatly.org/v1beta1"
kind: GrafanaContactPoint
---
# New Default?
# Maintains current behaviour but only allows GrafanaServiceAccounts within the namespace
spec:
allowedMatchers:
- namespaces:
from: Same
kinds: # Optional, matches all if undefined/empty
- kind: "GrafanaServiceAccount"
- namespaces:
from: All
---
spec:
allowedMatchers:
# Matches all and allows None
# If set as the first in the list, no resources can be applied, is pretty useless.
- namespaces:
from: None
---
spec:
allowedMatchers:
# Matches all and allows resources within the namespace.
# Blocks any resource outside the namespace with allowCrossNamespaceImport: true
- namespaces:
from: Same As I don't expect Matchers to be immutable, the only "unknown" is if there's a better way to handle |
The proposal has been reworked. Closed in favour of #2125 |
The proposal has been reworked. Closed in favour of #2125
#1469 · feat: declarative Grafana Service Account management
Design proposal: #003
Why
The Grafana Operator lets you manage Grafana through Kubernetes CRs, but service accounts were still a manual step (GUI or HTTP API). This PR lets you declare SAs in the Grafana CR so the operator can:
What's inside
spec.grafanaServiceAccounts
in the Grafana CR.GrafanaServiceAccountReconciler
runs after Grafana is ready.status.serviceAccounts
and exposes conditions.tests/e2e/grafanaserviceaccount/chainsaw-test.yaml
.Design notes
status.serviceAccounts
. User‑managed SAs are never touched.orgId
is defaultOut of scope (for now)
expires
) and Enterprise‑only permission rules.Known limitations
TODO
CR example