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.

Sign up for GitHub

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

Enable SecretsManager.get to load and return bytes #1798

Merged
merged 4 commits into from
Aug 25, 2023
Merged

Enable SecretsManager.get to load and return bytes #1798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd1795c
fix secretsmanager
ysysys3074 Aug 17, 2023
1e95a27
fix lint issue
ysysys3074 Aug 18, 2023
850ff3e
add doc
ysysys3074 Aug 21, 2023
df4b188
fix github check
ysysys3074 Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions flytekit/core/context_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,12 @@ def __getattr__(self, item: str) -> _GroupSecrets:
"""
return self._GroupSecrets(item, self)

def get(self, group: str, key: Optional[str] = None, group_version: Optional[str] = None) -> str:
def get(
self, group: str, key: Optional[str] = None, group_version: Optional[str] = None, encode_mode: str = "r"
Copy link
Collaborator

@eapolinario eapolinario Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

what if we make this new parameter an enum (containing only 2 options: "r" or "rb")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, I will add in next commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think using string is ok. It is consistent with what we pass to with open(fpath, <here>) . Enum seems a bit overkilled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. At the very least put a description in the function docstring, @ysysys3074 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can do a literal type if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eapolinario added in new commit

) -> str:
"""
Retrieves a secret using the resolution order -> Env followed by file. If not found raises a ValueError
param encode_mode, defines the mode to open files, it can either be "r" to read file, or "rb" to read binary file
"""
self.check_group_key(group)
env_var = self.get_secrets_env_var(group, key, group_version)
Expand All @@ -360,7 +363,7 @@ def get(self, group: str, key: Optional[str] = None, group_version: Optional[str
if v is not None:
return v
if os.path.exists(fpath):
with open(fpath, "r") as f:
with open(fpath, encode_mode) as f:
return f.read().strip()
raise ValueError(
f"Unable to find secret for key {key} in group {group} " f"in Env Var:{env_var} and FilePath: {fpath}"
Expand Down
10 changes: 10 additions & 0 deletions tests/flytekit/unit/core/test_context_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import base64
import os
from datetime import datetime

Expand Down Expand Up @@ -166,6 +167,15 @@ def test_secrets_manager_file(tmpdir: py.path.local):
w.write("my-password")
assert sec.get("group", "test") == "my-password"
assert sec.group.test == "my-password"

base64_string = "R2Vla3NGb3JHZWV =="
base64_bytes = base64_string.encode("ascii")
base64_str = base64.b64encode(base64_bytes)
with open(f, "wb") as w:
w.write(base64_str)
assert sec.get("group", "test") != base64_str
assert sec.get("group", "test", encode_mode="rb") == base64_str

del os.environ["FLYTE_SECRETS_DEFAULT_DIR"]


Expand Down