Adding rad install changes for AWS IRSA support #7741
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7741 +/- ##
=======================================
Coverage 61.04% 61.04%
=======================================
Files 521 521
Lines 27276 27276
=======================================
Hits 16651 16651
Misses 9159 9159
Partials 1466 1466 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
| @@ -69,6 +69,10 @@ spec: | |||
| volumeMounts: | |||
| - name: config-volume | |||
| mountPath: /etc/config | |||
| {{- if eq .Values.global.awsIRSA.enabled true }} | |||
| - name: aws-iam-token | |||
| mountPath: /var/run/secrets/eks.amazonaws.com/serviceaccount | |||
There was a problem hiding this comment.
the parameter name should be global.aws.irsa.enabled , based on ryan's comment..
Signed-off-by: Vishwanath Hiremath <[email protected]>
| @@ -26,6 +26,11 @@ global: | |||
| azureWorkloadIdentity: | |||
| enabled: false | |||
|
|
|||
| # Configure global.aws.irsa.enabled=true to enable AWS IRSA. | |||
| # Disabled by default. | |||
There was a problem hiding this comment.
this should be similar to
aws:
irsa:
enabled: false
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
| @@ -69,6 +69,10 @@ spec: | |||
| volumeMounts: | |||
| - name: config-volume | |||
| mountPath: /etc/config | |||
| {{- if eq .Values.global.aws.irsa.enabled true }} | |||
| - name: aws-iam-token | |||
| mountPath: /var/run/secrets/eks.amazonaws.com/serviceaccount | |||
There was a problem hiding this comment.
@nithyatsu did we say during design discussion that IRSA can be enabled on non EKS clusters as well? Would this mount path still be applicable on other clusters?
There was a problem hiding this comment.
Hi Karishma, IRSA is not specific to eks clusters. But the first iteration of the feature is based on eks.
We would have to make the token path configurable in order to support it on other clusters too. Would it be OK to log a ticket to cover this?
There was a problem hiding this comment.
@nithyatsu yeah that sounds good as long as it is intentional and being tracked
There was a problem hiding this comment.
created #7774 for tracking this.
There was a problem hiding this comment.
Is this actually something we'd have to change for another cluster?
From what I understand, the AWS SDKS will use the value of AWS_WEB_IDENTITY_TOKEN_FILE to load the token. There's nothing special about this file path (or any other file path) as long as the environment variable points to the token on disk.
There was a problem hiding this comment.
@rynowak In code, we added the token file location as a const:
The mount path should work, but it felt odd to reference eks in path for a cluster that was not on eks. Or is it OK since IRSA is a AWS feature? Also, instead of const, should I have looked to set the env variable for the pods?
There was a problem hiding this comment.
IMO we should do the following:
- Set the env-var
AWS_WEB_IDENTITY_TOKEN_FILEto/var/run/secrets/eks.amazonaws.com/serviceaccount/tokenin the helm chart when IRSA is enabled. This is also done by the webhook here. - We can simplify the logic for Terraform because Terraform also uses
AWS_WEB_IDENTITY_TOKEN_FILE. link
Then it doesn't matter what the path is, because it's not hardcoded.
There was a problem hiding this comment.
updated the ticket #7774 to implement this comment.
| sources: | ||
| - serviceAccountToken: | ||
| path: token | ||
| expirationSeconds: 86400 |
There was a problem hiding this comment.
@nithyatsu @vishwahiremat could you share details on how did we decide on this number?
There was a problem hiding this comment.
Hi Karishma, the typical approach for IRSA was to use the pod identity webhook. (
ref. https://github.com/radius-project/design-notes/blob/3b3f66e7b6d340ef7beebcc4212fcd7129bf8b06/cli/2024-06-04-aws-irsa-support.md#alternatives-considered , https://github.com/aws/amazon-eks-pod-identity-webhook#amazon-eks-pod-identity-webhook)
The reason we did not choose the webhook was since it wasnt compatible with multi-tenancy and needed service restarts to register credential. But, we did the exact same thing the webhook does. These are the configurations the webhook sets (https://github.dev/aws/amazon-eks-pod-identity-webhook#amazon-eks-pod-identity-webhook) and mentions it as a default value.
|
let's pls rerun tests. we haven't seen a clean run last. |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
LTGM. Pls get another approval as well. |
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
kachawla
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
kachawla
changed the title
# Description Added changes in deployment yaml to enable `global.awsIRSA.enabled` to true it is set using `rad install kubernetes` command ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: #issue_number --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Co-authored-by: Karishma Chawla <[email protected]> Signed-off-by: Reshma Abdul Rahim <[email protected]>
Description
Added changes in deployment yaml to enable
global.awsIRSA.enabledto true it is set usingrad install kubernetescommandType of change
Fixes: #issue_number