-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Include update operation in build admission controller #6576
Conversation
for _, strategy := range buildStrategyTypes() { | ||
for clientType, client := range clients { | ||
if _, err := updateBuild(t, client.Builds(testutil.Namespace()), builds[string(strategy)+clientType]); !kapierror.IsForbidden(err) { | ||
t.Errorf("epxected forbidden for strategy %s and client %s: got %v", strategy, clientType, 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.
expected
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.
fixed
774ae0e
to
8fef687
Compare
lgtm |
[Test]ing while waiting on the merge queue |
8fef687
to
b127dee
Compare
[test] |
@csrwng Not a known flake:
|
@deads2k thanks. Just discovered that this is a bug in the upstream API server... patch calls the admission control plugin with an empty object. If it gets permission, then proceeds to patch the stored object. So the admission control plugin never gets a chance to determine whether the patched object is valid or not. https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/apiserver/resthandler.go#L399-L407 To get this PR in, I can add a check for an empty object and let it go through. However, you'll be able to use patch to change a build config to a type that you're not allowed to use. |
Opened k8s issue: kubernetes/kubernetes#19479 |
b127dee
to
2658298
Compare
Added check for an actual update @deads2k ptal |
if err != nil { | ||
return false, err | ||
} | ||
return len(accessor.ResourceVersion()) > 0, nil |
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.
Can't I do a force update without a ResourceVersion
? Name
would have to be present.
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.
ha, you're right... name is a better check
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.
updated to use name
2658298
to
16acad2
Compare
Removing merge tags while I look at how hard proper admission will be. Depending on how bad it is, we may wait and apply this on top. I will update one way or the other this afternoon. |
16acad2
to
ef3bd0a
Compare
reverted to not checking object on update - will depend on upstream fix |
Try building on top of #6608. It should make the update admission chain work how you expect. |
ef3bd0a
to
27cdff8
Compare
[test] |
27cdff8
to
fab0067
Compare
[test] |
1 similar comment
[test] |
adbb734
to
fdc42c6
Compare
@deads2k I added a commit to modify the bootstrap policy for the build controller sa. Because we are now checking build update operations, the build controller cannot update builds without having access to the build type virtual resources. How would migration of existing policy be handled in this case? |
Wait, why didn't the build controller already have those permissions? How was it able to create builds without them? |
@liggitt the build controller doesn't create builds, only the BuildConfig controller does. Looking at the startup code, the BuildImageChangeTriggerController gets a PrivilegedLoopbackOpenShiftClient, not a service account client. |
@@ -35,7 +35,7 @@ type buildByStrategy struct { | |||
// on policy based on the build strategy type | |||
func NewBuildByStrategy(client client.Interface) admission.Interface { | |||
return &buildByStrategy{ | |||
Handler: admission.NewHandler(admission.Create), | |||
Handler: admission.NewHandler(admission.Create, admission.Update), |
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.
is it odd that we're gating create and update on create permission on the subresource? I'd sort of expect create to check create, and update to check update
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.
I had discussed this with @bparees ... I'm fine with either way. Because it is a virtual resource, separating create from update didn't seem to gain us much. You still have separate permission for create or update of the actual resource and requiring that permissions be aligned seemed overkill.
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.
fair enough, just reads weird
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.
maybe worth a comment in the policy, in case someone is confused
fdc42c6
to
8bcf43d
Compare
added comment and updated test fixture |
seems like a flake [test] |
flake: #6635 |
lgtm. squash and merge at will. |
8bcf43d
to
abdb4b3
Compare
[merge] |
1 similar comment
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4630/) (Image: devenv-rhel7_3146) |
Failing on #6651 |
[test][merge] |
Evaluated for origin test up to abdb4b3 |
Evaluated for origin merge up to abdb4b3 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8544/) |
Merged by openshift-bot
It is possible to update a bc that was created with an allowed type to a type that is not allowed. This change closes that loophole.
Fixes #6556