feat: adjust the provider to use the available service Account annotations instead of requiring it again in the SPC parameters #1443
Conversation
|
@microsoft-github-policy-service agree |
wenzel-felix
changed the title
wenzel-felix
force-pushed
the
feature/use_clientID_from_serviceAccount
branch
2 times, most recently
from
8c472e0 to
2a3335b
Compare
…instead of requiring it again in the SPC parameters
wenzel-felix
force-pushed
the
feature/use_clientID_from_serviceAccount
branch
from
2a3335b to
0afa0ee
Compare
website/content/en/configurations/identity-access-modes/workload-identity-mode.md
Outdated
Show resolved
Hide resolved
|
This PR is stale because it has been open 14 days with no activity. Please comment or this will be closed in 7 days. |
|
This PR is stale because it has been open 14 days with no activity. Please comment or this will be closed in 7 days. |
|
Please review the PR |
|
@nilekhc, could you please review the PR |
|
This PR is stale because it has been open 14 days with no activity. Please comment or this will be closed in 7 days. |
|
Apologies for the delay! I'll look at the PR this week. |
| {{ include "sscdpa.labels" . | indent 2 }} | ||
| rules: | ||
| - apiGroups: [""] | ||
| resources: ["serviceaccounts"] |
There was a problem hiding this comment.
This is another (minor) node isolation violation since this means all nodes will be able to get SAs. Maybe an okay starting point, but looks like a good candidate for scheduled node impersonation.
There was a problem hiding this comment.
Hi @enj, Thanks for the feedback. Do you have any suggestions for working around this?
Looking at other implementations, they also rely on these permissions:
https://github.com/aws/secrets-store-csi-driver-provider-aws/blob/e3b7e823d72e7fc9b43c117b41761c54c7cc67fa/charts/secrets-store-csi-driver-provider-aws/templates/rbac.yaml#L27
There was a problem hiding this comment.
The only one I have in mind would be to put it behind a Chart feature flag. Would it be sufficient to make this an optional feature?
There was a problem hiding this comment.
I don't think we can do anything special here to avoid the permissions - in the future when upstream kube supports this better, we can update the code / RBAC to leverage that functionality. There is no need to make this optional.
There was a problem hiding this comment.
Lets open an issue to track fixing this in the future.
There was a problem hiding this comment.
So for now, we pause the feature as we don't want to introduce the security risk and can't work around it?
|
For everyone interested in this functionality to be more efficient, consider using external-secrets as this or similar PRs seem unlikely to be merged anytime soon. |
|
This PR is stale because it has been open 14 days with no activity. Please comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 21 days with no activity. Feel free to re-open if it is ready for review. |
Reason for Change:
Currently if one wants to use workload identity with this CSI driver one has to add the clientID manually to the parameters even though usually the service account of the pod mounting has this set anyway in its annotations. Other Secrets Store CSI Driver's are also taking care of this to simplify the usage.
Is this a chart or deployment yaml update?
Yes, there is an additional cluster role and cluster role binding required to retrieve the service account annotations.
Requirements
Issue Fixed:
none
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
No
Special Notes for Reviewers: