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

add missing secret_requests to ContainerTask #1307

Merged

Conversation

flixr
Copy link
Contributor

@flixr flixr commented Nov 4, 2022

Signed-off-by: Felix Ruess felix.ruess@roboception.de

TL;DR

add missing secret_requests to ContainerTask

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Do the same as in other container tasks...

Tracking Issue

Follow-up issue

NA

Signed-off-by: Felix Ruess <felix.ruess@roboception.de>
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #1307 (3f68129) into master (aa1e8fa) will decrease coverage by 0.01%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
- Coverage   68.87%   68.85%   -0.02%     
==========================================
  Files         287      287              
  Lines       26429    26436       +7     
  Branches     2495     2498       +3     
==========================================
+ Hits        18202    18203       +1     
- Misses       7743     7748       +5     
- Partials      484      485       +1     
Impacted Files Coverage Δ
flytekit/core/container_task.py 27.45% <14.28%> (-2.10%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@flixr
Copy link
Contributor Author

flixr commented Nov 4, 2022

So while with this I can inject secrets into my container "the flyte way", it doesn't quite do what I expected/need.
What I really need is a way to define env vars using secret data.

@wild-endeavor
Copy link
Contributor

@flixr - the flytekit Secret does have a mount type. Does that do what you need? If not, you can use a straight up pod task too.

@flixr
Copy link
Contributor Author

flixr commented Nov 4, 2022

@wild-endeavor yeah, I used Secret(group="minio-write-creds", key="AWS_ACCESS_KEY_ID", mount_requirement=Secret.MountType.ENV_VAR) and it did inject it as an env var.
The point is that the name of the injected env var needs to be AWS_ACCESS_KEY_ID, but flyte injects it as _FSEC_MINIO_WRITE_CREDS_AWS_ACCESS_KEY_ID.
And there seems to be no way to influence this, as it is hardcoded in flytepropeller.
With normal k8s secrets I can specify the name of the env var, so there I can do it:
https://kubernetes.io/docs/tasks/inject-data-application/distribute-credentials-secure/#define-a-container-environment-variable-with-data-from-a-single-secret
But how to achieve that for my flyte ContainerTasks?

@wild-endeavor
Copy link
Contributor

Merging this for now... thanks! Can you make a new issue in the flyte repo and just copy in your last comment? I'm going to ask around and see what can be done. It does feel a bit weird that we're prepending seemingly random characters to the env var.

Just curious though, wrt this env var specifically, you're going with secrets instead of linking up iam roles to the eks service accounts?

@wild-endeavor wild-endeavor merged commit 3b5d15a into flyteorg:master Nov 4, 2022
@flixr flixr deleted the container_task_secret_requests branch November 5, 2022 00:14
@flixr
Copy link
Contributor Author

flixr commented Nov 5, 2022

I'm not running on EKS, but on my on-prem k3s cluster and how would I even change the service account used for my tasks?
And depending on the task, I need to access different S3 buckets (actually separate minio cluster), also with different credentials, some with write and some with read-only access.

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.

2 participants