Skip to content
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

clean up provisioner RBAC and reuse Contour RBAC for it #4438

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

skriss
Copy link
Member

@skriss skriss commented Mar 31, 2022

Tidies up provisioner RBAC markers and reuses
the Contour RBAC file when generating the
provisioner RBAC YAML instead of requiring all
markers to be duplicated in the provisioner's
file.

Signed-off-by: Steve Kriss krisss@vmware.com

Tidies up provisioner RBAC markers and reuses
the Contour RBAC file when generating the
provisioner RBAC YAML instead of requiring all
markers to be duplicated in the provisioner's
file.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Mar 31, 2022
@skriss skriss requested a review from a team as a code owner March 31, 2022 19:04
@skriss skriss requested review from stevesloka and youngnick and removed request for a team March 31, 2022 19:04
@@ -87,7 +88,7 @@ func desiredClusterRole(name string, contour *model.Contour) *rbacv1.ClusterRole
// Gateway API resources.
// Note, ReferencePolicy does not currently have a .status field so it's omitted from the status rule.
policyRuleFor(gatewayv1alpha2.GroupName, getListWatch, "gatewayclasses", "gateways", "httproutes", "tlsroutes", "referencepolicies"),
policyRuleFor(gatewayv1alpha2.GroupName, createGetUpdate, "gatewayclasses/status", "gateways/status", "httproutes/status", "tlsroutes/status"),
policyRuleFor(gatewayv1alpha2.GroupName, update, "gatewayclasses/status", "gateways/status", "httproutes/status", "tlsroutes/status"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skriss skriss requested a review from sunjayBhatia March 31, 2022 19:06
@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #4438 (bd6bd17) into main (a40bfc4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4438      +/-   ##
==========================================
+ Coverage   74.10%   74.12%   +0.01%     
==========================================
  Files         137      137              
  Lines       12218    12219       +1     
==========================================
+ Hits         9054     9057       +3     
+ Misses       2966     2964       -2     
  Partials      198      198              
Impacted Files Coverage Δ
...ovisioner/objects/rbac/clusterrole/cluster_role.go 45.90% <100.00%> (+0.90%) ⬆️
internal/sorter/sorter.go 98.78% <0.00%> (+1.21%) ⬆️

// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;roles;rolebindings,verbs=get;list;delete;create;update;watch
// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;delete;create;update
// +kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get;list;watch;delete;create;update
// This file only contains entries for RBAC that the Provisioner needs itself directly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you were right i like this method of doing things a lot more!

// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;roles;rolebindings,verbs=get;list;watch;create;update;delete
// ---

// RBAC for leader election for the provisioner.
// +kubebuilder:rbac:groups="",resources=events,verbs=create;get;update,namespace=projectcontour
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removable? tried w/o these couple lines and it didnt change the generated role

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this one is effectively duplicated with https://github.com/projectcontour/contour/blob/main/internal/k8s/rbac.go#L27-L29, hence why no change in the generated role, but I left it in here because the provisioner itself can do leader election, so even if Contour (for some reason) stopped doing leader election, the provisioner would still need it itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, good to eventually note somewhere if you want to deploy the provisioner in a custom namespace you'll have to change that part of the manifest 👍🏽

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk if we properly documented that except maybe in release notes for the xds server itself

@skriss skriss merged commit 105f6e5 into projectcontour:main Mar 31, 2022
@skriss skriss deleted the pr-gw-rbac-cleanup branch April 6, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants