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

readd required buildrun permission when OwnerRefererencesPermissionEnforcement is enabled #808

Conversation

gabemontero
Copy link
Member

@gabemontero gabemontero commented Jun 14, 2021

Changes

Fixes #806

/kind bug

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • [/ ] Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Use of the OwnerRefererencesPermissionEnforcement admission controller was broken because of permission reductions introduced in v0.5.0.  The required permissions have been reintroduced.

@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note kind/bug Categorizes issue or PR as related to a bug. labels Jun 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from gabemontero after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gabemontero
Copy link
Member Author

/assign @imjasonh

I believe the final determination of what from your RBAC PR caused this is in #806 (comment)

@@ -27,6 +27,10 @@ rules:
resources: ['buildruns/status']
verbs: ['update']

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This permission is required by the OwnerReferencesPermissionEnforcement admission controller,
# which requires that controllers wishing to set OwnerReferences with blockOwnerDeletion have permission
# to update finalizers on the owner resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added @imjasonh

@gabemontero gabemontero force-pushed the finalizer-perms-with-security-enabled branch from db1df1e to 39302fe Compare June 14, 2021 16:44
@adambkaplan
Copy link
Member

Won't we need permission to delete BuildRun objects, since we make the Build parent an owner?

See also #807 - I've updated our KinD config to enable the admission controllers that OpenShift uses.

@gabemontero
Copy link
Member Author

gabemontero commented Jun 14, 2021

Won't we need permission to delete BuildRun objects, since we make the Build parent an owner?

Those permissions were not needed in my testing to execute a build run on openshift 4.8.

See also #807 - I've updated our KinD config to enable the admission controllers that OpenShift uses.

Yep, ideally we see a similar failure with that new test, and then after merging this change, those failures go away.

@gabemontero gabemontero mentioned this pull request Jun 14, 2021
4 tasks
@gabemontero
Copy link
Member Author

Won't we need permission to delete BuildRun objects, since we make the Build parent an owner?

Those permissions were not needed in my testing to execute a build run on openshift 4.8.

See also #807 - I've updated our KinD config to enable the admission controllers that OpenShift uses.

Yep, ideally we see a similar failure with that new test, and then after merging this change, those failures go away.

fwiw @adambkaplan I created a new 4.8 cluster today, deployed the current pipelines release from operator hub, and did a ko apply -R -f ./deploy from my local branch with only this commit's rbac changes (i.e. only adding the 'update' perm for buildruns/finalizer) and was able to create buildruns and taskruns off of a shipwright build / buildstrategy

@sbose78
Copy link
Member

sbose78 commented Jun 16, 2021

Nice! After this goes in, should we be releasing a 0.5.1 ? cc @qu1queee

@gabemontero
Copy link
Member Author

As https://github.com/shipwright-io/build/pull/807/files contains this change, plus additional perms and testing enablement, I'm closing this out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"main" errors out with controller-runtime error
4 participants