-
Notifications
You must be signed in to change notification settings - Fork 153
[RFC-0010] Implement managed identity support for Azure Event Hub provider #1106
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
2bbc7be to
dc2fd41
Compare
d30e98a to
54ceab7
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.
Almost there 👌
I'd like to ask you to please investigate something: Here we are introducing a new operation performed by notification-controller in the Kubernetes API, we are now calling the TokenRequest API to issue a Kubernetes ServiceAccount token for the cloud provider STS exchange. This requires the create verb for the (sub)resource serviceaccounts/token in a ClusterRoleBinding (i.e. for all namespaces). Please check what RBAC permissions notification-controller has in order to be able to perform this operation. I suspect the obvious, it has cluster-admin like kustomize-controller
I meant to include a comment for this. In my install, I had to add a new rule to notification-controller's clusterrole for this permission. I used ARC extension for Flux to test this end-to-end and had to extend permissions there. To work with Flux bootstrap, do we need to extend the RBAC permissions somewhere, perhaps here ? Thanks. |
54ceab7 to
6d514d6
Compare
Yes, that looks like the right place, but I don't see how it binds to the Edit: I believe diff --git a/internal/controller/provider_controller.go b/internal/controller/provider_controller.go
index 1f7d0f9..5bca247 100644
--- a/internal/controller/provider_controller.go
+++ b/internal/controller/provider_controller.go
@@ -35,6 +35,7 @@ import (
// +kubebuilder:rbac:groups=notification.toolkit.fluxcd.io,resources=providers,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch
// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch
+// +kubebuilder:rbac:groups="",resources=serviceaccounts/token,verbs=create
// ProviderReconciler reconciles a Provider object to migrate it to static
// Provider.Edit 2: Actually I don't see the rules from Edit 3: Looking at the output of |
|
Hi Dipti 👋 We have finally released Please update this PR accordingly 🙏 Please also enable the token cache by default, see this comment from Stefan: |
- If authentication token is not specified in provider, attempt to get the token using workload identity. = Add new field .spec.serviceAccountName to support multi-tenant workload identity as defined in RFC-0010 to use an identity with a service account other than the notification-controller. - Use proxy to get the token if specified in provider spec. - Cache the tokens if enabled in the notification controller options. - If address has SAS connection string, use that for authentication, this takes priority over token-authentication - If static JWT token is specified in the secret reference, use it for authentication, this takes priority over workload identity-acquired token. - Update RBAC for notification-controller to be able to create service token requests. - Add unit tests for the 3 authentication mechanisms (SAS, JWT, managed identity). - Add documentation for using single-tenant and multi-tenant approaches of workload identity with azureeventhub provider. - Add operation post to github helpers and provider controller for cache event metrics - Enable token cache by default. Signed-off-by: Dipti Pai <[email protected]> review comments Signed-off-by: Dipti Pai <[email protected]> enable cache by default Signed-off-by: Dipti Pai <[email protected]>
6d514d6 to
0beb3d0
Compare
matheuscscp
left a comment
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.
Thanks @dipti-pai ❤️
Depends on: fluxcd/pkg#917
Part of: fluxcd/flux2#5022
Fixes: #1047
Changes include :
provider, attempt to get the token using workload identity..spec.serviceAccountNameto support multi-tenant workload identity as defined in RFC-0010 to use an identity with a service account other than the notification-controller.providerspec.azureeventhubprovider.Tested the feature with notification-controller service account (single tenant) and standalone service account with proxy and token cache. Also tested with existing auth mechanisms (JWT token in secret/SAS string).
Sharing test results below:
Notification controller logs sending the events to event hub
Cache metrics:
Proxy logs getting tokens once per hour since that's how long the token is valid: