Secrets Manager and secrets specification for tasks #406

kumare3 · 2021-03-04T00:39:04Z

Signed-off-by: Ketan Umare [email protected]

TL;DR

[wip]Pending adding secrets to flyteidl tasktemplate
This enables every python task or a python plugin to request for secrets and the secrets are retrieved in a standardized way during the execution using the flytekit context's new SecretsManager.

SecretsManager will resolve the secrets by first looking at Env Var and then at the configured secrets directory.
Rules:

the env var should be uppercased and should be prefixed with the configured prefix. Default will be FSEC{secret_key}
If env var is not found, then Secretsmanager will look for a file under {default_dir}/{prefix}{secret_key}
default_dir is set to /etc/secrets (can be overriden using an env var)
prefix is set to ""

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Tracking Issue

flyteorg/flyte#796
flyteorg/flyte#800

Signed-off-by: Ketan Umare <[email protected]>

codecov · 2021-03-09T21:14:33Z

Codecov Report

Merging #406 (93afa0e) into master (c7ff1e8) will increase coverage by 0.09%.
The diff coverage is 94.95%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   83.98%   84.07%   +0.09%     
==========================================
  Files         337      340       +3     
  Lines       24955    25166     +211     
  Branches     2028     2048      +20     
==========================================
+ Hits        20959    21159     +200     
- Misses       3441     3449       +8     
- Partials      555      558       +3

Impacted Files	Coverage Δ
flytekit/common/translator.py	`90.08% <ø> (ø)`
flytekit/models/security.py	`86.84% <86.84%> (ø)`
tests/flytekit/unit/core/test_type_hints.py	`95.54% <93.75%> (-0.06%)`	⬇️
flytekit/__init__.py	`100.00% <100.00%> (ø)`
flytekit/common/tasks/sdk_runnable.py	`84.21% <100.00%> (+2.23%)`	⬆️
flytekit/common/tasks/task.py	`62.89% <100.00%> (ø)`
flytekit/configuration/secrets.py	`100.00% <100.00%> (ø)`
flytekit/core/base_task.py	`84.92% <100.00%> (+0.38%)`	⬆️
flytekit/core/python_auto_container.py	`82.14% <100.00%> (+1.19%)`	⬆️
flytekit/core/task.py	`87.80% <100.00%> (+0.30%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ff1e8...93afa0e. Read the comment docs.

Signed-off-by: Ketan Umare <[email protected]>

Signed-off-by: wild-endeavor <[email protected]>

Signed-off-by: Ketan Umare <[email protected]>

wild-endeavor

I feel like the user interface for secrets will evolve as people start using it (and maybe possibly as we add an authz layer). Not sure how exactly though.

wild-endeavor · 2021-03-11T05:24:43Z

tests/flytekit/unit/core/test_type_hints.py

+
+
+def test_secrets():
+    @task(secret_requests=["my_secret"])


Should this be Secret("my_secret")

aah yes it should be, and thus in this case maybe i should do a check on this

Signed-off-by: Ketan Umare <[email protected]>

cosmicBboy · 2021-03-11T16:09:20Z

flytekit/core/base_task.py

@@ -304,16 +311,21 @@ def __init__(
        name: str,
        task_config: T,
        interface: Optional[Interface] = None,
-        environment=None,
-        task_type_version=0,
+        environment: Optional[Dict[str, str]] = None,


why are we removing task_type_version?

not removing, i am keeping it so that, if something is declared in the base, it does not need to repeated at every class layer - **kwargs should take care of it?

cosmicBboy · 2021-03-11T16:12:52Z

flytekit/common/tasks/sdk_runnable.py

+            return v
+        if os.path.exists(fpath):
+            with open(fpath, "r") as f:
+                return f.read()


maybe a strip() here in case of newlines??

cosmicBboy · 2021-03-11T16:14:50Z

flytekit/models/security.py

+    class MountType(Enum):
+        ENV_VAR = _sec.Secret.MountType.ENV_VAR
+        """
+        Use this if the secret can be injected as an environment variable. Usually works for symmetric keys, passwords etc


just a question: is this way of using multi-line strings as in-line comments a convention, or important for sphinx docs?

Thats what I understand, that sphinx will convert this to a doc-string on the constant

Signed-off-by: Ketan Umare <[email protected]>

EngHabu · 2021-03-22T21:53:10Z

flytekit/common/tasks/sdk_runnable.py

+    The resolution order is
+      - Try env var first. The env var should have the configuration.SECRETS_ENV_PREFIX. The env var will be all upper
+         cased
+      - If not then try the file where the name matches the key as lower case in configuration.SECRETS_DEFAULT_DIR


This needs to be updated right? it will be <DEFAULT_DIR>/Group/<PREFIX>Key

EngHabu · 2021-03-22T21:55:30Z

flytekit/configuration/secrets.py

+FLYTE_SECRETS_ENV_PREFIX variable
+"""
+
+SECRETS_DEFAULT_DIR = _common_config.FlyteStringConfigurationEntry("secrets", "default_dir", default="/etc/secrets")


for /etc/secrets you mean?

EngHabu · 2021-03-22T22:00:53Z

flytekit/models/security.py

+    group: str
+    key: str
+    group_version: Optional[str] = None
+    mount_requirement: MountType = MountType.ENV_VAR


Should this be ANY?

ANY what? - for MountType?

Signed-off-by: Ketan Umare <[email protected]>

EngHabu · 2021-03-22T23:46:22Z

flytekit/configuration/secrets.py

+"""
+
+SECRETS_DEFAULT_DIR = _common_config.FlyteStringConfigurationEntry(
+    "secrets", "default_dir", default=os.path.join("etc", "secrets")


Suggested change

"secrets", "default_dir", default=os.path.join("etc", "secrets")

"secrets", "default_dir", default=os.path.join(os.path.sep, "etc", "secrets")

Signed-off-by: Ketan Umare <[email protected]>

* Secrets Manager and secrets specification for tasks Signed-off-by: Ketan Umare <[email protected]> * Updated setup.py Signed-off-by: Ketan Umare <[email protected]> * requirements updated Signed-off-by: Ketan Umare <[email protected]> * Unit tests and models for security context Signed-off-by: Ketan Umare <[email protected]> * Updated test Signed-off-by: Ketan Umare <[email protected]> * Tests and exposed methods Signed-off-by: Ketan Umare <[email protected]> * SecretManager available in testing Signed-off-by: Ketan Umare <[email protected]> * requirements updated Signed-off-by: Ketan Umare <[email protected]> * ran make requirements Signed-off-by: wild-endeavor <[email protected]> * Fixed test Signed-off-by: Ketan Umare <[email protected]> * Fixed test and check that secrets are not bare strings Signed-off-by: Ketan Umare <[email protected]> * Updated to use new FlyteIDL Signed-off-by: Ketan Umare <[email protected]> * Updated group secrets Signed-off-by: Ketan Umare <[email protected]> * Updated secrets with group concat logic Signed-off-by: Ketan Umare <[email protected]> * wip Signed-off-by: Ketan Umare <[email protected]> * Updated to flyteidl 0.18.24. Secrets with version Signed-off-by: Ketan Umare <[email protected]> * Updated secrets, to always require group Signed-off-by: Ketan Umare <[email protected]> * addressed comments Signed-off-by: Ketan Umare <[email protected]> * updated to address comments Signed-off-by: Ketan Umare <[email protected]> * updated linter fix Signed-off-by: Ketan Umare <[email protected]> * Updated to address more comments Signed-off-by: Ketan Umare <[email protected]> * remove faulty test Signed-off-by: Ketan Umare <[email protected]> * update os.sep Signed-off-by: Ketan Umare <[email protected]> Co-authored-by: wild-endeavor <[email protected]>

Secrets Manager and secrets specification for tasks

15f36f7

Signed-off-by: Ketan Umare <[email protected]>

kumare3 requested review from honnix, katrogan and wild-endeavor as code owners March 4, 2021 00:39

kumare3 added 3 commits March 9, 2021 11:56

Updated setup.py

ecf3a4e

Signed-off-by: Ketan Umare <[email protected]>

Merge branch 'master' into secrets

eef6a5f

requirements updated

bc924b2

Signed-off-by: Ketan Umare <[email protected]>

kumare3 and others added 8 commits March 10, 2021 13:03

Unit tests and models for security context

989e3e1

Signed-off-by: Ketan Umare <[email protected]>

Updated test

664dbde

Signed-off-by: Ketan Umare <[email protected]>

Tests and exposed methods

9fe1af5

Signed-off-by: Ketan Umare <[email protected]>

SecretManager available in testing

cbdc0c0

Signed-off-by: Ketan Umare <[email protected]>

requirements updated

7859172

Signed-off-by: Ketan Umare <[email protected]>

Merge branch 'master' into secrets

073b5bc

ran make requirements

360fb40

Signed-off-by: wild-endeavor <[email protected]>

Fixed test

2a28926

Signed-off-by: Ketan Umare <[email protected]>

wild-endeavor previously approved these changes Mar 11, 2021

View reviewed changes

wild-endeavor reviewed Mar 11, 2021

View reviewed changes

kumare3 changed the title ~~[wip] Secrets Manager and secrets specification for tasks~~ Secrets Manager and secrets specification for tasks Mar 11, 2021

Fixed test and check that secrets are not bare strings

63a81c1

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed wild-endeavor’s stale review via 63a81c1 March 11, 2021 05:37

cosmicBboy reviewed Mar 11, 2021

View reviewed changes

kumare3 added 8 commits March 13, 2021 15:11

Merge branch 'master' into secrets

fecbbcc

Updated to use new FlyteIDL

f583cd8

Signed-off-by: Ketan Umare <[email protected]>

Updated group secrets

26f42ae

Signed-off-by: Ketan Umare <[email protected]>

Updated secrets with group concat logic

8430e60

Signed-off-by: Ketan Umare <[email protected]>

Merge branch 'master' into secrets

7215a69

Merge branch 'master' into secrets

739c874

Merge branch 'master' into secrets

61e3ad8

wip

8e7b5c9

Signed-off-by: Ketan Umare <[email protected]>

kumare3 added 3 commits March 19, 2021 11:40

Updated to flyteidl 0.18.24. Secrets with version

4f6b7dc

Signed-off-by: Ketan Umare <[email protected]>

Updated secrets, to always require group

a629fcd

Signed-off-by: Ketan Umare <[email protected]>

addressed comments

b434f67

Signed-off-by: Ketan Umare <[email protected]>

EngHabu previously approved these changes Mar 22, 2021

View reviewed changes

updated to address comments

38d9c9c

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed EngHabu’s stale review via 38d9c9c March 22, 2021 22:31

updated linter fix

59d5c28

Signed-off-by: Ketan Umare <[email protected]>

wild-endeavor previously approved these changes Mar 22, 2021

View reviewed changes

Updated to address more comments

5455c87

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed wild-endeavor’s stale review via 5455c87 March 22, 2021 22:54

wild-endeavor previously approved these changes Mar 22, 2021

View reviewed changes

EngHabu reviewed Mar 22, 2021

View reviewed changes

remove faulty test

4b1c266

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed wild-endeavor’s stale review via 4b1c266 March 23, 2021 00:19

update os.sep

93afa0e

Signed-off-by: Ketan Umare <[email protected]>

EngHabu approved these changes Mar 23, 2021

View reviewed changes

kumare3 merged commit 74fdf44 into master Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secrets Manager and secrets specification for tasks #406

Secrets Manager and secrets specification for tasks #406

kumare3 commented Mar 4, 2021 •

edited by EngHabu

Loading

codecov bot commented Mar 9, 2021 •

edited

Loading

wild-endeavor left a comment

wild-endeavor Mar 11, 2021

kumare3 Mar 11, 2021

kumare3 Mar 21, 2021

cosmicBboy Mar 11, 2021

kumare3 Mar 11, 2021

cosmicBboy Mar 11, 2021

kumare3 Mar 11, 2021

kumare3 Mar 21, 2021

cosmicBboy Mar 11, 2021

kumare3 Mar 11, 2021

EngHabu Mar 22, 2021

EngHabu Mar 22, 2021

kumare3 Mar 22, 2021

EngHabu Mar 22, 2021

kumare3 Mar 22, 2021 •

edited

Loading

EngHabu Mar 22, 2021

	"secrets", "default_dir", default=os.path.join("etc", "secrets")
	"secrets", "default_dir", default=os.path.join(os.path.sep, "etc", "secrets")

Secrets Manager and secrets specification for tasks #406

Secrets Manager and secrets specification for tasks #406

Conversation

kumare3 commented Mar 4, 2021 • edited by EngHabu Loading

TL;DR

Type

Are all requirements met?

Tracking Issue

codecov bot commented Mar 9, 2021 • edited Loading

Codecov Report

wild-endeavor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 commented Mar 4, 2021 •

edited by EngHabu

Loading

codecov bot commented Mar 9, 2021 •

edited

Loading

kumare3 Mar 22, 2021 •

edited

Loading