Add support for Pod Identity Association #376
Conversation
| @@ -1,7 +1,7 @@ | |||
| image: | |||
| repository: public.ecr.aws/aws-secrets-manager/secrets-store-csi-driver-provider-aws | |||
| pullPolicy: IfNotPresent | |||
| tag: 1.0.r2-72-gfb78a36-2024.05.29.23.03 | |||
| tag: 0.3.9-st | |||
There was a problem hiding this comment.
This tag corresponds to the docker image version id, we push this image and update the chart information accordingly. Can you remove this from your diff?
There was a problem hiding this comment.
@simonmarty removed tag and .DS_STORE
.DS_Store
Outdated
There was a problem hiding this comment.
Exclude this from the diff
| klog.Errorf("Need IAM role for service account %s (namespace: %s) - %s", p.svcAcc, p.nameSpace, docURL) | ||
| return nil, fmt.Errorf("An IAM role must be associated with service account %s (namespace: %s)", p.svcAcc, p.nameSpace) | ||
| } | ||
| // if len(roleArn) <= 0 { |
There was a problem hiding this comment.
Did you mean to delete this?
There was a problem hiding this comment.
yes ..otherwise we need to have some other condition to know this pod use IRSA or PIA
Current logic is if role-arn annotations in ServiceAcccount is empty then assume its PIA else IRSA
let me know if you have any other suggestion to differentiate between IRSA and PIA
| @@ -1,6 +1,6 @@ | |||
| apiVersion: v2 | |||
| name: secrets-store-csi-driver-provider-aws | |||
| version: 0.3.9 | |||
| version: 0.3.9-st | |||
There was a problem hiding this comment.
Leave version numbers as-is. We'll take care of updating those.
| @@ -41,3 +40,4 @@ securityContext: | |||
| allowPrivilegeEscalation: false | |||
|
|
|||
| useFipsEndpoint: false | |||
| clusterName: xxxx | |||
There was a problem hiding this comment.
Why xxxx instead of an empty key?
server/server.go
Outdated
| @@ -248,7 +242,6 @@ func (s *CSIDriverProviderServer) Version(ctx context.Context, req *v1alpha1.Ver | |||
| // then describing the node to get the region label. | |||
| // | |||
| // See also: https://pkg.go.dev/k8s.io/client-go/kubernetes/typed/core/v1 | |||
| // | |||
There was a problem hiding this comment.
Do these formatting changes serve a practical purpose (legitimately asking)? If the existing formatting is messing up generated docs or whatnot, it's fair to update them. It would be better to do it in a separate PR and focus this one on Pod Identity Association support.
simonmarty
changed the title
|
Thank you submitting the changes. We are conducting initial investigation on this feature request and will share updates soon. |
| }, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| if ctx.Value("roleArn") != nil && ctx.Value("roleArn").(string) == "" { // get token for Pod Identity Assosciation |
There was a problem hiding this comment.
are conditions correct? if roleArn NOT = nil? What about the case where there is no annotation at all?
There was a problem hiding this comment.
yes ..if no role-arn then assume its Pod Identity otherwise we need to have a placeholder to inform drvier that this is using IRSA or PIA
| if err != nil { | ||
| return nil, err | ||
| if len(*roleArn) <= 0 { | ||
| klog.Info("RoleArn is empty so assuming it's Pod Identity and Getting session for Pod Idendity Role for podname - ", podName) |
Issue #, if available: Closes #300
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.