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

SecurtyContext: applying PodTemplate for InitContainers and removing SYS_PTRACE #5556

Closed
wants to merge 112 commits into from

Conversation

vlibov
Copy link

@vlibov vlibov commented Jul 11, 2024

Tracking issue

Related to #5462

Why are the changes needed?

The Issue #5462 describes the problem in detail.

What changes were proposed in this pull request?

  1. Remove the SYS_PTRACE capability
  2. Apply the PodTemplate settings to the InitContainers. In particular, this allows the SecurityContext settings to be applied also to the InitContainer.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

welcome bot commented Jul 11, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@vlibov vlibov changed the title Podsecurity SecurtyContext: applying PodTemplate for InitContainers and removing SYS_PTRACE Jul 11, 2024
@davidmirror-ops davidmirror-ops requested a review from hamersaw July 11, 2024 17:36
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.75%. Comparing base (81afb76) to head (92bd3a0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5556      +/-   ##
==========================================
- Coverage   60.98%   60.75%   -0.23%     
==========================================
  Files         796      645     -151     
  Lines       51647    41432   -10215     
==========================================
- Hits        31498    25174    -6324     
+ Misses      17249    13958    -3291     
+ Partials     2900     2300     -600     
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.69% <ø> (-0.05%) ⬇️
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 67.44% <ø> (ø)
unittests-flyteidl 79.06% <ø> (ø)
unittests-flyteplugins ?
unittests-flytepropeller 57.42% <ø> (ø)
unittests-flytestdlib 65.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -172,7 +172,10 @@ func AddCoPilotToContainer(ctx context.Context, cfg config.FlyteCoPilotConfig, c
if c.SecurityContext.Capabilities == nil {
c.SecurityContext.Capabilities = &v1.Capabilities{}
}
c.SecurityContext.Capabilities.Add = append(c.SecurityContext.Capabilities.Add, pTraceCapability)
var add_sys_ptrace bool = false
if add_sys_ptrace {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it always false here? Should we add add_sys_ptrace to the plugin config?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a larger conversation here, which I suspect there is. We can split this into multiple PRs - I don't think merging support for applying a PodTemplate to initContainers is controversial.

Copy link
Contributor

@davidmirror-ops davidmirror-ops Jul 22, 2024

Choose a reason for hiding this comment

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

We can split this into multiple PRs

Agree. @vlibov thoughts?

Copy link
Author

@vlibov vlibov Jul 24, 2024

Choose a reason for hiding this comment

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

Hi all,
yes it's always false here - the idea was that this SYS_PTRACE can be removed altogether
(but I wanted to keep the relevant code for now so didn't remove the line completely).
However, indeed a more flexible approach would be to make this configurable.

I'm fine with both options - splitting the PR or deciding whether we remvoe SYS_PTRACE vs make it configurable via add_sys_ptrace now. How difficult is it to create a new plugin config? if not too difficult, we could just do it.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @davidmirror-ops,
how do we want to proceed? Shall we split the PR or make the SYS_PTRACE configurable (or remove it completely?)
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we split the PR or make the SYS_PTRACE configurable
Hey @vlibov! Can we do both? Split this into two PRs: one for the PodTemplate for initContainer and the other making SYS_PTRACE configurable.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, @davidmirror-ops!
I reverted the SYS_PTRACE commit in this PR so it now only affects the initContainer part.

I will also create a new PR for the SYS_PTRACE config - can anybody point me on how to properly implement this? i.e. where is this option supposed to be defined and how to propagate it down to the relevant function which sets SYS_PTRACE? Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Hey @davidmirror-ops, actually I created a new PR also for this change (initContainers)
#5664
I hope this is ok!
The #5556 can thus be closed...

Choose a reason for hiding this comment

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

Hi @vlibov, just wondering did you end up creating a separate PR for removing SYS_PTRACE?

vlibov and others added 17 commits August 16, 2024 10:16
This reverts commit ce0b4fc.
We decided to split the PR flyteorg#5556 into two, therefore reverting the relevant change.
This reverts commit ce0b4fc.
We decided to split the PR flyteorg#5556 into two, therefore reverting the relevant change.

Signed-off-by: Vladyslav Libov <[email protected]>
…mary/sidecar containers)

Signed-off-by: Vladyslav Libov <[email protected]>
…ws (flyteorg#5463)

* Fix: Make 'flytectl compile' consider launchplans used within workflows

Signed-off-by: Dennis Keck <[email protected]>

* Add raw file for test

Signed-off-by: Dennis Keck <[email protected]>

* Add documentation on how to create a package

Signed-off-by: Dennis Keck <[email protected]>

---------

Signed-off-by: Dennis Keck <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Trevor McGuire <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
…teorg#4726)

* Add ShowWhilePending arg to TaskLog flyteidl message

Signed-off-by: Fabio Graetz <[email protected]>

* Allow showing specific logs already during queued phase

Signed-off-by: Fabio Graetz <[email protected]>

* Use core.PhaseInfoQueuedWithTaskInfo instead of core.PhaseInfoQueued in plugins so log links are available

Signed-off-by: Fabio Graetz <[email protected]>

* Bump phase version in pytorch plugin

Signed-off-by: Fabio Graetz <[email protected]>

* Fix nil containerId in pending phase

Signed-off-by: Fabio Graetz <[email protected]>

* Undo changes from rebase in ray.go

Signed-off-by: Fabio Graetz <[email protected]>

* Regenerate protos

Signed-off-by: Fabio Graetz <[email protected]>

* Fix after rebasing

Signed-off-by: Fabio Graetz <[email protected]>

* Add HideOnceFinished option to TaskLog proto message

Signed-off-by: Fabio Graetz <[email protected]>

* Hide certain logs once finished

Signed-off-by: Fabio Graetz <[email protected]>

* Move log link filtering (by phase) from propeller to admin

Signed-off-by: Fabio Graetz <[email protected]>

* Move bumping of plugin state phase version into function

Signed-off-by: Fabio Graetz <[email protected]>

* Move helper function which bumps phase version to k8s plugin package

Signed-off-by: Fabio Graetz <[email protected]>

* Consistently bump phase version when reason changes in pod, pytorch, tensorflow, and mpi plugins

Signed-off-by: Fabio Graetz <[email protected]>

* Make controlling lifetime of log links work with dask plugin

Signed-off-by: Fabio Graetz <[email protected]>

* Make controlling lifetime of log links work with ray plugin

Signed-off-by: Fabio Graetz <[email protected]>

* Make controlling lifetime of log links work with spark plugin

Signed-off-by: Fabio Graetz <[email protected]>

* Don't return pluginsCore.PhaseInfoUndefined but already known phaseInfo if we fail to update the phase version

Signed-off-by: Fabio Graetz <[email protected]>

* Remove now obsolete logic to check whether dask job is queued

Signed-off-by: Fabio Graetz <[email protected]>

* Adapt docstring explaining why we treat queued and init phase the same while filtering log links

Signed-off-by: Fabio Graetz <[email protected]>

* Make propeller tests pass

Signed-off-by: Fabio Graetz <[email protected]>

* Make pluginmachinery/flytek8s tests pass

Signed-off-by: Fabio Graetz <[email protected]>

* Fix dask, pytorch, tensorflow, and mpi tests

Signed-off-by: Fabio Graetz <[email protected]>

* Make log link filtering by phase work for map tasks

Signed-off-by: Fabio Graetz <[email protected]>

* Add tests for filtering log links when updating task execution

Signed-off-by: Fabio Graetz <[email protected]>

* Show All user logs while queueing phase as before

Signed-off-by: Fabio Graetz <[email protected]>

* Fix spark tests

Signed-off-by: Fabio Graetz <[email protected]>

* Fix after rebase

Signed-off-by: Fabio Graetz <[email protected]>

* Fix flyteidl go.mod

Signed-off-by: Fabio Graetz <[email protected]>

* Fix mpi test

Signed-off-by: Fabio Graetz <[email protected]>

* Add tests for PR flyteorg#4726 (flyteorg#5200)

* Add tests to ensure the phase version is bumped in kubeflow plugin if reason changes within the same phase

Signed-off-by: Fabio Graetz <[email protected]>

* Test that ray and dask plugins bump phase version in GetTaskPhase

Signed-off-by: Fabio Graetz <[email protected]>

* Test phase version increase when reason changes for spark plugin

Signed-off-by: Fabio Graetz <[email protected]>

* Fix ray tests after rebase

Signed-off-by: Fabio Graetz <[email protected]>

* Make lint pass

Signed-off-by: Fabio Graetz <[email protected]>

---------

Signed-off-by: Fabio Graetz <[email protected]>

* Update flyteplugins/go/tasks/logs/logging_utils.go

Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>

* Update go.mod after flyteidl make generate

Signed-off-by: Fabio Graetz <[email protected]>

* Restrict numpy version in single binary e2e tests

Signed-off-by: Fabio Graetz <[email protected]>

---------

Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
…target (flyteorg#5484)

* Favor golangci-lint --fix instead running goimports,gci directly

Signed-off-by: Jason Parraga <[email protected]>

* Enable gci lint on flytectl

Signed-off-by: Jason Parraga <[email protected]>

* check in gci fixes on flytectl

Signed-off-by: Jason Parraga <[email protected]>

* Deduplicate per feedback

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: zychen5186 <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* keep EnvFrom from pod template

not complete nor tested, just as hint for potential fix for regression
in flyteorg#4969

* Add test and fix build error

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix test

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
katrogan and others added 27 commits August 16, 2024 11:25
* Upgrade docker dependency to address vulnerability

Signed-off-by: Katrina Rogan <[email protected]>

* update

Signed-off-by: Katrina Rogan <[email protected]>

---------

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* Support offloading workflow CRD inputs

Signed-off-by: Katrina Rogan <[email protected]>

* Add tests

Signed-off-by: Katrina Rogan <[email protected]>

* not just single tasks

Signed-off-by: Katrina Rogan <[email protected]>

* lint

Signed-off-by: Katrina Rogan <[email protected]>

---------

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* Refactor panic handling to middleware

Signed-off-by: Jason Parraga <[email protected]>

* Remove registration of old panicCounter

Signed-off-by: Jason Parraga <[email protected]>

* Add test coverage

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* TEST build

Signed-off-by: Future-Outlier <[email protected]>

* remove emphasize-lines

Signed-off-by: Future-Outlier <[email protected]>

* test build

Signed-off-by: Future-Outlier <[email protected]>

* revert

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
…teorg#5612)

* FlytePropeller Compiler Avoid Crash when Type not found

Signed-off-by: Future-Outlier <[email protected]>

* Update pingsu's error message advices

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>

* fix lint

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* first version

Signed-off-by: Future-Outlier <[email protected]>

* update

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* add send arg

Signed-off-by: Yee Hing Tong <[email protected]>

* Add acction to remove cache in gh runner

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use correct checked out path

Signed-off-by: Eduardo Apolinario <[email protected]>

* Path in strings

Signed-off-by: Eduardo Apolinario <[email protected]>

* Checkout repo in root

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use the correct path to new action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Do not use gh var in path to clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove wrong invocation of clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* GITHUB_WORKSPACE is implicit in the checkout action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Refer to local `flyte` directory

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
…g#5635)

* Make flyteidl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Make flytectl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Rewrite the documentation for `version` and clarify wording in RELEASE.md

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* fix CHANGELOG-v0.2.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.0.2-b1.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.1.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.3.0.md

Signed-off-by: Christina <[email protected]>

---------

Signed-off-by: Christina <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* Fetch all tags in flyteidl-release.yml

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix sed expression for npm job

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
…g#5545)

* update

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* typo

Signed-off-by: Desi Hsu <[email protected]>

---------

Signed-off-by: Desi Hsu <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
flyteorg#5649)

* Don't error when attempting to trigger schedules for inactive projects

Signed-off-by: Katrina Rogan <[email protected]>

* regen

Signed-off-by: Katrina Rogan <[email protected]>

---------

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* Update Flyte Components

Signed-off-by: Flyte-Bot <[email protected]>

* Bump conf.py and add changelog

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Flyte-Bot <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: eapolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
* Improve error messaging for invalid arguments

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* Define a separate identifier for the launchplan test

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
This reverts commit ce0b4fc.
We decided to split the PR flyteorg#5556 into two, therefore reverting the relevant change.

Signed-off-by: Vladyslav Libov <[email protected]>
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.