-
Notifications
You must be signed in to change notification settings - Fork 22
provide better examples for setting up IAM roles #317
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.
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
base: main
Are you sure you want to change the base?
provide better examples for setting up IAM roles #317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR significantly enhances the documentation for setting up IAM roles for Percona Backup for MongoDB in AWS environments. It expands the content from basic instructions to comprehensive, step-by-step guides with concrete examples.
Key Changes:
- Restructured EC2 instance IAM section to focus on role assumption with detailed policy examples
- Expanded IRSA (IAM Roles for Service Accounts) section with complete setup instructions including OIDC provider configuration, policy creation, and service account annotation
- Added extensive JSON policy examples for trust relationships and S3 permissions
- Included practical bash commands for AWS CLI operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/manage/automate-s3-access.md
Outdated
| { | ||
| "Effect": "Allow", | ||
| "Principal": { | ||
| "AWS": "arn:aws:iam::EC2_ACCOUNT_ID:role/pbm-target-role" |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role name in the trust policy example is incorrect. The trust policy is for the target role that will be assumed, but line 40 shows pbm-target-role which is the target role itself. The trust policy should reference the EC2 instance role that is allowed to assume this target role, not the target role itself.
The Principal.AWS field should specify the EC2 instance role ARN (e.g., arn:aws:iam::EC2_ACCOUNT_ID:role/pbm-ec2-instance-role), not pbm-target-role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
docs/manage/automate-s3-access.md
Outdated
| ] | ||
| } | ||
| ``` | ||
| > Remember to replace `EC2_ACCOUNT_ID` and `pbm-target-role` with the account ID and role name of your EC2 instance. |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guidance text at line 47 is misleading. It states to "replace EC2_ACCOUNT_ID and pbm-target-role", but pbm-target-role in line 40 should actually be replaced with the EC2 instance role name, not the target role name. The note should clarify that users need to replace these placeholders with their EC2 instance's role information, not the target role being created.
| > Remember to replace `EC2_ACCOUNT_ID` and `pbm-target-role` with the account ID and role name of your EC2 instance. | |
| > Remember to replace `EC2_ACCOUNT_ID` and `pbm-target-role` with the account ID and role name of your EC2 instance's IAM role (the role attached to your EC2 instance that will assume the target role). Do **not** use the target role's name here. |
docs/manage/automate-s3-access.md
Outdated
|
|
||
| * **Trust Policy**: The trust policy of the target role must allow the EC2 instance's role to assume it. | ||
|
|
||
| For example, if your EC2 instance role is `arn:aws:iam::EC2_ACCOUNT_ID:role/pbm-target-role`, use the following trust policy for your target role: |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introductory text for the trust policy is confusing. Line 31 states "if your EC2 instance role is arn:aws:iam::EC2_ACCOUNT_ID:role/pbm-target-role", but this should refer to the EC2 instance role (e.g., pbm-ec2-instance-role), not pbm-target-role. Using the same role name for both the EC2 instance role and the target role creates confusion about which role is which.
| First, create a trust policy JSON file (e.g., `pbm-trust-policy.json`). This policy allows your Kubernetes service account to assume the role. Replace `<account-id>`, `<region>`, and `<oidc-id>` with your AWS account ID, EKS cluster region, and the OIDC ID from step 1. | ||
| ```json | ||
| { | ||
| "Version": "2012-10-17", | ||
| "Statement": [ | ||
| { | ||
| "Effect": "Allow", | ||
| "Principal": { | ||
| "Federated": "arn:aws:iam::<account-id>:oidc-provider/oidc.eks.<region>.amazonaws.com/id/<oidc-id>" |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing context for <oidc-id> placeholder. While the text at line 163 mentions "the OIDC ID from step 1", step 1 shows how to get the OIDC issuer URL but doesn't explicitly explain that the OIDC ID is the unique identifier at the end of that URL. Consider adding a note explaining how to extract the <oidc-id> from the issuer URL returned in step 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
|
@radoslawszulgo I've opened a new pull request, #318, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: radoslawszulgo <175048287+radoslawszulgo@users.noreply.github.com>
Co-authored-by: radoslawszulgo <175048287+radoslawszulgo@users.noreply.github.com>
Co-authored-by: radoslawszulgo <175048287+radoslawszulgo@users.noreply.github.com>
Add OIDC ID extraction guidance for IRSA setup
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@radoslawszulgo I've opened a new pull request, #319, to work on those changes. Once the pull request is ready, I'll request review from you. |
…rget role Co-authored-by: radoslawszulgo <175048287+radoslawszulgo@users.noreply.github.com>
Fix IAM trust policy example to reference EC2 instance role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ```bash | ||
| export AWS_ROLE_ARN=arn:aws:iam::ACCOUNT_ID:role/pbm-target-role | ||
| export AWS_SESSION_NAME=pbm-session-$(hostname) |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AWS environment variable name AWS_SESSION_NAME should be AWS_ROLE_SESSION_NAME according to AWS SDK documentation. The correct environment variable for specifying the session name when assuming a role is AWS_ROLE_SESSION_NAME, not AWS_SESSION_NAME.
| export AWS_SESSION_NAME=pbm-session-$(hostname) | |
| export AWS_ROLE_SESSION_NAME=pbm-session-$(hostname) |
| "s3:DeleteObject", | ||
| "s3:GetBucketLocation" | ||
| ], | ||
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | ||
| }, | ||
| { | ||
| "Effect": "Allow", | ||
| "Action": "s3:ListBucket", |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permissions in the S3 policy may be insufficient. The policy includes s3:GetBucketLocation but is missing s3:ListBucket permissions on the same resource level. Line 63 has s3:GetBucketLocation in the array with object-level permissions (arn:aws:s3:::your-pbm-bucket/*), but GetBucketLocation is a bucket-level operation and should be applied to arn:aws:s3:::your-pbm-bucket (without the /*). Consider moving s3:GetBucketLocation to be with s3:ListBucket on lines 69-70, or add it as a separate statement.
| "s3:DeleteObject", | |
| "s3:GetBucketLocation" | |
| ], | |
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | |
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": "s3:ListBucket", | |
| "s3:DeleteObject" | |
| ], | |
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | |
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": [ | |
| "s3:ListBucket", | |
| "s3:GetBucketLocation" | |
| ], |
| "s3:DeleteObject", | ||
| "s3:GetBucketLocation" | ||
| ], | ||
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | ||
| }, | ||
| { | ||
| "Effect": "Allow", | ||
| "Action": "s3:ListBucket", |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permissions in the S3 policy may be insufficient. The policy includes s3:GetBucketLocation in the object-level permissions (line 141: arn:aws:s3:::your-pbm-bucket/*), but GetBucketLocation is a bucket-level operation and should be applied to arn:aws:s3:::your-pbm-bucket (without the /*). Consider moving s3:GetBucketLocation to be with s3:ListBucket on lines 147-148, or add it as a separate statement for the bucket resource.
| "s3:DeleteObject", | |
| "s3:GetBucketLocation" | |
| ], | |
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | |
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": "s3:ListBucket", | |
| "s3:DeleteObject" | |
| ], | |
| "Resource": "arn:aws:s3:::your-pbm-bucket/*" | |
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": [ | |
| "s3:ListBucket", | |
| "s3:GetBucketLocation" | |
| ], |
| Then, annotate the service account. Replace `<namespace>`, `<service-account-name>`, and `<role_arn>` with the correct values. | ||
| ```bash | ||
| kubectl annotate serviceaccount <service-account-name> -n <namespace> \ | ||
| eks.amazonaws.com/role-arn="<role_arn>" |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent placeholder format. Throughout the document, placeholders use different formats: <cluster_name> (line 119), <region> and <cluster-name> (line 123), <account-id>, <region>, and <oidc-id> (line 163), <namespace> and <service-account-name> (lines 180, 187), and <role_arn> (line 213). Consider using a consistent format for all placeholders, either all with hyphens (e.g., <cluster-name>) or all with underscores (e.g., <cluster_name>).
| Then, annotate the service account. Replace `<namespace>`, `<service-account-name>`, and `<role_arn>` with the correct values. | |
| ```bash | |
| kubectl annotate serviceaccount <service-account-name> -n <namespace> \ | |
| eks.amazonaws.com/role-arn="<role_arn>" | |
| Then, annotate the service account. Replace `<namespace>`, `<service-account-name>`, and `<role-arn>` with the correct values. | |
| ```bash | |
| kubectl annotate serviceaccount <service-account-name> -n <namespace> \ | |
| eks.amazonaws.com/role-arn="<role-arn>" |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.