-
Notifications
You must be signed in to change notification settings - Fork 46
Draft: AWS standalone cross account doc #401
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?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RomanBednar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
README-tmp.md
Outdated
## 5.9.5.1 Requirements | ||
|
||
- The EFS CSI Operator must be installed prior to beginning this procedure. Follow the _Installing the AWS EFS CSI Driver Operator_ procedure. | ||
- Both the Red Hat OpenShift Service on AWS cluster and EFS file system must be located in the same AWS region. |
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.
- Both the Red Hat OpenShift Service on AWS cluster and EFS file system must be located in the same AWS region. | |
- Both the Openshift cluster and EFS file system must be located in the same AWS region. |
Red Hat OpenShift Service on AWS cluster stands for ROSA, if it will be used for doc team reference better to correct it.
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.
Missed it - fixed.
|
||
- The EFS CSI Operator must be installed prior to beginning this procedure. Follow the _Installing the AWS EFS CSI Driver Operator_ procedure. | ||
- Both the Red Hat OpenShift Service on AWS cluster and EFS file system must be located in the same AWS region. | ||
- Ensure that the two VPCs referenced in this guide utilize different network CIDR ranges. |
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.
There's also another scenario 2 accounts use the same shared-vpc, not sure it is worth to do some tests about this.
cc to @ropatil010
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.
I don't think so, similar to the "same availability zone" feature mentioned in another comment here - we don't document it now and probably should not add new features in scope of this change.
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.
Acked and thanks for the double confirm!
README-tmp.md
Outdated
export AWS_ACCOUNT_B="<ACCOUNT_B_NAME>" | ||
``` | ||
|
||
5. Ensure that your AWS CLI is configured with JSON output format as the default for both accounts: |
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.
5. Ensure that your AWS CLI is configured with JSON output format as the default for both accounts: | |
5. Ensure that your AWS CLI is configured with JSON output format as the default for both accounts(the output format could be override by `export AWS_DEFAULT_OUTPUT="json"`): |
Maybe better.
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.
Yes this is one of the methods of changing the output format, however I intentionally avoided explaining it here since we would be documenting AWS here, which is not desired. For this purposes I've added a link in the paragraph below to official AWS doc (which also explains AWS_DEFAULT_OUTPUT var): https://github.com/openshift/csi-operator/pull/401/files#diff-5c4f1f33bb4c41d6c3e57b67e95dc097ebf581e3fdc2f5bbf848f15135179035R61
README-tmp.md
Outdated
done | ||
``` | ||
|
||
5. Enable DNS resolution for Account A to access Account B's VPC: |
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.
I do not add the step(optional step?) in CI, from the upstream doc it seems also needs if we want to ensure that your EFS Mount Target is in the same availability zone with node, it also needs extra config in secret right?
If you would like to ensure that your EFS Mount Target is in the same availability zone as your EKS Node, then ensure you have completed the prerequisites for cross-account DNS resolution and include the crossaccount key with value true. For example, kubectl create secret generic x-account --namespace=kube-system --from-literal=awsRoleArn='arn:aws:iam::123456789012:role/EFSCrossAccountAccessRole' --from-literal=crossaccount='true' instead.
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.
Good point, it seems like this would be a prerequisite to using crossaccount='true'
which is indeed reflected in a secret -> but this is something we do not document currently. So I would remove this step for now unless we get an RFE just so we don't introduce this change and bloat the testing even more.
README-tmp.md
Outdated
|
||
```bash | ||
for SUBNET in $(aws ec2 describe-subnets \ | ||
--query 'Subnets[*].{SubnetId:SubnetId}' \ |
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.
--query 'Subnets[*].{SubnetId:SubnetId}' \ | |
--filters "Name=vpc-id,Values=${AWS_ACCOUNT_B_VPC_ID}" \ | |
--query 'Subnets[*].{SubnetId:SubnetId}' \ |
We'd better filter with vpc id since current way will query all subnets in the region.
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.
Nice catch, corrected.
export STORAGE_CLASS_NAME=efs-sc-cross | ||
``` | ||
|
||
2. Create a secret that references the role ARN in Account B: |
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.
Maybe we'd better also mention that openshift-cluster-csi-drivers
is the operator installed namespace, sometimes customer install it in some other namespace.
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.
Yes we should mention it - changing this to variable.
@Phaow Thank you for the comments and review, I've addressed them now - please take a look and let me know if there's anything else. When done I suggest we test this manually to make sure there are no mistakes before handoff to docs team. |
LGTM, @ropatil010 as the feature main QE owner, could you please cross check and verify the doc works as expected? Thanks! |
fa7af77
to
276d1f8
Compare
276d1f8
to
95e640e
Compare
Hi @RomanBednar, After testing the steps as mentioned in PR, observed 2 issues.
Need to update to:
|
@ropatil010 So you create the secret in default while install the csi driver operator in |
Agree @Phaow , Thanks for input. |
@ropatil010 For the issues:
Thanks for the feedback and let us know when you're done with the testing so we can proceed. |
EOF | ||
``` | ||
|
||
3. In AWS Account A, attach the AWS-managed policy "AmazonElasticFileSystemClientFullAccess" to the OpenShift Container Platform cluster master 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.
Currently tested on OCP standalone and OCP HCP kind clusters.
Note: If running on HCP kind cluster no need of executing this 3rd steps. Since HCP cluster has only worker nodes.
I have tested with all mentioned steps on HCP kind cluster except this 3rd step and we can provision volume, able to read/write/execute.
We can mark this doc on both OCP standalone and OCP HCP kind clusters.
cc: @jsafrane @Phaow
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.
So do you test without the step volumeReclaimPolicy work as expected? I mean in hcp clusters delete the pvc then the pv should be deleted to be consistent with the volumeReclaimPolicy:Delete
. The step is used for driver controller reclaim volumes.
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.
I guess on HCP Kind clusters we have to add permission to worker role nodes instead of master nodes. I tested using this and it worked fine.
EFS_POLICY_ARN=arn:aws:iam::aws:policy/AmazonElasticFileSystemClientFullAccess for NODE in $(oc get nodes --selector=node-role.kubernetes.io/worker | tail -n +2 | awk '{print $1}') do INSTANCE_PROFILE=$(aws ec2 describe-instances --filters "Name=private-dns-name,Values=${NODE}" --query 'Reservations[].Instances[].IamInstanceProfile.Arn' --output text | awk -F'/' '{print $NF}') WORKER_ROLE_ARN=$(aws iam get-instance-profile --instance-profile-name ${INSTANCE_PROFILE} --query 'InstanceProfile.Roles[0].Arn' --output text) WORKER_ROLE_NAME=$(echo ${WORKER_ROLE_ARN} | awk -F'/' '{print $NF}') echo "Assigning policy ${EFS_POLICY_ARN} to role ${WORKER_ROLE_NAME} for node ${NODE}" aws iam attach-role-policy --role-name ${WORKER_ROLE_NAME} --policy-arn ${EFS_POLICY_ARN} done
`
oc get pv
NAME CAPACITY ACCESS MODES RECLAIM POLICY STATUS CLAIM STORAGECLASS VOLUMEATTRIBUTESCLASS REASON AGE
pvc-fd9dabc9-a47f-4431-a435-267040c753a6 1Gi RWX Delete Bound testropatil/mypvc-fs efs-sc-cross 10s
oc delete deployment --all -n testropatil
deployment.apps "mydep-fs" deleted
oc delete pvc --all -n testropatil
persistentvolumeclaim "mypvc-fs" deleted
oc get pv
No resources found
`
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.
Yes, it is up to which nodes the driver controller pod located(hcp clusters the driver controller located at workers), thanks for the doulbe checking!
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.
Great catch! I've updated the doc to explain which node selector to use in this step: deea616
@RomanBednar: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
provisioningMode: efs-ap | ||
fileSystemId: ${CROSS_ACCOUNT_FS_ID} | ||
directoryPerms: "700" | ||
gidRangeStart: "1000" |
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.
Hi @RomanBednar,
Can we remove these 2 fields gidRangeStart, gidRangeEnd. Not sure if customer may hit issue while creating new volume "2000+" since it may reach gitRangeEnd: "2000"?
Others LGTM. Thank you.!
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.
I think you're right, gid range parameters serve no purpose in this configuration. However I've checked the current standalone documentation and the storage class has the same parameters in that example. So I'd rather keep it unchanged here for the sake of consistency.
Added test results here: https://issues.redhat.com/browse/STOR-2378 |
/hold We won't be merging this PR - after official docs are finished we'll close this. |
Notes
Documentation
-
5.9.5. AWS EFS CSI cross account support
-
5.9.6. Creating and configuring access to EFS volumes in AWS
- These two sections should be consolidated into a single comprehensive section, as they are interdependent and cannot function independently
5.9.8. Creating static PVs with Amazon Elastic File Storage
should mention that when using Cross Account feature the volumeHandle has to reference EFS ID of the filesystem in Account BManual testing procedures:
CI configuration considerations:
Testing
Install EFS operator from OperatorHub as usual, for STS use ccoctl to create AWS Role
Create testing namespace: