apache / submarine

Submarine is Cloud Native Machine Learning Platform.
https://submarine.apache.org/
Apache License 2.0
689 stars 252 forks source link

SUBMARINE-1417. Retrieve SUBMARINE_AUTH_SECRET from environment variable instead of using hard-coded value #1125

Closed laiyousin closed 4 months ago

laiyousin commented 4 months ago

What is this PR for?

Retrieve SUBMARINE_AUTH_SECRET from environment variable instead of using hard-coded value

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-1417

How should this be tested?

Screenshots (if appropriate)

Questions:

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 43.71%. Comparing base (4e68894) to head (a6a1015).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1125 +/- ## ======================================= Coverage 43.71% 43.71% ======================================= Files 130 130 Lines 6320 6320 ======================================= Hits 2763 2763 Misses 3557 3557 ``` | [Flag](https://app.codecov.io/gh/apache/submarine/pull/1125/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [python-unit](https://app.codecov.io/gh/apache/submarine/pull/1125/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.71% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cdmikechen commented 3 months ago

@laiyousin @xunliu

submarine.auth.default.secret is not hard-coded and can be overridden by SUBMARINE_AUTH_DEFAULT_SECRET in the environment variable. Therefore, I think this PR still needs more consideration, and there are too many problems in CI that have not been solved, and it still needs the support of other fixes to merge.

Refer to the documentation https://submarine.apache.org/docs/designDocs/wip-designs/security-implementation#authentication for more authentication details.

cdmikechen commented 3 months ago

If there is no more to deal with this PR, I will revert this recently and fix the problem in CI first.