Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding rad install changes for AWS IRSA support #7741

Merged
merged 13 commits into from
Jul 30, 2024
Merged

Adding rad install changes for AWS IRSA support #7741

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4b65354
adding changes to enable global.awsIRSA.enabled flag
vishwahiremat Jul 17, 2024
7da1a87
update ucp pod spec
vishwahiremat Jul 17, 2024
0781da1
updaing pod spec for application-rp
vishwahiremat Jul 17, 2024
d49e676
addressing comments
vishwahiremat Jul 19, 2024
a1e8018
addressed comments
vishwahiremat Jul 23, 2024
ed398bb
Merge branch 'main' into vishwahiremat/irsa-cli-changes
vishwahiremat Jul 24, 2024
fcd6718
Trigger Build
vishwahiremat Jul 24, 2024
13f3352
addressing comments
vishwahiremat Jul 25, 2024
2bc7e54
reverting the name
vishwahiremat Jul 25, 2024
aff2f10
Merge branch 'main' into vishwahiremat/irsa-cli-changes
vishwahiremat Jul 25, 2024
cfb5e80
Merge branch 'main' into vishwahiremat/irsa-cli-changes
vishwahiremat Jul 29, 2024
9f55960
Merge branch 'main' into vishwahiremat/irsa-cli-changes
vishwahiremat Jul 29, 2024
7dfbbbf
Merge branch 'main' into vishwahiremat/irsa-cli-changes
kachawla Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions deploy/Chart/templates/rp/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameter name should be global.aws.irsa.enabled , based on ryan's comment..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@kachawla kachawla Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nithyatsu yeah that sounds good as long as it is intentional and being tracked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created #7774 for tracking this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nithyatsu nithyatsu Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rynowak In code, we added the token file location as a const:

radius/pkg/recipes/terraform/config/providers/aws.go

Line 56 in 007f7e6

tokenFilePath = "/var/run/secrets/eks.amazonaws.com/serviceaccount/token"

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should do the following:

  • Set the env-var AWS_WEB_IDENTITY_TOKEN_FILE to /var/run/secrets/eks.amazonaws.com/serviceaccount/token in 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the ticket #7774 to implement this comment.

{{- end }}
- name: terraform
mountPath: {{ .Values.rp.terraform.path }}
{{- if .Values.global.rootCA.cert }}
Expand All @@ -83,6 +87,15 @@ spec:
- name: config-volume
configMap:
name: applications-rp-config
{{- if eq .Values.global.aws.irsa.enabled true }}
- name: aws-iam-token
projected:
sources:
- serviceAccountToken:
path: token
expirationSeconds: 86400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nithyatsu @vishwahiremat could you share details on how did we decide on this number?

Copy link
Contributor

@nithyatsu nithyatsu Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

audience: "sts.amazonaws.com"
{{- end }}
- name: terraform
emptyDir: {}
{{- if .Values.global.rootCA.cert }}
Expand Down
13 changes: 13 additions & 0 deletions deploy/Chart/templates/ucp/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,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
{{- end }}
- name: cert
mountPath: '/var/tls/cert'
readOnly: true
Expand All @@ -77,6 +81,15 @@ spec:
# Provide the name of the ConfigMap containing the files you want
# to add to the container
name: ucp-config
{{- if eq .Values.global.aws.irsa.enabled true }}
- name: aws-iam-token
projected:
sources:
- serviceAccountToken:
path: token
expirationSeconds: 86400
audience: "sts.amazonaws.com"
{{- end }}
- name: cert
secret:
secretName: ucp-cert
Expand Down
6 changes: 6 additions & 0 deletions deploy/Chart/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ global:
azureWorkloadIdentity:
enabled: false

# Configure global.aws.irsa.enabled=true to enable AWS IRSA.
# Disabled by default.
Copy link
Contributor

@nithyatsu nithyatsu Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be similar to

aws:
  irsa:
    enabled: false

aws:
irsa:
enabled: false

controller:
image: ghcr.io/radius-project/controller
# Default tag uses Chart AppVersion.
Expand Down
Loading