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

fix: ensure that compute functions are running as non-root #228

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Jul 16, 2024

Related issue

fixes FL-1664

Summary

Notes

Please check if the PR fulfills these requirements

  • If the feature has an impact on the user experience, the changelog has been updated
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • The commit message follows the conventional commit specification

@SdgJlbl SdgJlbl requested a review from a team as a code owner July 16, 2024 09:59
Copy link

linear bot commented Jul 16, 2024

@SdgJlbl SdgJlbl force-pushed the fix/compute-function-non-root branch from 85ed65f to 274b2d0 Compare July 16, 2024 10:00
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Jul 16, 2024

/e2e --tests=substrafl,mnist,doc

@Owlfred
Copy link

Owlfred commented Jul 16, 2024

End to end tests: ❌ FAILURE

Jobs status:

Oh well.

@Substra Substra deleted a comment from Owlfred Jul 16, 2024
@SdgJlbl SdgJlbl force-pushed the fix/compute-function-non-root branch 2 times, most recently from 59627a2 to ba33753 Compare July 22, 2024 14:31
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Jul 22, 2024

/e2e --tests=substrafl

@Owlfred
Copy link

Owlfred commented Jul 22, 2024

End to end tests: ✔️ SUCCESS

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl SdgJlbl force-pushed the fix/compute-function-non-root branch from ba33753 to 9086b1e Compare July 22, 2024 14:43
@Substra Substra deleted a comment from Owlfred Jul 22, 2024
Copy link
Contributor

@thbcmlowk thbcmlowk left a comment

Choose a reason for hiding this comment

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

Nice job! On minor question tho

Comment on lines +73 to +79
# create a non-root user
RUN addgroup --gid 1001 group
RUN adduser --disabled-password --gecos "" --uid 1001 --gid 1001 --home /home/user user
ENV PYTHONPATH /home/user
WORKDIR /home/user
USER user

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow factorize some of this and reuse part of the DOCKERFILE_TEMPLATE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the test. We are checking that the dockerfile template is what we expect it to be.
Else, we would just be checking that Python f-strings are working as intended, and that should be tested in the standard library :)

Copy link
Contributor

@thbcmlowk thbcmlowk Jul 23, 2024

Choose a reason for hiding this comment

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

I meant, I think we could avoid updating twice the constant part (where we just run commands that does not depend on anything else) and extract it in a dedicated variable that could be re-injected in the expected result (precisely because we can assume that (f-)strings work as intended) so the test actually focus on the templating part. But no big deal.

@SdgJlbl SdgJlbl merged commit 1412f88 into main Jul 23, 2024
6 checks passed
@SdgJlbl SdgJlbl deleted the fix/compute-function-non-root branch July 23, 2024 09:09
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.

3 participants