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

fix: Restrict IAM permissions to those related to Karpenter managed resources #1332

Merged
merged 27 commits into from
Apr 7, 2022
Merged

fix: Restrict IAM permissions to those related to Karpenter managed resources #1332

merged 27 commits into from
Apr 7, 2022

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Feb 13, 2022

1. Issue, if available:
Relates to #507
Closes terraform-aws-modules/terraform-aws-iam#208

2. Description of changes:

  • Restrict ssm:GetParameter IAM permissions to only the AWS service parameters where AMI IDs are stored/retrieved. This prevents the controller from having access to any parameters in the accounts its deployed within since SSM parameters do not support resource based policies. Its common for secrets or sensitive values to be stored in SSM so this ensures the controller role doesn't have access to those in the accounts its provisioned within. The policy is scoped based on the SSM GetParameter API calls identified here https://github.com/aws/karpenter/blob/ed9473a9863ca949b61b9846c8b9f33f35b86dbd/pkg/cloudprovider/aws/ami.go#L105-L123
  • Restrict ec2:RunInstances, ec2:TerminateInstances, and ec2:DeleteLaunchTemplate API calls to only those resources that meet the condition where the tag ec2:ResourceTag/karpenter.sh/discovery = the cluster name. This limits the roles permissions to only run/terminate instances/launch templates that are tagged for Karpenter to control
  • Restrict iam:PassRole to only allow Karpenter to pass the associated node role, not any role
  • Update IAM permissions to use AWS partition so that policies can be used in other regions without modification (Gov cloud, china, etc.)
  • Update Terraform example based on current standards from Terraform AWS modules
  • Fixed bug in the example helm chart of the Terraform example - clusterName and clusterEndpoint need their values nested under controller
  • Remove unnecessary variable from Terraform example - a local variable works well in this case and makes the commands for users simpler/easier

3. How was this change tested?

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Feb 13, 2022

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 8576324
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/624eeac338b9ce0008e33561
😎 Deploy Preview https://deploy-preview-1332--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@suket22 suket22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making this change, this is awesome!

Before we merge this in, could you test this change using our development guide?

@bryantbiggs
Copy link
Member Author

Thank you for making this change, this is awesome!

Before we merge this in, could you test this change using our development guide?

I'm trying to follow the guide but nearly all commands are failing similarly

controller-gen \
	object:headerFile="hack/boilerplate.go.txt" \
	crd \
	paths="./pkg/..." \
	output:crd:artifacts:config=charts/karpenter/crds
/bin/sh: 1: controller-gen: not found
make: *** [Makefile:64: codegen] Error 127

make toolchain works fine though

@ellistarn
Copy link
Contributor

Can you add #507 to the issue decription?

@bryantbiggs bryantbiggs changed the title fix: Restrict ssm:GetParameter IAM permissions to only the AWS service parameters fix: Restrict IAM permissions to those related to Karpenter managed resources Feb 14, 2022
@bwagner5
Copy link
Contributor

Thank you for making this change, this is awesome!
Before we merge this in, could you test this change using our development guide?

I'm trying to follow the guide but nearly all commands are failing similarly

controller-gen \
	object:headerFile="hack/boilerplate.go.txt" \
	crd \
	paths="./pkg/..." \
	output:crd:artifacts:config=charts/karpenter/crds
/bin/sh: 1: controller-gen: not found
make: *** [Makefile:64: codegen] Error 127

make toolchain works fine though

controller-gen should be installed here w/ a go install in make toolchain https://github.com/aws/karpenter/blob/main/hack/toolchain.sh#L22

We check on the next line if the go bin dir is in your path, maybe that check isn't working as intended?

Can you check if the controller-gen binary is in your go bin dir?
Is go bin in your PATH?

@bryantbiggs
Copy link
Member Author

@bwagner5 / @ellistarn we just released a new sub-module which makes creating IRSA roles easier for common addons/controllers. That sub-module has the policy that I was updating here baked in - you can see it here. So now its just simply:

module "karpenter_irsa" {
  source  = "terraform-aws-modules/iam/aws//modules/iam-role-for-service-accounts-eks"

  role_name                          = "karpenter_controller"
  attach_karpenter_controller_policy = true

  karpenter_controller_cluster_ids        = [module.eks.cluster_id]
  karpenter_controller_node_iam_role_arns = [
    module.eks.eks_managed_node_groups["default"].iam_role_arn
  ]

  oidc_providers = {
    main = {
      provider_arn               = module.eks.oidc_provider_arn
      namespace_service_accounts = ["karpenter:karpenter"]
    }
  }
}

I have also updated our EKS docs to show this use case as well https://github.com/terraform-aws-modules/terraform-aws-eks#irsa-integration

@ellistarn
Copy link
Contributor

ellistarn commented Feb 21, 2022

Thank you for making this change, this is awesome!
Before we merge this in, could you test this change using our development guide?

I'm trying to follow the guide but nearly all commands are failing similarly

controller-gen \
	object:headerFile="hack/boilerplate.go.txt" \
	crd \
	paths="./pkg/..." \
	output:crd:artifacts:config=charts/karpenter/crds
/bin/sh: 1: controller-gen: not found
make: *** [Makefile:64: codegen] Error 127

make toolchain works fine though

You need to add your $GOBIN to your $PATH for those binaries to be discovered.

@dewjam
Copy link
Contributor

dewjam commented Mar 15, 2022

Hey @bryantbiggs . It looks like we are looking for these changes to be tested. Were you able to build and test these changes locally? (Also looks like we have some merge conflicts. Might be worth pulling down most recent commits and refreshing this PR).

dewjam
dewjam previously approved these changes Mar 29, 2022
@bryantbiggs
Copy link
Member Author

Updated to use the kubectl provider to deploy the Karpenter provisioner via Terraform. This removes the need for setting the cluster name environment variable. I have a working reference architecture for this here https://github.com/clowdhaus/eks-reference-architecture/tree/main/karpenter/us-east-1 which follows the community norm of simple terraform init && terraform apply commands for examples

@tzneal
Copy link
Contributor

tzneal commented Mar 30, 2022

Updated to use the kubectl provider to deploy the Karpenter provisioner via Terraform. This removes the need for setting the cluster name environment variable. I have a working reference architecture for this here https://github.com/clowdhaus/eks-reference-architecture/tree/main/karpenter/us-east-1 which follows the community norm of simple terraform init && terraform apply commands for examples

Does that work when creating a new provisioner? In #1551 I had to add defaulted values to the provisioner spec:

        "apiVersion" = "extensions.karpenter.sh/v1alpha1"
        "kind"       = "AWS"

Without this, creating a brand new provisioner using kubernetes_manifest failed.

@bryantbiggs
Copy link
Member Author

Updated to use the kubectl provider to deploy the Karpenter provisioner via Terraform. This removes the need for setting the cluster name environment variable. I have a working reference architecture for this here https://github.com/clowdhaus/eks-reference-architecture/tree/main/karpenter/us-east-1 which follows the community norm of simple terraform init && terraform apply commands for examples

Does that work when creating a new provisioner? In #1551 I had to add defaulted values to the provisioner spec:

        "apiVersion" = "extensions.karpenter.sh/v1alpha1"
        "kind"       = "AWS"

Without this, creating a brand new provisioner using kubernetes_manifest failed.

yes this works - here is a complete working example that mirrors the changes in this PR https://github.com/clowdhaus/eks-reference-architecture/tree/main/karpenter/us-east-1

@dewjam
Copy link
Contributor

dewjam commented Mar 31, 2022

yes this works - here is a complete working example that mirrors the changes in this PR https://github.com/clowdhaus/eks-reference-architecture/tree/main/karpenter/us-east-1

This doesn't appear to be working for me, at least not on Terraform 1.1.7. From what I can tell the kubectl terraform provider needs to be sourced from a specific author.

Could not retrieve the list of available versions for provider hashicorp/kubectl: provider registry registry.terraform.io does not have a provider named registry.terraform.io/hashicorp/kubectl

@bryantbiggs
Copy link
Member Author

This doesn't appear to be working for me, at least not on Terraform 1.1.7. From what I can tell the kubectl terraform provider needs to be sourced from a specific author.

Could not retrieve the list of available versions for provider hashicorp/kubectl: provider registry registry.terraform.io does not have a provider named registry.terraform.io/hashicorp/kubectl

Added in the required providers in order to properly resolve 3rd party providers

@dewjam
Copy link
Contributor

dewjam commented Apr 5, 2022

This look good to me @bryantbiggs . I've tested the procedure and it works as expected. I've asked @chrisnegus to take a look as well, but we should be close to merging.

Copy link
Member

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a small editing suggestion (not important), but otherwise it looks good from an editing standpoint. Because I have not tested it, someone else should okay the merge.

@dewjam
Copy link
Contributor

dewjam commented Apr 6, 2022

Hey @bryantbiggs . Found one more issue here. It doesn't appear that the kube-config is updated automatically when following the instructions in this PR. Is there a way to have Terraform update the kube-config file automatically?

@bryantbiggs
Copy link
Member Author

Hey @bryantbiggs . Found one more issue here. It doesn't appear that the kube-config is updated automatically when following the instructions in this PR. Is there a way to have Terraform update the kube-config file automatically?

Updated documentation to ensure users update their local kubeconfig before interacting with the cluster using kubectl in 8576324

@dewjam
Copy link
Contributor

dewjam commented Apr 7, 2022

I tested this locally and found it to work as expected. I'm going to go ahead and merge this change.

1 similar comment
@dewjam
Copy link
Contributor

dewjam commented Apr 7, 2022

I tested this locally and found it to work as expected. I'm going to go ahead and merge this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iam-role-for-service-accounts-eks] Karpenter IAM Policy expects a tag
8 participants