Additional managed node outputs #644

TBeijen · 2019-12-22T13:16:58Z

PR o'clock

Description

Work in progress for #643

Additional node group output.

Checklist

Change added to CHANGELOG.md. All changes must be added and breaking changes and highlighted
CI tests are passing
README.md has been updated after any changes to variables and outputs. See /~https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

TBeijen · 2019-12-22T13:44:47Z

Sample output in current form:

node_groups = [
  {
    "arn" = "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-a-rapid-skylark/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
    "iam_role_arn" = "arn:aws:iam::NNNNNNNN:role/apps-1-managed-node-groups"
    "id" = "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-a-rapid-skylark/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
    "name" = "apps-1-ng-managed-a-rapid-skylark"
    "resources" = [
      {
        "autoscaling_groups" = [
          {
            "name" = "eks-nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
          },
        ]
        "remote_access_security_group_id" = "sg-nnnnnnnnnnn"
      },
    ]
    "status" = "ACTIVE"
  },
  {
    "arn" = "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-b-wired-parrot/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
    "iam_role_arn" = "arn:aws:iam::NNNNNNNN:role/apps-1-managed-node-groups"
    "id" = "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-b-wired-parrot/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
    "name" = "apps-1-ng-managed-b-wired-parrot"
    "resources" = [
      {
        "autoscaling_groups" = [
          {
            "name" = "eks-nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
          },
        ]
        "remote_access_security_group_id" = "sg-nnnnnnnnnnn"
      },
    ]
    "status" = "ACTIVE"
  },
]

Same order as node group definition.

Would this format be favorable over having a list for each attribute, like:

node_group_arns = [
    "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-a-rapid-skylark/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn",
    "arn:aws:eks:eu-west-1:NNNNNNNN:nodegroup/apps-1/apps-1-ng-managed-b-wired-parrot/nnnnnnnn-nnnn-nnnn-nnnn-nnnnnnnnnnnn"
}
node_group_iam_role_arns = [
    "arn:aws:iam::NNNNNNNN:role/apps-1-managed-node-groups",
    "arn:aws:iam::NNNNNNNN:role/apps-1-managed-node-groups"
]
# etc...

The latter might be more consistent with existing worker group outputs but I wonder what would be most user-friendly. Thoughts?

barryib · 2019-12-23T23:10:35Z

outputs.tf

@@ -165,3 +165,19 @@ output "node_groups_iam_role_arns" {
    node_group.node_group_name => node_group.node_role_arn
  }
 }
+
+output "node_groups" {
+  value = [


could you please add description ?

Sure. Was planning on adding a docs .md describing matters in more detail. First I wanted to see if there's feedback on the output structure.

dpiddockcmp · 2019-12-26T18:38:13Z

The node_groups themselves are generated from a for_each. I feel this output would be much easier to work with if it was a map of maps instead of a list of maps. Similar to the node_groups_iam_role_arns output just above. The node_groups don't have a predictable order due to being created from a for_each.

And now I've looked at the code a little closer, the node_group.node_group_name is generated based on a random pet resource which makes that output pretty much useless for predictable reuse. 🤷‍♂

This node_group code is confusing. Why is user input a list of maps, which then get turned into a map of maps based on something that's not enforced unique in the user input? Oh my.

Totally out of scope of this PR but I'd love for this to be input:

node_groups = {
  group1 = { instance_type = "t3.medium" }
  group2 = { instance_type = "c5.xlarge" }
}

And your outputs to be:

node_groups = {
  group1 = {
    role_arn = "..."
    ...
  }
  group2 = {
    role_arn = "..."
    ...
  }
}

TBeijen · 2019-12-27T13:43:02Z

The node_groups themselves are generated from a for_each. I feel this output would be much easier to work with if it was a map of maps instead of a list of maps.

This node_group code is confusing. Why is user input a list of maps, which then get turned into a map of maps based on something that's not enforced unique in the user input? Oh my.

I chose list output based on node_groups input being a list. But, admittedly I didn't take a very close look at the inner workings of the existing code, assuming (never assume) constructs were there for a reason.

Imo the intermediate map based on something not guaranteed unique is a red flag.

In the light of @max-rocket-internet comment in #635 I'm inclined to focus on a poc for a separate managed_node_group module instead. It might be less work than hammering this into a good shape and quite a lot of the complexity in the current state seems to originate from needing to manage lists/maps of inputs and outputs.

dpiddockcmp · 2019-12-27T22:37:44Z

Ran with my comments and created #650

max-rocket-internet · 2020-01-02T14:24:32Z

I'm inclined to focus on a poc for a separate managed_node_group module instead

I think that's a good idea. I'll close this then 🙂

Add change from @TBeijin from PR terraform-aws-modules#644

* WIP Move node_groups to a submodule * Split the old node_groups file up * Start moving locals * Simplify IAM creation logic * depends_on from the TF docs * Wire in the variables * Call module from parent * Allow to customize the role name. As per workers * aws_auth ConfigMap for node_groups * Get the managed_node_groups example to plan * Get the basic example to plan too * create_eks = false works "The true and false result expressions must have consistent types. The given expressions are object and object, respectively." Well, that's useful. But apparently set(string) and set() are ok. So everything else is more complicated. Thanks. * Update Changelog * Update README * Wire in node_groups_defaults * Remove node_groups from workers_defaults_defaults * Synchronize random and node_group defaults * Error: "name_prefix" cannot be longer than 32 * Update READMEs again * Fix double destroy Was producing index errors when running destroy on an empty state. * Remove duplicate iam_role in node_group I think this logic works. Needs some testing with an externally created role. * Fix index fail if node group manually deleted * Keep aws_auth template in top module Downside: count causes issues as usual: can't use distinct() in the child module so there's a template render for every node_group even if only one role is really in use. Hopefully just output noise instead of technical issue * Hack to have node_groups depend on aws_auth etc The AWS Node Groups create or edit the aws-auth ConfigMap so that nodes can join the cluster. This breaks the kubernetes resource which cannot do a force create. Remove the race condition with explicit depend. Can't pull the IAM role out of the node_group any more. * Pull variables via the random_pet to cut logic No point having the same logic in two different places * Pass all ForceNew variables through the pet * Do a deep merge of NG labels and tags * Update README.. again * Additional managed node outputs #644 Add change from @TBeijin from PR #644 * Remove unused local * Use more for_each * Remove the change when create_eks = false * Make documentation less confusing * node_group version user configurable * Pass through raw output from aws_eks_node_groups * Merge workers defaults in the locals This simplifies the random_pet and aws_eks_node_group logic. Which was causing much consernation on the PR. * Fix typo Co-authored-by: Max Williams <max.williams@deliveryhero.com>

* Don't fail on destroy, when provider resource was removed * Update Changelog * Node groups submodule (#650) * WIP Move node_groups to a submodule * Split the old node_groups file up * Start moving locals * Simplify IAM creation logic * depends_on from the TF docs * Wire in the variables * Call module from parent * Allow to customize the role name. As per workers * aws_auth ConfigMap for node_groups * Get the managed_node_groups example to plan * Get the basic example to plan too * create_eks = false works "The true and false result expressions must have consistent types. The given expressions are object and object, respectively." Well, that's useful. But apparently set(string) and set() are ok. So everything else is more complicated. Thanks. * Update Changelog * Update README * Wire in node_groups_defaults * Remove node_groups from workers_defaults_defaults * Synchronize random and node_group defaults * Error: "name_prefix" cannot be longer than 32 * Update READMEs again * Fix double destroy Was producing index errors when running destroy on an empty state. * Remove duplicate iam_role in node_group I think this logic works. Needs some testing with an externally created role. * Fix index fail if node group manually deleted * Keep aws_auth template in top module Downside: count causes issues as usual: can't use distinct() in the child module so there's a template render for every node_group even if only one role is really in use. Hopefully just output noise instead of technical issue * Hack to have node_groups depend on aws_auth etc The AWS Node Groups create or edit the aws-auth ConfigMap so that nodes can join the cluster. This breaks the kubernetes resource which cannot do a force create. Remove the race condition with explicit depend. Can't pull the IAM role out of the node_group any more. * Pull variables via the random_pet to cut logic No point having the same logic in two different places * Pass all ForceNew variables through the pet * Do a deep merge of NG labels and tags * Update README.. again * Additional managed node outputs #644 Add change from @TBeijin from PR #644 * Remove unused local * Use more for_each * Remove the change when create_eks = false * Make documentation less confusing * node_group version user configurable * Pass through raw output from aws_eks_node_groups * Merge workers defaults in the locals This simplifies the random_pet and aws_eks_node_group logic. Which was causing much consernation on the PR. * Fix typo Co-authored-by: Max Williams <max.williams@deliveryhero.com> * Update Changelog * Add public access endpoint CIDRs option (terraform-aws-eks#647) (#673) * Add public access endpoint CIDRs option (terraform-aws-eks#647) * Update required provider version to 2.44.0 * Fix formatting in docs * Re-generate docs with terraform-docs 0.7.0 and bump pre-commit-terraform version (#668) * re-generate docs with terraform-docs 0.7.0 * bump pre-commit-terraform version * Release 8.0.0 (#662) * Release 8.0.0 * Update changelog * remove 'defauls' node group * Make curl silent * Update Changelog Co-authored-by: Daniel Piddock <33028589+dpiddockcmp@users.noreply.github.com> Co-authored-by: Max Williams <max.williams@deliveryhero.com> Co-authored-by: Siddarth Prakash <1428486+sidprak@users.noreply.github.com> Co-authored-by: Thierno IB. BARRY <ibrahima.br@gmail.com>

github-actions · 2022-11-20T02:31:57Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

TBeijen added 3 commits December 21, 2019 14:44

Added node group outputs.

697f285

Added node group resources to outputs.

bb4b504

Added resulting iam role arn and eks node group name to outputs.

ac61778

TBeijen force-pushed the managed_node_outputs branch from f9ff4c5 to ac61778 Compare December 22, 2019 13:38

TBeijen mentioned this pull request Dec 23, 2019

Managed node group limits to 1 instance type (TF resource allows multiple) #642

Closed

4 tasks

barryib suggested changes Dec 23, 2019

View reviewed changes

TBeijen mentioned this pull request Dec 27, 2019

Added readme with design considerations. #649

Closed

3 tasks

dpiddockcmp mentioned this pull request Dec 27, 2019

Node groups submodule #650

Merged

7 tasks

max-rocket-internet closed this Jan 2, 2020

dpiddockcmp pushed a commit to dpiddock/terraform-aws-eks that referenced this pull request Jan 2, 2020

Additional managed node outputs terraform-aws-modules#644

bec2a76

Add change from @TBeijin from PR terraform-aws-modules#644

dpiddockcmp pushed a commit to dpiddock/terraform-aws-eks that referenced this pull request Jan 7, 2020

Additional managed node outputs terraform-aws-modules#644

78c9272

Add change from @TBeijin from PR terraform-aws-modules#644

github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional managed node outputs #644

Additional managed node outputs #644

TBeijen commented Dec 22, 2019

TBeijen commented Dec 22, 2019 •

edited

Loading

barryib Dec 23, 2019

TBeijen Dec 24, 2019

dpiddockcmp commented Dec 26, 2019

TBeijen commented Dec 27, 2019

dpiddockcmp commented Dec 27, 2019

max-rocket-internet commented Jan 2, 2020

github-actions bot commented Nov 20, 2022

Additional managed node outputs #644

Additional managed node outputs #644

Conversation

TBeijen commented Dec 22, 2019

PR o'clock

Description

Checklist

TBeijen commented Dec 22, 2019 • edited Loading

barryib Dec 23, 2019

Choose a reason for hiding this comment

TBeijen Dec 24, 2019

Choose a reason for hiding this comment

dpiddockcmp commented Dec 26, 2019

TBeijen commented Dec 27, 2019

dpiddockcmp commented Dec 27, 2019

max-rocket-internet commented Jan 2, 2020

github-actions bot commented Nov 20, 2022

TBeijen commented Dec 22, 2019 •

edited

Loading