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

pass an internal pod object to SCC admission control so it works #14891

Merged
merged 1 commit into from Jun 27, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Jun 26, 2017

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2017

[test]

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2017

[testextended][extended:core(builds)]

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2017

@smarterclayton @deads2k ptal, i had to undo a bit of the v1 changes @smarterclayton made because the SCC admission controller only likes internal objects.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d186041

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to d186041

@@ -112,27 +112,27 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
func (bs *SourceBuildStrategy) canRunAsRoot(build *buildapi.Build) bool {
var rootUser int64
rootUser = 0
pod := &v1.Pod{
Copy link
Contributor

Choose a reason for hiding this comment

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

Scary

@smarterclayton
Copy link
Contributor

Did this fail closed or fail open?

@bparees
Copy link
Contributor Author

bparees commented Jun 27, 2017 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 27, 2017 via email

@bparees
Copy link
Contributor Author

bparees commented Jun 27, 2017

We should probably fail closed in SCC on a known set of resources if
the object type is not recognized

w/ the known set being the internal versions of all the resources in k8s+openshift? doesn't the admission controller get called on every create of every resource, or is the chain smarter than that? (that's why i decided the admission controller is probably right as is, because the alternative seems awkward and we really only ran into this situation because we're sort of abusing the admission controller)

@openshift openshift deleted a comment from openshift-bot Jun 27, 2017
@openshift openshift deleted a comment from openshift-bot Jun 27, 2017
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2667/) (Base Commit: 955efb1) (PR Branch Commit: d186041)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/750/) (Base Commit: 955efb1) (PR Branch Commit: d186041) (Extended Tests: core(builds))

@pweil-
Copy link
Member

pweil- commented Jun 27, 2017

We already check for resource and subresource. Failing closed on the conversion seems like the right thing to do here. #14901

@deads2k
Copy link
Contributor

deads2k commented Jun 27, 2017

We already check for resource and subresource. Failing closed on the conversion seems like the right thing to do here. #14901

Yeah, we should fail closed. We already try to check if its a pod coming to us.

@bparees is there an issue open to switch the check APIs?

[merge]

@bparees
Copy link
Contributor Author

bparees commented Jun 27, 2017

@bparees is there an issue open to switch the check APIs?

you mean switch the way we are checking if the user can run as root? no, can you open one and put some details/pointers in about what we should be using instead (and then assign it to me).

@deads2k
Copy link
Contributor

deads2k commented Jun 27, 2017

you mean switch the way we are checking if the user can run as root? no, can you open one and put some details/pointers in about what we should be using instead (and then assign it to me).

Opened #14909 with a link to the API type.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2017

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 8

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d186041

@smarterclayton smarterclayton merged commit c7d9fb8 into openshift:master Jun 27, 2017
@bparees bparees deleted the root_builds branch June 30, 2017 21:04
openshift-merge-robot added a commit that referenced this pull request Aug 1, 2017
Automatic merge from submit-queue (batch tested with PRs 15162, 14901, 15195)

fail closed on versioned pods

We already check the resource and subresource.  If we cannot convert to the internal pod type then we should fail.

Note: this should not happen during the normal admission process but could happen if someone manually integrates with the admission controller - as in the instance of builds.

Ref: #14891

## Note
must merge after #14891 and must be backported as a group if backports are desired
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants