vmagent and vmsingle: updated RBAC#2364
Conversation
c7ba94f to
4d7a12e
Compare
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
4d7a12e to
4c2f559
Compare
| if err := ensureCRBExist(ctx, rclient, cr, prevCR); err != nil { | ||
| return fmt.Errorf("cannot ensure state of vmagent's cluster role binding: %w", err) | ||
| } | ||
| // Secrets access must be namespace-scoped even in cluster-wide mode to prevent |
There was a problem hiding this comment.
This is a breaking functional change - for instance, you can no longer reuse the same license across multiple namespaces
There was a problem hiding this comment.
it's not, operator collects secrets from all scrape configurations and remote writes and writes them all together into a single secret. for license operator just mounts a secret to a pod, it means it already should be in the same namespace. no changes here
| func buildRole(cr *vmv1beta1.VMAgent) *rbacv1.Role { | ||
| return &rbacv1.Role{ | ||
| // buildRole builds the Role for ns. | ||
| // In cr's own namespace: includes the scoped secrets rule (for config-reloader) plus SD rules, |
There was a problem hiding this comment.
That would also be a functionally breaking change - a single VMAgent can't scrape targets across namespaces with secrets?
There was a problem hiding this comment.
watch namespace supports a list of namespaces, but currently vmagent can only access secrets in a namespace it resides, which is not valid.
| // buildRole builds the Role for ns. | ||
| // In cr's own namespace: includes the scoped secrets rule (for config-reloader) plus SD rules, | ||
| // with an owner reference so K8s GC cleans it up. | ||
| // In any other watched namespace: SD rules only, no owner reference. |
There was a problem hiding this comment.
This would be problematic for garbage collector
There was a problem hiding this comment.
well, same as other resources, namespaced resources have owner references, so even for some reason they were not removed they will be purged during CR removal
| "nodes", | ||
| }, | ||
| Verbs: []string{"get", "list", "watch"}, | ||
| Resources: []string{"pods", "namespaces"}, |
There was a problem hiding this comment.
That would still be a cluster-wide rule to access all pods in all namespaces
There was a problem hiding this comment.
yes, just removed nodes permissions if it's not needed.
actually removed these changes, nodes permissions are required unconditionally
c5bf4ec to
1fcb2ba
Compare
1fcb2ba to
d434323
Compare
updated VMAgent and VMSingle RBAC:
introduced
VM_CLUSTERWIDE_RBAC_ALLOWED_NAMESPACESto set allowed namespaces, where CRs with cluster access can be created. right now it only throws warnings (temporary to avoid breaking changes) if not set and error if it's set, but resource that requires cluster wide permissions is deployed outside a list of allowed namespaces