cloudposse / terraform-aws-eks-cluster

Terraform module for provisioning an EKS cluster
https://cloudposse.com/accelerate
Apache License 2.0
513 stars 354 forks source link

Use AWS API for EKS authentication and authorization #206

Closed Nuru closed 8 months ago

Nuru commented 8 months ago

Major Breaking Changes

[!WARNING] This release has major breaking changes and requires significant manual intervention to upgrade existing clusters. Read the migration document for more details.

what

why

references

Nuru commented 8 months ago

/terratest

Nuru commented 8 months ago

/terratest

z0rc commented 8 months ago

I'm working on migrating my test cluster. Haven't reached plan stage yet, just changing code to new module variables. I'd appreciate, if there were examples about new access_* variables.

With module version 3.0.0, I have somewhat typical configuration:

  map_additional_iam_roles = [
    {
      rolearn  = replace(data.aws_iam_role.administrator_access.arn, "${data.aws_iam_role.administrator_access.path}/", "")
      username = "devops"
      groups   = ["system:masters", "devops"]
    },
    {
      rolearn  = data.aws_iam_role.gitlab_ci.arn
      username = "gitlab-ci"
      groups   = ["system:masters", "ci"]
    },
    {
      rolearn  = aws_iam_role.karpenter_node.arn
      username = "system:node:{{EC2PrivateDNSName}}"
      groups   = ["system:bootstrappers", "system:nodes"]
    },
    {
      rolearn  = aws_iam_role.fargate.arn
      username = "system:node:{{SessionName}}"
      groups   = ["system:bootstrappers", "system:nodes", "system:node-proxier"]
    },
  ]

And it isn't clear what would new access_* values should look like without checking module's code.

AFAIU I should:

Is this right?

Also there is some abbreviations to simplify things at https://github.com/cloudposse/terraform-aws-eks-cluster/pull/206/files#diff-7ca243b22dbf3bdfd94ff409bd87a336e7f6f61601041b57d470ae3b2f11e71fR6-R14, which aren't documented.

Nuru commented 8 months ago

@z0rc Thank you for the feedback.

Answers to your questions are supposed to be in the migration document, and the README in the form of documentation of the input variables.

Particularly regarding the abbreviations, there is the migration doc and the variable description. There is alos a brief example, as always, in examples/complete

I grant that there probably should be more in the README, but are you saying you didn't see the documentation, or that you read it but it did not satisfy you? (Your answer will guide my improvements.)

P.S. I'm not sure where data.aws_iam_session_context.current.issuer_arn came from, but in general we do not recommend implicit configuration like that. Otherwise I think you have it right.

I updated the migration doc with an example transformation, using your configuration as a starting point.

Nuru commented 8 months ago

/terratest

Nuru commented 8 months ago

Not too much involved in EKS, so leaving the more specific reviews for others.

Would suggest to add more "BREAKING CHANGE" to commit and PR titles, so it cannot be overlooked.

We're going to rely on the major version number bump to do the heavy lifting of "BREAKING CHANGE". The mitigating factors are that the new module will simply error out in a number of ways if you blindly try to update, and in general none of the changes should cause you to lose data. Worst consequence I can foresee from blindly plowing ahead is a security group or rule might change that cuts off network access.

Nuru commented 8 months ago

/terratest

z0rc commented 8 months ago

@Nuru thanks!

With provided example I was able to understand access entries relationships and migration path. I believe I was a bit overwhelmed by amount of migration documentation. Ultimately I'd be able to do migration myself given more time.

P.S. I'm not sure where data.aws_iam_session_context.current.issuer_arn came from

I was reading example at https://github.com/cloudposse/terraform-aws-eks-cluster/blob/v4-rc/examples/complete/main.tf#L39-L48, plus https://github.com/cloudposse/terraform-aws-eks-cluster/blob/v4-rc/docs/migration-v3-v4.md#error-creating-eks-access-entry I wrongly assumed that I must add this entry now.

Overall I'm very impressed with how many thoughts were put into migration process. Much appreciated!

z0rc commented 8 months ago

I migrated my test cluster to access_config.authentication_mode = "API" successfully.

mergify[bot] commented 8 months ago

[!IMPORTANT]

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

mergify[bot] commented 8 months ago

This pull request now has conflicts. Could you fix it @Nuru? 🙏

Nuru commented 8 months ago

/terratest

Nuru commented 8 months ago

/terratest