-
Notifications
You must be signed in to change notification settings - Fork 113
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
End2End - Pod Security Admission fixes #368
Conversation
Add `pod-security.kubernetes.io/*` labels to namespace created during tests. Signed-off-by: Andrea Panattoni <[email protected]>
If a test use a different namespace than `sriov-conformance-testing`, clean.All() should remove any sriov resource. Signed-off-by: Andrea Panattoni <[email protected]>
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 3197352728
💛 - Coveralls |
/test-e2e-all |
if !namespaces.Exists(namespaces.Test, clients) { | ||
return nil | ||
} | ||
|
||
err := namespaces.DeleteAndWait(clients, namespaces.Test, 5*time.Minute) |
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'm a bit confused by this change and the commit message: will it remove sriov resources from all namespaces or just from the specified one?
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.
this function delete the namespace and all the objects in that namespace. it's for a test namespace we don't run this function on the operator namespace.
There is also a clean sriov function to remove all the sriov related objects like sriovNetwork and sriovNetworkNodePolicy
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.
/lgtm
thanks!
@@ -20,6 +20,13 @@ import ( | |||
// Test is the namespace to be use for testing | |||
const Test = "sriov-conformance-testing" | |||
|
|||
var inhibitSecurityAdmissionLabels = map[string]string{ | |||
"pod-security.kubernetes.io/audit": "privileged", |
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.
We might want to align this with what we have for #371
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.
From what I see the labels have a delta.
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.
@wizhaoredhat this is fine for the test-pods we have this in other places where we run tests like openshift-kni/cnf-features-deploy#1238
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.
Ah ok, got it. Thanks!
@@ -20,6 +20,13 @@ import ( | |||
// Test is the namespace to be use for testing | |||
const Test = "sriov-conformance-testing" | |||
|
|||
var inhibitSecurityAdmissionLabels = map[string]string{ | |||
"pod-security.kubernetes.io/audit": "privileged", |
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.
@wizhaoredhat this is fine for the test-pods we have this in other places where we run tests like openshift-kni/cnf-features-deploy#1238
These changes aim to solve Pod Security Admission problem when running tests against k8s v1.25+ , as privileged pods are often needed during tests.