-
Notifications
You must be signed in to change notification settings - Fork 807
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
Update example policy, use it in tests, and document it #940
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wongma7 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 |
834e030
to
48fdb49
Compare
…olumes with in-tree tags
…/* volumes so that CSI can Delete KCM-created volumes
/cc @AndyXiangLi |
/test pull-aws-ebs-csi-driver-e2e-multi-az |
docs/README.md
Outdated
"aws:RequestTag/kubernetes.io/cluster/<ID of the Kubernetes cluster>": "owned" | ||
} | ||
} | ||
}, |
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.
Nit: do we need this partial example? Seems unnecessary, why not just leave it and let them click the link? Or maybe it should just say: "The driver needs to be able to able to manipulate volumes that were created by the in-tree plugin prior to migration turned on, so it needs to have permissions to volumes with either the deprecated KubernetesCluster=<clusterName>
tag or the newer format: kubernetes.io/cluster/<clusterName>: <owned>
"
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 that's too much detail, I wanted to highlight the specific part of the policy i'm talking about but let me try adding a link to the line instead of including the partial example
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.
/lgtm |
/skip |
/retest |
Is this a bug fix or adding new feature?
What is this PR about? / Why do we need it? The example policy is a minimal restrictive example so it's not guaranteed to work in all scenarios... but there are is at least one* notable scenario where it breaks, so we should update* the example and documentation to account for this scenario:
"aws:RequestTag/kubernetes.io/cluster/<ID of the Kubernetes cluster>": "owned"
. also add permission to create although it's not totally necessary, maybe redundant, but to be consistent.What testing is done?