-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add PSA Flag to Scorecard #6187
Add PSA Flag to Scorecard #6187
Conversation
Signed-off-by: Ish Shah <ishah@redhat.com>
Signed-off-by: Ish Shah <ishah@redhat.com>
Signed-off-by: Ish Shah <ishah@redhat.com>
Signed-off-by: Ish Shah <ishah@redhat.com>
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
podSecFlag := true | ||
if c.podSecurity == "resticted" { | ||
podSecFlag = true | ||
} else if c.podSecurity == "legacy" { | ||
podSecFlag = false | ||
} | ||
|
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.
A nit/typo:
podSecFlag := true | |
if c.podSecurity == "resticted" { | |
podSecFlag = true | |
} else if c.podSecurity == "legacy" { | |
podSecFlag = false | |
} | |
podSecFlag := true | |
if c.podSecurity == "restricted" { | |
podSecFlag = true | |
} else if c.podSecurity == "legacy" { | |
podSecFlag = false | |
} | |
As an aside, since we are processing this as a boolean, would it be easier to just make this a boolean flag rather than a string flag? Maybe something like --enable-psa
? This way we don't have to do any if statements like this where we are processing a string into a boolean.
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.
Somehow I missed this. No it should not be treated as a boolean. It should match what we're doing in run bundle. #6062 we treat it as an enum to match what is in operator-framework/api.
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.
Would it be possible for the scorecard implementation to import and use the same logic that was created for operator-sdk run bundle
?
I'm fine to leave this as is if we are happy with it, but it would be nice to be using the same tooling/logic in all places we want this kind of functionality rather than having slightly different methods for each implementation.
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.
It might be possible, just seems weird. We would likely want to move the enum stuff out of run bundle into a common area so that it can be used by both sides: scorecard & run bundle.
|
||
// Create a Pod to run the test | ||
podDef := getPodDefinition(r.configMapName, test, r) | ||
if podSec { | ||
secCtx := v1.PodSecurityContext{} |
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.
In my testing, a bit more than this was necessary.
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.
He's adding the Seccompprofile
// SecurityContext: &corev1.PodSecurityContext{
// SeccompProfile: &corev1.SeccompProfile{
// Type: corev1.SeccompProfileTypeRuntimeDefault,
// },
That's what we ended up adding to the run bundle command. You shouldn't have to add the runuser to it.
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.
When I was testing my patch, the 'scorecard-untar' init container needed the RunAsUser (the RunAsGroup may not be necessary, but I put it in there for completeness). This is needed because this container utilizes UBI directly, and UBI does not specify a user. Hence, the untar will not run. Here is the error message:
Warning Failed 4s (x2 over 4s) kubelet Error: container has runAsNonRoot and image will run as root (pod: "scorecard-test-tw6p_default(dba24b81-7d41-4941-9b4f-946e2a15a079)", container: scorecard-untar)
This is from running kubectl describe pod scorecard-test-xxxx
with this patch. That's why the timeout is occurring in the e2e test. This is consistent with what I saw when testing before I put in the RunAsUser.
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 we should consider creating and pushing a custom image that will run as a non-root user by default?
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.
@bcrochet I discussed with @everettraven and you are correct with the RunUser. The reason I didn't have to do it with run bundle
was the index images have the proper userids. So +1 to adding your changes to this PR.
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 we should consider creating and pushing a custom image that will run as a non-root user by default?
My first thought was "why bother?" but then I had the thought that it might not be a bad idea to do that, and have it be based on ubi(8|9):latest so that it is kept up-to-date on every release of operator-sdk.
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.
My first thought was "why bother?" but then I had the thought that it might not be a bad idea to do that, and have it be based on ubi(8|9):latest so that it is kept up-to-date on every release of operator-sdk.
My only reason for suggesting this was because I believe that there is a problem when setting RunAsUser
on OpenShift >= 4.11.
There is this note in the sample pod security manifest in the Operator SDK Pod Security Standards docs:
# WARNING: Ensure that the image used defines an UserID in the Dockerfile
# otherwise the Pod will not run and will fail with `container has runAsNonRoot and image has non-numeric user`.
# If you want your workloads admitted in namespaces enforced with the restricted mode in OpenShift/OKD vendors
# then, you MUST ensure that the Dockerfile defines a User ID OR you MUST leave the `RunAsNonRoot` and
# RunAsUser fields empty.
Creating the custom container image that can be used by default would allow the default logic to always work without specifying RunAsUser
which, as far as I understand, should make the default logic work on any cluster.
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.
The "why bother?" was more about just one more asset to have to track. I'm totally on board with doing it though. I think it makes a lot of sense once I took a step back and thought about it.
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.
@theishshah was there ever any decision on the direction to go in regards to creating a new container image for scorecard that will run as a non root user within the container?
@theishshah @everettraven why is this PR still here? This should've been done a while ago. What's holding it up? |
Honestly not sure. I'm on vacation for the next 2 weeks but if this PR is still hanging out when I get back I'll try to move this along. |
sanity errors fix
@everettraven - Yes, I updated the website content for the ScoreCard. |
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
Closes #5939 |
👏🏽 👏🏽 👏🏽 |
Add PSA Flag to Scorecard