Inject and Use values from Security Context #153
Conversation
anandswaminathan
force-pushed
the
security-override
branch
from
e1bffe0 to
1f0526d
Compare
| @@ -50,7 +50,12 @@ func EnsureJobDefinition(ctx context.Context, tCtx pluginCore.TaskExecutionConte | |||
| return nil, errors.Errorf(pluginErrors.BadTaskSpecification, "Tasktemplate does not contain a container image.") | |||
| } | |||
|
|
|||
| role := awsUtils.GetRole(ctx, cfg.RoleAnnotationKey, tCtx.TaskExecutionMetadata().GetAnnotations()) | |||
| role := awsUtils.GetRole(ctx, cfg.RoleSecurityContextKey, tCtx.TaskExecutionMetadata().GetSecurityContext()) | |||
There was a problem hiding this comment.
should we make this into a protobuf enum - cfg.RoleSecurityContextKey
There was a problem hiding this comment.
I am not inclined for this one. But for serviceAccountName may be? What do you think ?
There was a problem hiding this comment.
Why not for Role - AWS Batch, Sagemaker, Athena etc may all need a role, then how do we make it consistent
|
PTAL @akhurana001 This is a backward incompatible for SPARK - right fix. If someone passes the ServiceAccount. Spark plugin has to respect it. |
I am still wrapping my head around why users of the platform need to know about service accounts or K8s concepts. How can users roll-out this change safely if this is backward incompatible ? Can we default to |
|
@akhurana001 Let me clarify
|
Signed-off-by: Anand Swaminathan <[email protected]>
anandswaminathan
force-pushed
the
security-override
branch
from
5ce44ca to
761e9e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #153 +/- ##
==========================================
+ Coverage 60.33% 60.37% +0.03%
==========================================
Files 130 130
Lines 7027 7044 +17
==========================================
+ Hits 4240 4253 +13
- Misses 2362 2364 +2
- Partials 425 427 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| role := awsUtils.GetRole(ctx, cfg.RoleAnnotationKey, taskCtx.TaskExecutionMetadata().GetAnnotations()) | ||
| if role == "" { | ||
| role := awsUtils.GetRoleFromSecurityContext(taskCtx.TaskExecutionMetadata().GetSecurityContext()) | ||
|
|
There was a problem hiding this comment.
nit: helper method since looks like this is being repetitive ?
| role := awsUtils.GetRoleFromSecurityContext(taskCtx.TaskExecutionMetadata().GetSecurityContext()) | ||
|
|
||
| // Continue this for backward compatibility | ||
| if len(role) == 0 { |
There was a problem hiding this comment.
We should probably encapsulate the backward compat code in one place... so we can deprecate it easier and avoid new plugins having to know to do that...
|
LGTM , one minor comment. Also can we test this if not already :) |
Signed-off-by: Anand Swaminathan <[email protected]>
anandswaminathan
force-pushed
the
security-override
branch
from
d0e67d4 to
17c1f2e
Compare
Signed-off-by: Anand Swaminathan <[email protected]>
Signed-off-by: Anand Swaminathan <[email protected]>
anandswaminathan
force-pushed
the
security-override
branch
from
a75483a to
2aa7875
Compare
* Inject and Use values from Security Context Signed-off-by: Anand Swaminathan <[email protected]> Signed-off-by: Chao-Han Tsai <[email protected]>
* Inject and Use values from Security Context Signed-off-by: Anand Swaminathan <[email protected]>
This change is related to deprecation on auth from Flyte and injecting SecurityContext.
Type
@EngHabu @kumare3