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

Kubernetes-addon namespaces can be specified and creation optional #595

Merged
merged 5 commits into from Jun 30, 2022

Conversation

bobdoah
Copy link
Contributor

@bobdoah bobdoah commented Jun 3, 2022

What does this PR do?

This change makes it possible to use any of the kubernetes-addon modules with an already extant namespace. In a number of instances, the namespace creation is done via the IRSA module. In those cases, this PR also ensures the namespace is passed from the helm_config.

Motivation

I'm attempting to get my colleagues to adopt this module into our EKS Terraform projects. In our case we deploy operational modules to a single namespace. This is created outside the addon modules. To migrate to these modules, we need to be able to disable namespace creation and allow the namespace to be specified.

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 updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs required examples and docs except a new pattern or add-on added.

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@bobdoah bobdoah temporarily deployed to EKS Blueprints Test June 3, 2022 19:29 Inactive
@bobdoah bobdoah changed the title Optional namespaces Namespaces can be specified and creation optional Jun 3, 2022
@bobdoah bobdoah changed the title Namespaces can be specified and creation optional Kubernetes-addon namespaces can be specified and creation optional Jun 3, 2022
@Hokwang
Copy link
Contributor

Hokwang commented Jun 8, 2022

Please add kubernetes-dashboard

@bobdoah
Copy link
Contributor Author

bobdoah commented Jun 9, 2022

Please add kubernetes-dashboard

Done @Hokwang. I thought the namespace had been removed entirely from the Terraform. Thanks for highlighting this.

@bobdoah
Copy link
Contributor Author

bobdoah commented Jun 14, 2022

@bryantbiggs any chance you could review this again?

@bobdoah
Copy link
Contributor Author

bobdoah commented Jun 24, 2022

Thanks for the changes @bryantbiggs. Was just looking at your comments and then they appeared.

@bryantbiggs
Copy link
Contributor

@bobdoah 😅 give me a sec and I should have the rest of the updates - we'll get this across the line today

@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test June 24, 2022 21:07 Inactive
@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test June 24, 2022 21:52 Inactive
Copy link
Contributor

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bobdoah !

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

@bobdoah Thanks for the PR but would expect that this PR should address only the namespace issue as mentioned the description.

This is a very big change with major refactoring for all the add-ons. It's advisable to do this kind of refactoring with small PRs for each add-on with proper testing.

Comment on lines 1 to 18
module "helm_addon" {
source = "../helm-addon"
source = "../helm-addon"

manage_via_gitops = var.manage_via_gitops
helm_config = local.helm_config
irsa_config = null
addon_context = var.addon_context

depends_on = [kubernetes_namespace_v1.this]
}
helm_config = merge(
{
name = "agones"
chart = "agones"
repository = "https://agones.dev/chart/stable"
version = "1.23.0"
namespace = "agones-system"
create_namespace = true
description = "Agones Gaming Server Helm Chart deployment configuration"
values = [file("${path.module}/values.yaml")]
},
var.helm_config
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bobdoah Any reason why you are refactoring all the add-ons?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vara-bonthu I made most of the changes. When reviewing the PR, there were too many inconsistencies across the different addons in how names, nampespaces, etc. + the merging of provided config with var.helm_config that I just dove in to start correcting. Please let me know how you would like to handle these changes so that the users can opt in or out of creating namespaces while keeping the addons consistent

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

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

LGTM! pending CI checks.

@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test June 30, 2022 20:16 Inactive
@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test June 30, 2022 21:54 Inactive
@bryantbiggs
Copy link
Contributor

@askulkarni2 should be ready to re-review if you have time - merge away if things look ok

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@askulkarni2 askulkarni2 merged commit 7c7b6ec into aws-ia:main Jun 30, 2022
@bobdoah
Copy link
Contributor Author

bobdoah commented Jul 1, 2022

Thanks for all the changes to sort this @bryantbiggs and merging @askulkarni2

@bobdoah bobdoah deleted the optional-namespaces branch July 13, 2022 09:37
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Dec 15, 2022
…on optional (aws-ia#595)

Co-authored-by: Bryant Biggs <bryantbiggs@gmail.com>
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.

None yet

7 participants