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

Added securityContext profile to cronjob in istio-system and remove PSS workflow warnings #2848

Merged
merged 19 commits into from
Aug 29, 2024

Conversation

biswajit-9776
Copy link
Contributor

Pull Request Template for Kubeflow manifests Issues

✏️ A brief description of the changes

I added securityContext profile to cronjob kubeflow-m2m-oidc-configurator in istio-system.

📦 List any dependencies that are required for this change

My PR depends on #

🐛 If this PR is related to an issue, please put the link to the issue here.

The following issues are related, because ...

✅ Contributor checklist


You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

@juliusvonkohout
Copy link
Member

Why are you suing while instead of for ?

https://stackoverflow.com/questions/49110/how-do-i-write-a-for-loop-in-bash

@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Aug 22, 2024

What range would you like me to put in the for loop's condition?
I think whichever loop we use, we should constrain the script to not run beyond a specified time that I implemented using SECONDS variable.

@juliusvonkohout
Copy link
Member

The range is up to you. 60 seconds is a good start.

@biswajit-9776
Copy link
Contributor Author

Perhaps I should change this PR for cronjob to just constraining loop time to the script.
Since, changing the cronjob in anyway leads to failure of pipeline workflows.

@juliusvonkohout
Copy link
Member

Perhaps I should change this PR for cronjob to just constraining loop time to the script. Since, changing the cronjob in anyway leads to failure of pipeline workflows.

Maybe you need to check the script and OCI image

@biswajit-9776
Copy link
Contributor Author

I also thought the curlimages/curl image might have some issue so I added the latest verion 8.9.1 and pushed in one of the above commits. Seems like it's still facing the same.
Let me add some commands to view the logs in the script before the loop that's failing.

@juliusvonkohout
Copy link
Member

Please also check that you use a rootless image. Most bitnami images support runasnonroot.

@biswajit-9776
Copy link
Contributor Author

I believe the following was the reason for the failing cronjob even in my local system. I added runAsUser:999 to counter it.

https://stackoverflow.com/questions/49720308/kubernetes-podsecuritypolicy-set-to-runasnonroot-container-has-runasnonroot-and#:~:text=And%20here%20is%20the%20validation%20call%20with%20the%20comment%3A

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Aug 26, 2024

Now it is down to

Warning: existing pods in namespace "auth" violate the new PodSecurity enforce level "restricted:latest"
Warning: dex-7b97f48486-9rwl2: allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
Patching the PSS-restricted labels for namespace cert-manager...
namespace/cert-manager patched
Patching the PSS-restricted labels for namespace oauth2-proxy...
Warning: existing pods in namespace "oauth2-proxy" violate the new PodSecurity enforce level "restricted:latest"
Warning: oauth2-proxy-86d8c97455-58btw (and 1 other pod): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
namespace/oauth2-proxy patched
Patching the PSS-restricted labels for namespace kubeflow...
Warning: existing pods in namespace "kubeflow" violate the new PodSecurity enforce level "restricted:latest"
Warning: cache-server-69f48c65dd-cctpm (and 11 other pods): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, runAsUser=0, seccompProfile
Warning: kubeflow-pipelines-profile-controller-5dcf75b69f-x2h4v (and 1 other pod): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
Warning: metacontroller-0: seccompProfile
Warning: workflow-controller-68d76bcb8b-vcwvm: unrestricted capabilities, runAsNonRoot != true, runAsUser=0, seccompProfile
namespace/kubeflow patched

After fixing this we can create PRs to upstream all patches into the right places/repositories

@juliusvonkohout juliusvonkohout self-assigned this Aug 26, 2024
@biswajit-9776
Copy link
Contributor Author

Now it is down to

Warning: existing pods in namespace "auth" violate the new PodSecurity enforce level "restricted:latest"
Warning: dex-7b97f48486-9rwl2: allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
Patching the PSS-restricted labels for namespace cert-manager...
namespace/cert-manager patched
Patching the PSS-restricted labels for namespace oauth2-proxy...
Warning: existing pods in namespace "oauth2-proxy" violate the new PodSecurity enforce level "restricted:latest"
Warning: oauth2-proxy-86d8c97455-58btw (and 1 other pod): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
namespace/oauth2-proxy patched
Patching the PSS-restricted labels for namespace kubeflow...
Warning: existing pods in namespace "kubeflow" violate the new PodSecurity enforce level "restricted:latest"
Warning: cache-server-69f48c65dd-cctpm (and 11 other pods): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, runAsUser=0, seccompProfile
Warning: kubeflow-pipelines-profile-controller-5dcf75b69f-x2h4v (and 1 other pod): allowPrivilegeEscalation != false, unrestricted capabilities, runAsNonRoot != true, seccompProfile
Warning: metacontroller-0: seccompProfile
Warning: workflow-controller-68d76bcb8b-vcwvm: unrestricted capabilities, runAsNonRoot != true, runAsUser=0, seccompProfile
namespace/kubeflow patched

After fixing this we can create PRs to upstream all patches into the right places/repositories

Shall I fix these warnings in this PR itself?

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Aug 26, 2024
@biswajit-9776 biswajit-9776 changed the title Added securityContext profile to cronjob in istio-system Added securityContext profile to cronjob in istio-system and remove PSS workflow warnings Aug 26, 2024
@juliusvonkohout
Copy link
Member

changes to the m2m script should trigger many workflows in the CICD. There seem to be missing trigger paths.

@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Aug 28, 2024

changes to the m2m script should trigger many workflows in the CICD. There seem to be missing trigger paths.

Yes, that happened due to the typo Hansini made in her previous merged PR replacing /common/oauth2-proxy with common/oidc-client/oauth2-proxy. If she creates the PR, I could rebase and run the tests here again, or I could push a PR to fix the trigger paths myself and then rebase.

@juliusvonkohout
Copy link
Member

Rebase to master first and see what is needed. Then lets merge and continue in a new PR, because this one here is getting too big.

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Aug 29, 2024

Let me change the trigger paths for the m2m tests in this PR itself.

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Aug 29, 2024

Hey @juliusvonkohout, the UID 1000 works for the cronjob as of now. It passes the m2m tests in 25s for the latest commits. I have also increased the time of cronjob test from 60s to 100s. In future we may change it as per requirement.

@@ -11,7 +11,7 @@ on:
- common/cert-manager/**
- common/oauth2-proxy/**
- common/istio*/**
- common/oidc-client/**
- common/**
Copy link
Member

Choose a reason for hiding this comment

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

This line is probably too much. Let's revisit it in your follow up PR

@juliusvonkohout
Copy link
Member

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Aug 29, 2024
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit b91cbc4 into kubeflow:master Aug 29, 2024
11 checks passed
@biswajit-9776 biswajit-9776 deleted the cronjob branch September 2, 2024 12:39
pschoen-itsc pushed a commit to pschoen-itsc/kf-manifests that referenced this pull request Sep 3, 2024
…SS workflow warnings (kubeflow#2848)

* Added securityContext profile to cronjob in istio-system

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Limited script to run for a minute

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Undo change to script

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Added time constraint to waiting for job loop

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Added if condition to script

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Added version to curl image

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Undo change to curl image

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Fixed failing cronjob

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Refactored the script

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Added workflow job to clear PSS warnings

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Removed cluster-local-gateway PSS patch

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Fixed typo in patches

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Debugging failing warnings

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Empty commit

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Fixed trigger paths for m2m tests

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Remove debug commands

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Fixed typo

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Increases cronjob time from 60s to 100s

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

* Change UID to debug

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>

---------

Signed-off-by: biswajit-9776 <biswajitpatt139@gmail.com>
Signed-off-by: Patrick Schönthaler <patrick.schoenthaler@itsc.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants