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

Enable 1 Pod Execution Role Per Cluster #33

Merged
merged 4 commits into from
Jul 11, 2023
Merged

Enable 1 Pod Execution Role Per Cluster #33

merged 4 commits into from
Jul 11, 2023

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Jul 11, 2023

what

  • Enable 1 Pod Execution Role Per Cluster by allowing users to create either a role or a profile or both
  • Deprecate "profile role" terminology in favor of "pod execution role"
  • Update example
  • Update tests
  • Update README and workflows

why

  • All Fargate Profiles can use the same Pod Execution Role, and there is no advantage (and several disadvantages) to using a different role with each profile. This module used to always create both a profile and a role. Now it allows you to create one or the other or both.
  • The term "profile role" is confusing and suggests using one role per profile. The term "pod execution role" matches AWS documentation.
  • Example used old modules that were incompatible with AWS provider v5 and did not set ALB tags.
  • Explain 1 role per cluster. Supersedes and closes Update README.md and docs #32

@Nuru Nuru added the minor New features that do not break anything label Jul 11, 2023
@Nuru Nuru requested review from a team as code owners July 11, 2023 09:50
@Nuru Nuru requested a review from joe-niland July 11, 2023 09:50
@Nuru
Copy link
Contributor Author

Nuru commented Jul 11, 2023

/terratest

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

LGTM, a few nitpicks

main.tf Outdated
Comment on lines 17 to 19
fargate_profile_iam_role_name = local.fargate_pod_execution_role_name != null ? local.fargate_pod_execution_role_name : (
var.fargate_profile_enabled ? "${module.role_label.id}${var.iam_role_kubernetes_namespace_delimiter}${var.kubernetes_namespace}" :
module.role_label.id)
Copy link

Choose a reason for hiding this comment

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

I think this would be less painful like this

Suggested change
fargate_profile_iam_role_name = local.fargate_pod_execution_role_name != null ? local.fargate_pod_execution_role_name : (
var.fargate_profile_enabled ? "${module.role_label.id}${var.iam_role_kubernetes_namespace_delimiter}${var.kubernetes_namespace}" :
module.role_label.id)
fargate_profile_concatenated = "${module.role_label.id}${var.iam_role_kubernetes_namespace_delimiter}${var.kubernetes_namespace}"
default_fargate_profile_role_name = var.fargate_profile_enabled ? local.fargate_profile_concatenated : module.role_label.id
fargate_profile_iam_role_name = coalesce(local.fargate_pod_execution_role_name, local.default_fargate_profile_role_name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is:

fargate_profile_concatenated = "${module.role_label.id}${var.iam_role_kubernetes_namespace_delimiter}${var.kubernetes_namespace}"

fails with Error if var.kubernetes_namespace is null, which it could be if user is only provisioning the Pod Execution Role.

Copy link

Choose a reason for hiding this comment

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

okay, I wouldn't see why that couldn't be coalesced as well.
So:

fargate_profile_concatenated = "${module.role_label.id}${var.iam_role_kubernetes_namespace_delimiter}${coalesce(var.kubernetes_namespace,"")}"

or if any number of the elements could be null, then use:

fargate_profile_concatenated = join("", compact([module.role_label.id,var.iam_role_kubernetes_namespace_delimiter,var.kubernetes_namespace]))

Copy link

Choose a reason for hiding this comment

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

To be clear, I'm just offering options to avoid a nested conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coalesce fails when all the arguments are empty. The advantage of the ternary operator in Terraform is that the unused expressions are not evaluated and need not be valid.

dudymas
dudymas previously approved these changes Jul 11, 2023
Copy link

@dudymas dudymas left a comment

Choose a reason for hiding this comment

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

This is functionally fine, but added some readability and comprehension remarks

@Nuru
Copy link
Contributor Author

Nuru commented Jul 11, 2023

/terratest

aknysh
aknysh previously approved these changes Jul 11, 2023
Benbentwo
Benbentwo previously approved these changes Jul 11, 2023
@Nuru Nuru dismissed stale reviews from Benbentwo and aknysh via 1acd9a8 July 11, 2023 22:45
@Nuru
Copy link
Contributor Author

Nuru commented Jul 11, 2023

/terratest

@Nuru Nuru merged commit 76953f5 into main Jul 11, 2023
@Nuru Nuru deleted the single-role branch July 11, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants