[flyte-core] Flyte Connection #5126
Conversation
Signed-off-by: Kevin Su <pingsutw@gmail.com>
Signed-off-by: Kevin Su <pingsutw@gmail.com>
Signed-off-by: Kevin Su <pingsutw@gmail.com>
dosubot
bot
added
size:XXL
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5126 +/- ##
==========================================
- Coverage 61.10% 60.23% -0.88%
==========================================
Files 794 647 -147
Lines 51215 45739 -5476
==========================================
- Hits 31296 27551 -3745
+ Misses 17038 15593 -1445
+ Partials 2881 2595 -286
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@pingsutw Would it be possible to merge the work of this PR with that of my PR? It seems like both include secrets in requests made to the agent, but this PR goes a step farther by resolving the value of secrets. On the other hand, my PR includes more details about which user is executing a task, which is also very useful for the agent to have. |
pingsutw
changed the title
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
flytepropeller/pkg/controller/nodes/task/connectionmanager/config.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
pingsutw
changed the title
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
| @@ -36,6 +36,9 @@ enum MatchableResource { | |||
|
|
|||
| // Controls how to select an available cluster on which this execution should run. | |||
| CLUSTER_ASSIGNMENT = 7; | |||
|
|
|||
| // Configures the task connection to be used by the agent to connect to external systems. | |||
| EXTERNAL_RESOURCE = 8; | |||
There was a problem hiding this comment.
We could extend this to be used by regular tasks too right ?
There was a problem hiding this comment.
What do you mean by extending here? something like
@task(external_resource=...)
# or
@task(connection=...)| // Flyte will use the default connection in the project-domain settings, but users | ||
| // still be able to override it by specifying the connection in the task decorator. | ||
| // +optional | ||
| string connection = 4; |
There was a problem hiding this comment.
Could we make a note of the order of overrides as its just not project-domain settings alone. Also can we add details on how this should map to values in external resource attributes
| @@ -45,6 +45,14 @@ message Secret { | |||
| MountType mount_requirement = 4; | |||
| } | |||
|
|
|||
| message Connection { | |||
| // The credentials to use for the connection, such as API keys, OAuth2 tokens, etc. | |||
| map<string, string> secrets = 1; | |||
There was a problem hiding this comment.
Can we also add comment on the format of what the key and what the value is and how they map to what the agent expects
| map<string, string> secrets = 1; | ||
|
|
||
| // The configuration to use for the connection, such as the endpoint, account name, etc. | ||
| map<string, string> configs = 2; |
There was a problem hiding this comment.
Similalrly for the configs
| @@ -97,6 +99,24 @@ func (p Plugin) Create(ctx context.Context, taskCtx webapi.TaskExecutionContextR | |||
| taskCategory := admin.TaskCategory{Name: taskTemplate.Type, Version: taskTemplate.TaskTypeVersion} | |||
| agent, isSync := getFinalAgent(&taskCategory, p.cfg, p.agentRegistry) | |||
|
|
|||
| connection := flyteIdl.Connection{} | |||
| if taskTemplate.SecurityContext != nil && taskTemplate.SecurityContext.Connection != "" { | |||
| conn, ok := taskCtx.TaskExecutionMetadata().GetExternalResourceAttributes().GetConnections()[taskTemplate.SecurityContext.Connection] | |||
There was a problem hiding this comment.
Can we use getters for taskTemplate.SecurityContext.Connection and would be cleaner to define this as connectionKey may be as we are not defining the connection in the security context but just a ref/key. May be connectionRef might also be ok
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Tracking issue
#3936
Why are the changes needed?
It will allow users to pass different API_KEY to the chatGPT agent from flytepropeller
What changes were proposed in this pull request?
Agent client will read the secret from env or local file, and pass it to the agent server.
How was this patch tested?
local sandbox
Setup process
Add below config to
flyte-single-binary-local.yamlm and Add your API key to local envexternalResources: connections: chatgpt: secrets: openai_api_key: OPENAI_API_KEY configs: openai_organization: org-P2rdnZQry4Fw7Ak3vSpXEIrxcd flyte pyflyte serve agent make compile flyte start --config flyte-single-binary-local.yamlScreenshots
Secret not found in propeller
Related PRs
flyteorg/flytekit#2297
Docs link
NA