-
Notifications
You must be signed in to change notification settings - Fork 630
Create a ServiceAccount for Broker Ingress #1241
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
Changes from all commits
4f74fcf
e3aedf6
8464d25
fcdced0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,8 +56,8 @@ const ( | |
|
|
||
| // Name of the corev1.Events emitted from the reconciliation process. | ||
| brokerCreated = "BrokerCreated" | ||
| serviceAccountCreated = "BrokerFilterServiceAccountCreated" | ||
| serviceAccountRBACCreated = "BrokerFilterServiceAccountRBACCreated" | ||
| serviceAccountCreated = "BrokerServiceAccountCreated" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be better to have the 4 events: BrokerFilterXXX and BrokerIngressXXX, and you pass them as args when you call the method?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess with the message you are printing the name of the service account or role binding. We can leave it as is... |
||
| serviceAccountRBACCreated = "BrokerServiceAccountRBACCreated" | ||
| ) | ||
|
|
||
| var ( | ||
|
|
@@ -162,59 +162,64 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error | |
| if ns.DeletionTimestamp != nil { | ||
| return nil | ||
| } | ||
| if err := r.reconcileServiceAccountAndRoleBinding(ctx, ns, resources.IngressServiceAccountName, resources.IngressRoleBindingName, resources.IngressClusterRoleName); err != nil { | ||
| return fmt.Errorf("broker ingress: %v", err) | ||
| } | ||
| if err := r.reconcileServiceAccountAndRoleBinding(ctx, ns, resources.FilterServiceAccountName, resources.FilterRoleBindingName, resources.FilterClusterRoleName); err != nil { | ||
| return fmt.Errorf("broker filter: %v", err) | ||
| } | ||
|
|
||
| sa, err := r.reconcileBrokerFilterServiceAccount(ctx, ns) | ||
| b, err := r.reconcileBroker(ctx, ns) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account for the namespace", zap.Error(err)) | ||
| return err | ||
| return fmt.Errorf("broker: %v", err) | ||
| } | ||
|
|
||
| // Tell tracker to reconcile this namespace whenever the Service Account changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(sa, serviceAccountGVK), ns); err != nil { | ||
| logging.FromContext(ctx).Error("Unable to track changes to ServiceAccount", zap.Error(err)) | ||
| return err | ||
| // Tell tracker to reconcile this namespace whenever the Broker changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(b, brokerGVK), ns); err != nil { | ||
| return fmt.Errorf("track broker: %v", err) | ||
| } | ||
|
|
||
| rb, err := r.reconcileBrokerFilterRBAC(ctx, ns, sa) | ||
| return nil | ||
| } | ||
|
|
||
| // reconcileServiceAccountAndRoleBinding reconciles the service account and role binding for | ||
| // Namespace 'ns'. | ||
| func (r *Reconciler) reconcileServiceAccountAndRoleBinding(ctx context.Context, ns *corev1.Namespace, saName, rbName, clusterRoleName string) error { | ||
| sa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, saName)) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account RBAC for the namespace", zap.Error(err)) | ||
| return err | ||
| return fmt.Errorf("service account '%s': %v", saName, err) | ||
| } | ||
|
|
||
| // Tell tracker to reconcile this namespace whenever the RoleBinding changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(rb, roleBindingGVK), ns); err != nil { | ||
| logging.FromContext(ctx).Error("Unable to track changes to RoleBinding", zap.Error(err)) | ||
| return err | ||
| // Tell tracker to reconcile this namespace whenever the Service Account changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(sa, serviceAccountGVK), ns); err != nil { | ||
| return fmt.Errorf("track service account '%s': %v", sa.Name, err) | ||
| } | ||
|
|
||
| b, err := r.reconcileBroker(ctx, ns) | ||
| rb, err := r.reconcileBrokerRBAC(ctx, ns, sa, resources.MakeRoleBinding(rbName, sa, clusterRoleName)) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Error("Unable to reconcile Broker for the namespace", zap.Error(err)) | ||
| return err | ||
| return fmt.Errorf("role binding '%s': %v", rbName, err) | ||
| } | ||
|
|
||
| // Tell tracker to reconcile this namespace whenever the Broker changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(b, brokerGVK), ns); err != nil { | ||
| logging.FromContext(ctx).Error("Unable to track changes to Broker", zap.Error(err)) | ||
| return err | ||
| // Tell tracker to reconcile this namespace whenever the RoleBinding changes. | ||
| if err = r.tracker.Track(utils.ObjectRef(rb, roleBindingGVK), ns); err != nil { | ||
| return fmt.Errorf("track role binding '%s': %v", rb.Name, err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // reconcileBrokerFilterServiceAccount reconciles the Broker's filter service account for Namespace 'ns'. | ||
| func (r *Reconciler) reconcileBrokerFilterServiceAccount(ctx context.Context, ns *corev1.Namespace) (*corev1.ServiceAccount, error) { | ||
| current, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Get(resources.ServiceAccountName, metav1.GetOptions{}) | ||
| // reconcileBrokerServiceAccount reconciles the Broker's service account for Namespace 'ns'. | ||
| func (r *Reconciler) reconcileBrokerServiceAccount(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount) (*corev1.ServiceAccount, error) { | ||
| current, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Get(sa.Name, metav1.GetOptions{}) | ||
|
|
||
| // If the resource doesn't exist, we'll create it. | ||
| if k8serrors.IsNotFound(err) { | ||
| sa := resources.MakeServiceAccount(ns.Name) | ||
| sa, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Create(sa) | ||
| sa, err = r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Create(sa) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| r.Recorder.Event(ns, corev1.EventTypeNormal, serviceAccountCreated, | ||
| fmt.Sprintf("Service account created for the Broker '%s'", sa.Name)) | ||
| fmt.Sprintf("Service account '%s' created for the Broker", sa.Name)) | ||
| return sa, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
|
|
@@ -223,19 +228,18 @@ func (r *Reconciler) reconcileBrokerFilterServiceAccount(ctx context.Context, ns | |
| return current, nil | ||
| } | ||
|
|
||
| // reconcileBrokerFilterRBAC reconciles the Broker's filter service account RBAC for the Namespace 'ns'. | ||
| func (r *Reconciler) reconcileBrokerFilterRBAC(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount) (*rbacv1.RoleBinding, error) { | ||
| current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(resources.RoleBindingName, metav1.GetOptions{}) | ||
| // reconcileBrokerRBAC reconciles the Broker's service account RBAC for the Namespace 'ns'. | ||
| func (r *Reconciler) reconcileBrokerRBAC(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount, rb *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) { | ||
| current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(rb.Name, metav1.GetOptions{}) | ||
|
|
||
| // If the resource doesn't exist, we'll create it. | ||
| if k8serrors.IsNotFound(err) { | ||
| rb := resources.MakeRoleBinding(sa) | ||
| rb, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Create(rb) | ||
| rb, err = r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Create(rb) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| r.Recorder.Event(ns, corev1.EventTypeNormal, serviceAccountRBACCreated, | ||
| fmt.Sprintf("Service account RBAC created for the Broker Filter '%s'", rb.Name)) | ||
| fmt.Sprintf("Service account RBAC '%s' created for the Broker", rb.Name)) | ||
| return rb, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
|
|
||
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.
Now that you are editing this file... can you fix the cosmetic:
the implmentation may change at any time, absolutely no guarantees are made about the implmentation
Also, inside Implementation, Namespace, you will have to add the steps of creating the eventing-broker-filter service account, and the RBAC.
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.
Now that is fresh, there is a broker_trigger.md file in knative/docs, with this manual steps as well. It'd be great if you can create a follow up PR on docs with this same changes...
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.
Made the changes here.
Once this gets in, I'll immediately open a PR on docs to update it their too.
Maybe we should just delete this file? And rely on docs only?
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.
Yes, I think it'd be better to remove this one. Just check if it has the same content. I think the docs one doesn't have the implementation, but I think it's fine to skip that section.
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.
OK. In a follow up PR (after I submit the changes to docs), I'll pair this down to a link to docs and implementation details.
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.
Sounds good!