-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add conformance test for setting user #3423
Conversation
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.
@dgerd: 3 warnings.
In response to this:
This change adds a conformance test to validate that a user set in the
securityContext is reflected in the container. This change also adds the
group information to the runtime test image, but we do not validate this
as 1. it is not currently part of the runtime contract and 2. Setting
group is currently an alpha feature that does not work on many Kubernetes
clusters. See kubernetes/enhancements#213Related to: #3223
Fixes #
Proposed Changes
Release Note
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
This change adds a conformance test to validate that a user set in the securityContext is reflected in the container. This change also adds the group information to the runtime test image, but we do not validate this as 1. it is not currently part of the runtime contract and 2. Setting group is currently an alpha feature that does not work on many Kubernetes clusters. See kubernetes/enhancements#213 Related to: knative#3223
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.
Looks good in general.
RunAsUser: &runAsUser, | ||
} | ||
|
||
ri, err := fetchRuntimeInfo(t, clients, &test.Options{SecurityContext: securityContext}) |
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 wonder,
what if we actually create the service and pull all the options in a TestMain and then the tests just verify different pieces of the contract. Or, at least, they are all part of the same test suite with the same TestMain
, that creates the service and tests just query the runtime info?
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 am happy to look into refactoring this, but would prefer to do it in a separate follow-up PR as it will involve changing the existing tests. I would like to keep this one to strictly be additive of the new test.
/lgtm |
/retest |
1 similar comment
/retest |
/assign @mattmoor |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgerd, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
/test pull-knative-serving-integration-tests |
e2e failure looks real |
Yeah I noticed that. It looks like the image either must not be updating or
my local image is out of sync with what i pushed.
…On Thu, Mar 21, 2019 at 6:22 PM Matt Moore ***@***.***> wrote:
e2e failure looks real
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3423 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ApafMqevJNXN5EBWJzzzcr6ETVVnBLElks5vZDBQgaJpZM4bwN0a>
.
|
/retest It looks like the
This is probably related to the move to |
/retest |
Still not respecting the base option
|
/retest |
3 similar comments
/retest |
/retest |
/retest |
/retest |
This change adds a conformance test to validate that a user set in the
securityContext is reflected in the container. This change also adds the
group information to the runtime test image, but we do not validate this
as 1. it is not currently part of the runtime contract and 2. Setting
group is currently an alpha feature that does not work on many Kubernetes
clusters. See kubernetes/enhancements#213
Related to: #3223