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

Show SCC provider in error message #13842

Merged

Conversation

php-coder
Copy link
Contributor

Let's use parameter fldPath in the AssignSecurityContext function. In this case provider name is included in the error message that is super helpful during debug.

Before:

admission.go:125] unable to validate pod (generate: es-master-2387585559-) against any security context constraint: [spec.initContainers[0].securityContext.privileged: Invalid value: true: Privileged containers are not allowed capabilities.add: Invalid value: "IPC_LOCK": capability may not be added capabilities.add: Invalid value: "SYS_RESOURCE": capability may not be added]

After

admission.go:125] unable to validate pod (generate: es-master-2387585559-) against any security context constraint: [provider restricted: .spec.initContainers[0].securityContext.privileged: Invalid value: true: Privileged containers are not allowed capabilities.add: Invalid value: "IPC_LOCK": capability may not be added capabilities.add: Invalid value: "SYS_RESOURCE": capability may not be added]

This message is also including in the FailedCreate event. The message still hard to read but now it contains helpful information.

PTAL @pweil-

@php-coder
Copy link
Contributor Author

I see that we're missing information about container here

allErrs = append(allErrs, s.runAsUserStrategy.Validate(pod, container)...)
allErrs = append(allErrs, s.seLinuxStrategy.Validate(pod, container)...)
allErrs = append(allErrs, s.seccompStrategy.ValidateContainer(pod, container)...)
if !s.scc.AllowPrivilegedContainer && *sc.Privileged {
allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *sc.Privileged, "Privileged containers are not allowed"))
}
allErrs = append(allErrs, s.capabilitiesStrategy.Validate(pod, container)...)
because Validate() accepts only container and inside the method there is no information whether it was init-container or not and what is the index of a container.

In the error message that I provided the first error relates to the init container but two others relate to a simple container.

@php-coder
Copy link
Contributor Author

BTW original issue is also exist in kubernetes where fldPath parameter isn't being used by assignSecurityContext(). Let me know if I need to create PR for that too.

@php-coder
Copy link
Contributor Author

@pweil- Gently ping :)

@mfojtik
Copy link
Contributor

mfojtik commented Apr 27, 2017

I don't see any sensitive information leaking here and it indeed improves the messaging.

LGTM (but will defer to @pweil- for merge ;-)

@php-coder
Copy link
Contributor Author

[test]

@php-coder
Copy link
Contributor Author

I see that we're missing information about container

I've created a separate issue for that: #13909

@php-coder
Copy link
Contributor Author

php-coder commented Apr 27, 2017

Tests failed because of #13619

@php-coder
Copy link
Contributor Author

[test]

Ping @pweil-

@php-coder
Copy link
Contributor Author

Ping @pweil-

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 0b87377

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1682/) (Base Commit: 67275e1)

@php-coder
Copy link
Contributor Author

test flake #14328

Copy link
Contributor

@pweil- pweil- left a comment

Choose a reason for hiding this comment

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

LGTM

@php-coder
Copy link
Contributor Author

@mfojtik Can we merge it?

@php-coder
Copy link
Contributor Author

@mfojtik Ping. I don't see a reason of not merging it.

@mfojtik
Copy link
Contributor

mfojtik commented Jun 6, 2017

[merge] sorry for the delay

@php-coder
Copy link
Contributor Author

test flake #12072 and #14489

@php-coder
Copy link
Contributor Author

@mfojtik Could you re-run merge process, please?

@php-coder
Copy link
Contributor Author

@mfojtik and this one too, please :)

@mfojtik
Copy link
Contributor

mfojtik commented Jun 14, 2017 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 0b87377

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 14, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/994/) (Base Commit: e87bd0c) (Image: devenv-rhel7_6358)

@openshift-bot openshift-bot merged commit ee9f436 into openshift:master Jun 14, 2017
@php-coder php-coder deleted the improve_scc_provider_reporting branch June 14, 2017 17:54
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.

4 participants