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.

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

docs: precise AWS IAM policy example #6448

Merged

Conversation

blanchardma
Copy link
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

On the same page where I made my changes for this PR is the following instruction written:
"In this example, only the second block of actions should be updated to restrict the resources/add conditionals:"

If you then add a common condition like its often mentioned here in the issues or also recommended in the EKS Best Practices Guides which looks like:

            "Condition": {
                "StringEquals": {
                    "aws:ResourceTag/k8s.io/cluster-autoscaler/enabled": "true",
                    "aws:ResourceTag/k8s.io/cluster-autoscaler/<my-cluster>": "owned"
                }
            }

It's important to note that applying such conditions togehter with the example IAM policy often lead to an issue where the Cluster Autoscaler does not have the necessary access on the EKS API. If for example all is deployed with terraform-aws-eks, the EKS does not have out of the box these tags but the Auto Scaling Group already have it. This causes the following exception of the Cluster Autoscaler:

E0110 12:34:56.789123 1 managed_nodegroup_cache.go:133] Failed to query the managed nodegroup example-node-group-name for the cluster exmaple-cluster-name while looking for labels/taints: AccessDeniedException: User: arn:aws:sts::account-id:assumed-role/example-role/resource-id is not authorized to perform: eks:DescribeNodegroup on resource: arn:aws:eks:region:account-id:nodegroup/exmaple-cluster-name/example-node-group-name/resource-id

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jan 12, 2024
Copy link

linux-foundation-easycla bot commented Jan 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 12, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @blanchardma!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 12, 2024
@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider area/cluster-autoscaler labels Jan 12, 2024
@Shubham82
Copy link
Contributor

Hi @blanchardma
you have to sign the CLA before the PR can be reviewed.
See the following document to sign the CLA: Signing Contributor License Agreements(CLA)

@Shubham82
Copy link
Contributor

To check EasyCLA

/easycla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 17, 2024
@gjtempleton
Copy link
Member

Hey @blanchardma, thanks for the PR!

Just to confirm my understanding, the issue is when you use a common set of tag based restriction as you've linked to, there's a few of the actions you end up unable to perform when deploying with the terraform-aws-eks module?

Can you share an (appropriately obfuscated) config example for me to understand a bit more?

@blanchardma
Copy link
Contributor Author

blanchardma commented Feb 9, 2024

Yes, if you follow the instructions from the docs (full-cluster-autoscaler-features-policy-recommended) and add as recommended conditions it will end up in an IAM policy like this:

{
    "Statement": [
        {
            "Action": [
                "ec2:DescribeLaunchTemplateVersions",
                "ec2:DescribeInstanceTypes",
                "autoscaling:DescribeTags",
                "autoscaling:DescribeScalingActivities",
                "autoscaling:DescribeLaunchConfigurations",
                "autoscaling:DescribeAutoScalingInstances",
                "autoscaling:DescribeAutoScalingGroups"
            ],
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        },
        {
            "Action": [
                "eks:DescribeNodegroup",
                "ec2:GetInstanceTypesFromInstanceRequirements",
                "ec2:DescribeImages",
                "autoscaling:TerminateInstanceInAutoScalingGroup",
                "autoscaling:SetDesiredCapacity"
            ],
            "Condition": {
                "StringEquals": {
                    "autoscaling:ResourceTag/k8s.io/cluster-autoscaler/enabled": "true",
                    "autoscaling:ResourceTag/kubernetes.io/cluster/eks-cluster-name": "owned"
                }
            },
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        }
    ],
    "Version": "2012-10-17"
}

If you use this IAM policy above, for example together with eksctl or the already mentioned terraform-aws-eks module, then it will end in an access denied for the Cluster Autoscaler on EKS API. This is because EKS does not meet the condition because the tags are missing.

The IAM policy should look like as proposed in the PR:

{
    "Statement": [
        {
            "Action": [
                "eks:DescribeNodegroup",
                "ec2:GetInstanceTypesFromInstanceRequirements",
                "ec2:DescribeLaunchTemplateVersions",
                "ec2:DescribeInstanceTypes",
                "ec2:DescribeImages",
                "autoscaling:DescribeTags",
                "autoscaling:DescribeScalingActivities",
                "autoscaling:DescribeLaunchConfigurations",
                "autoscaling:DescribeAutoScalingInstances",
                "autoscaling:DescribeAutoScalingGroups"
            ],
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        },
        {
            "Action": [
                "autoscaling:TerminateInstanceInAutoScalingGroup",
                "autoscaling:SetDesiredCapacity"
            ],
            "Condition": {
                "StringEquals": {
                    "autoscaling:ResourceTag/k8s.io/cluster-autoscaler/enabled": "true",
                    "autoscaling:ResourceTag/kubernetes.io/cluster/eks-cluster-name": "owned"
                }
            },
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        }
    ],
    "Version": "2012-10-17"
}

It separates the policy into read actions (Describe/Get) and write actions (Set/Terminate) and binds the more critical ones to the condition.

@Chili-Man
Copy link

I am ran into the access denied issue for eks:DescribeNodeGroup permission due to the documentation being incorrect about the IAM policy. It works after the policy as proposed by @blanchardma

@drmorr0
Copy link
Contributor

drmorr0 commented Apr 5, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2024
@drmorr0
Copy link
Contributor

drmorr0 commented Apr 5, 2024

/approved

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blanchardma, drmorr0

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blanchardma, drmorr0

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit bbe242e into kubernetes:master Apr 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants