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

feat: Add auto discovery permission of endpoint to karpenter role #1417

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Feb 7, 2023

What does this PR do?

🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.

Motivation

Adds the necessary policy for aws/karpenter-provider-aws#3363

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs require a new example and/or doc page. In general:

  • Use an existing example when possible to demonstrate a new addons usage
  • A new docs page under docs/add-ons/* is required for new a new addon

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test February 11, 2023 00:59 — with GitHub Actions Inactive
@bryantbiggs bryantbiggs changed the title feat(karpenter): add auto discovery permission of endpoint to karpenter role feat: Add auto discovery permission of endpoint to karpenter role Feb 11, 2023
@bryantbiggs bryantbiggs merged commit 05da30c into aws-ia:main Feb 11, 2023
@FernandoMiguel
Copy link
Contributor

@woehrl01 @bryantbiggs this is a breaking change

│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 30, in data "aws_iam_policy_document" "karpenter":
│   30:       "arn:${var.addon_context.aws_partition_id}:ec2:${var.addon_context.aws_region_name}:${var.addon_context.aws_caller_identity_account_id}:*",
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.
╵
╷
│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 31, in data "aws_iam_policy_document" "karpenter":
│   31:       "arn:${var.addon_context.aws_partition_id}:ec2:${var.addon_context.aws_region_name}::image/*"
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.
╵
╷
│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 46, in data "aws_iam_policy_document" "karpenter":
│   46:     resources = ["arn:${var.addon_context.aws_partition_id}:iam::${var.addon_context.aws_caller_identity_account_id}:role/*"]
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.
╵
╷
│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 66, in data "aws_iam_policy_document" "karpenter":
│   66:     resources = ["arn:${var.addon_context.aws_partition_id}:ssm:${var.addon_context.aws_region_name}::parameter/*"]
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.
╵
╷
│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 75, in data "aws_iam_policy_document" "karpenter":
│   75:     resources = ["arn:${var.addon_context.aws_partition_id}:eks:*:${var.addon_context.aws_caller_identity_account_id}:cluster/${var.addon_context.eks_cluster_id}"]
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.
╵
╷
│ Error: Invalid template interpolation value
│
│   on .terraform/modules/base_system.karpenter.eks_blueprints_kubernetes_addons_karpenter/modules/kubernetes-addons/karpenter/data.tf line 81, in data "aws_iam_policy_document" "karpenter":
│   81:     resources = ["arn:${var.addon_context.aws_partition_id}:ec2:${var.addon_context.aws_region_name}:${var.addon_context.aws_caller_identity_account_id}:instance/*"]
│     ├────────────────
│     │ var.addon_context.aws_partition_id is null
│
│ The expression result is null. Cannot include a null value in a string template.

since until now we didn't need aws_partition_id, that was not set.
now v4.22.0 requires it but nothing is mentioned in the release notes

@bryantbiggs
Copy link
Contributor

this is not a breaking change since aws_partition_id has always been included in the variable definition

@FernandoMiguel
Copy link
Contributor

it has been working fine in all previous versions with that set to null since it was not required until now.

@bryantbiggs
Copy link
Contributor

That may be true, but per the definition of the module defined here - it is not a breaking change. Any of the attributes under the addon_context variable are available to use, which is what has been done in the change for this PR

@FernandoMiguel
Copy link
Contributor

ok. good to know.
but a notice in the changelog would be appreciated, so others using this module are not caught by surprise, like we were.
thanks

@bryantbiggs
Copy link
Contributor

understood, but unfortunately we cannot capture the things that we are not aware of (i.e. - the different ways that folks may or may not be using the code provided here). We only capture the changes as they relate to the API or functionality as exposed by the code here per its definition. The definition did not change and therefore, no additional changes are reported other than there is an additional permission added for cluster endpoint discovery

@FernandoMiguel
Copy link
Contributor

I totally understand that this project can't guess everything practitioners will do.
but at the same time, this PR changed the requirements.
previously, aws_partition_id was not used and now is. So that is a change of behaviour.
Expecting the variable to be there is one thing, but there is still a change of expectations.

@bryantbiggs
Copy link
Contributor

but at the same time, this PR changed the requirements.

I think that is the misconception here. The requirements have not changed - whether the variable attribute was used or not is inconsequential - it was part of the variable definition and its not using the newer optional syntax, so it is therefore assumed to be required.

That said, I do not like our use of these large, obfuscated variables and would very much like to do away with them but that will be a larger, breaking change to refactor those

gminiba pushed a commit to gminiba/terraform-aws-eks-blueprints that referenced this pull request Mar 17, 2023
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.

3 participants