fix: cleanup IRSA for EKS #39
Conversation
gazal-k
commented
Mar 28, 2020
•
gazal-k
commented
- correct SA names
- remove unnecessary permissions
- add IRSA for cluster-autoscaler
ghost
added
the
| - iam:GetPolicy | ||
| - iam:CreatePolicy | ||
| - iam:DeleteRole | ||
| - iam:GetOpenIDConnectProvider |
There was a problem hiding this comment.
would tekton have ever needed these permissions ?
There was a problem hiding this comment.
I can't be sure of this. But in EKS with regular jx boot, I imagine the SA was getting overwritten anyway and didn't get any of these permissions but just the node permissions. We have been creating EKS clusters with the node having almost full ECR permissions which is why tekton didn't have any trouble.
In any case, at least the jxl boot run job I think doesn't really need to deal with IAM or cloudformation stacks or EKS roles. I would imagine it needs ECR permissions and S3 (logs, test reports etc)
There was a problem hiding this comment.
we've verified and the only role tekton-bot needs is elevated ECR access (https://github.com/jenkins-x-labs/jenkins-x-versions/pull/39/files#diff-75c8c8fbc6a827a3d9818b81a608fc2dL17) which cannot be leveraged yet because of: jenkins-x-labs/issues#21.
| Action: | ||
| - route53:ListHostedZones | ||
| - route53:ListResourceRecordSets | ||
| Resource: "*" | ||
| CFNCertManagerPolicies: | ||
| Type: AWS::IAM::ManagedPolicy | ||
| Properties: |
There was a problem hiding this comment.
does certmanager really need Route53 permissions?
There was a problem hiding this comment.
it could be externaldns needing that? cc @rawlingsj
There was a problem hiding this comment.
this: https://github.com/jenkins-x-labs/jenkins-x-versions/pull/39/files#diff-75c8c8fbc6a827a3d9818b81a608fc2dR17-R32 should take care of permissions for externaldns, which is the same as this used to do: https://github.com/jenkins-x-labs/jenkins-x-versions/pull/39/files#diff-475eb62541c9c9f5f30b5776da4c20a3L24-L39
There was a problem hiding this comment.
looks like cert-manager does need these route53 permissions, but cert-manager-cainjector might not
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
we don't use this cloudformation & |
| namespace: jx | ||
| labels: {aws-usage: "jenkins-x"} | ||
| attachPolicyARNs: | ||
| - "arn:aws:iam::aws:policy/AmazonS3FullAccess" | ||
| - metadata: | ||
| name: jxui | ||
| name: jxl-boot | ||
| namespace: jx | ||
| labels: {aws-usage: "jenkins-x"} | ||
| attachPolicyARNs: | ||
| - "arn:aws:iam::aws:policy/AmazonS3FullAccess" |
There was a problem hiding this comment.
AmazonS3ReadOnlyAccess might be enough. Will verify and change later
There was a problem hiding this comment.
during jx step verify preinstall it verifies the presence of the storage buckets
| - metadata: | ||
| name: jenkins-x-controllerbuild | ||
| name: jxui | ||
| namespace: jx | ||
| labels: {aws-usage: "jenkins-x"} | ||
| attachPolicyARNs: | ||
| - "arn:aws:iam::aws:policy/AmazonS3FullAccess" |
There was a problem hiding this comment.
AmazonS3ReadOnlyAccess might be enough. Will verify and change later
| - metadata: | ||
| name: cert-manager-cainjector |
There was a problem hiding this comment.
cert-manager-cainjector does not need route53 or any IAM access that cert-manager needs
|
we can probably tweak these further later. |
- correct SA names - remove unnecessary permissions - add IRSA for cluster-autoscaler
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
