Ensure serviceaccount created in correct namespace #398

bobdoah · 2022-04-11T15:23:49Z

What does this PR do?

The serviceaccount for a module is created via the IRSA module. If the
namespace of a module differs from the default, it needs to also be
passed to the IRSA module. Otherwise any deployments tied to the
serviceaccount will fail to deploy pods.

This PR changes the modules so that they can be deployed in a namespace other than "kube-system".

Motivation

I deploy the aws-load-balancer to a different namespace, in my configuration. It is a security requirement. I am unable to use recent versions of this module to achieve that. This should fix that issue.

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 ran pre-commit run -a with this PR

For Moderators

E2E Test successfully complete before merge?

Additional Notes

askulkarni2

@bobdoah thanks for the PR! LGTM!

vara-bonthu

Thanks for the PR 👍🏼 Few changes required

vara-bonthu · 2022-04-12T16:34:15Z

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf

@@ -40,7 +40,7 @@ locals {
  }

  irsa_config = {
-    kubernetes_namespace              = "kube-system"
+    kubernetes_namespace              = local.helm_config["namespace"]
    kubernetes_service_account        = local.service_account_name
    create_kubernetes_namespace       = false


This needs to be changed to create_kubernetes_namespace = true when we use namespace other than kube-system to ensure namespace is created by the IRSA module

@askulkarni2 May be we need to handle this in IRSA module to look for kube-system when create_kubernetes_namespace=true.

See below

resource "kubernetes_namespace_v1" "irsa" { count = var.create_kubernetes_namespace && var.kubernetes_namespace != "kube-system" ? 1 : 0 metadata { name = var.kubernetes_namespace labels = { "app.kubernetes.io/managed-by" = "terraform-eks-blueprints" } } }

This needs to be changed to create_kubernetes_namespace = true when we use namespace other than kube-system to ensure namespace is created by the IRSA module

I'm not sure I follow the logic. In my use-case, the namespace already exists. So this wouldn't work for me. That isn't done in other instances such as for the kubernetes_dashboard module.

I would suggest the create_kubernetes_namespace option should be configurable for each of the given addon modules. Perhaps as part of the addon_context.

I'd be happy to look at that, but this is not the intent of this PR. I'm trying to draw these modules level with others such as the kubernetes_dashboard that would work with an existing namespace that is not kube-system. This behaviour did previously work.

Thanks @bobdoah I appreciate your logic. Let me go through in detail

Namespace creation is handled only by EKS Blueprints module at this moment apart from existing system namespaces like kube-system. Users can pass any name as namespace through the below configuration and expects module to create the namespace

enable_aws_load_balancer_controller = true # Optional aws_load_balancer_controller_helm_config = { name = "aws-load-balancer-controller" chart = "aws-load-balancer-controller" repository = "https://aws.github.io/eks-charts" version = "1.3.1" namespace = "my_alb_ns" values = [templatefile("${path.module}/values.yaml", {})] }

User defined my_alb_ns namespace overrides the locally defined namespace. When we specify kubernetes_namespace = local.helm_config["namespace"] and create_kubernetes_namespace = false then the above example will introduce a defect as my_alb_ns wont be created due to false condition.

Yes, I just noticed that kubernetes-dashboard config is incorrect as well. I think the solution for this would be as i mentioned in my previous comment.

We will discuss about your suggestion allowing users to define create_kubernetes_namespace through helm config

@vara-bonthu I understand what you're saying, but that doesn't fit my use case. The namespace has already been created externally, it's not kube-system. I have no control over this, sadly. If I set it to true I still won't be able to use this module, as-is.

I have pushed a further commit. I think this is the simplest way I can satisfy both yours and my use cases. The namespace will be created, if it is not the default kube-system. The create_kubernetes_namespace option can be overridden by supplying a custom irsa_config to the module. The contents will be merged, in the same way as is done for the helm_config.

kcoleman731

LGTM. @vara-bonthu anything else needed here?

vara-bonthu · 2022-04-16T12:34:42Z

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf

+
+  irsa_config = merge(
+    local.default_irsa_config,
+    var.irsa_config
+  )


remove this block

vara-bonthu · 2022-04-16T12:35:50Z

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf

@@ -39,11 +39,16 @@ locals {
    serviceAccountName = local.service_account_name
  }

-  irsa_config = {
-    kubernetes_namespace              = "kube-system"
+  default_irsa_config = {


change this back to irsa_config

vara-bonthu · 2022-04-16T12:36:06Z

modules/kubernetes-addons/aws-load-balancer-controller/variables.tf

+variable "irsa_config" {
+  type        = any
+  description = "IRSA config for the aws_load_balancer_controller. "
+  default     = {}
+}


remove this variable

vara-bonthu · 2022-04-16T12:40:34Z

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf

    kubernetes_service_account        = local.service_account_name
-    create_kubernetes_namespace       = false
+    create_kubernetes_namespace       = var.kubernetes_namespace != "kube-system" ? true : false


replace this line in IRSA module

terraform-aws-eks-blueprints/modules/irsa/main.tf

Line 2 in 91dc365

count = var.create_kubernetes_namespace ? 1 : 0

with the below

count = var.create_kubernetes_namespace && var.kubernetes_namespace != "kube-system" ? 1 : 0

vara-bonthu · 2022-04-16T12:49:27Z

modules/kubernetes-addons/cluster-autoscaler/locals.tf

@@ -36,7 +36,7 @@ locals {

  irsa_config = {
    create_kubernetes_namespace       = false
-    kubernetes_namespace              = "kube-system"


Here we are forcing the Cluster CA to run on kube-system. Don't change this line

Running CA in other namespace on a user node is risky. If CA erases the node it is sitting on and some race condition occurs you may end up with cluster without both CA and capacity to run anything.

Hence its really important that...

CA stays in kube-system OR it has restrictive PodDisruptionBudget
CA is run at high priority or is marked as a critical pod

vara-bonthu

Hi @bobdoah I thought about a better way of handling this scenario. Please see my new comments to the PR.

Please follow these steps if you want to bring your own namespace.

Note:

EKS Blueprints manages the Namespace by default
If user wants to manage your namespace then you can pass the following config.

  #---------------------------------------
  # AWS LOAD BALANCER INGRESS CONTROLLER HELM ADDON
  #---------------------------------------
  enable_aws_load_balancer_controller = true
  # Optional
  aws_load_balancer_controller_helm_config = {
    name       = "aws-load-balancer-controller"
    namespace  = "my-own-namespace"
    create_namespace = false
  }

Give this change a try and let me know if it works for you. I will try to update the other Add-ons with the similar approach.

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf

The serviceaccount for a module is created via the IRSA module. If the namespace of a module differs from the default, it needs to also be passed to the IRSA module. Otherwise any deployments tied to the serviceaccount will fail to deploy pods.

@vara-bonthu

Create the namespace, if it is not the default "kube-system" which (typically) exists on every cluster. To allow the namespace to be created external to this module, allow the IRSA config to be overridden. This should fit both my and @vara-bonthu's proposed use cases.

bobdoah · 2022-04-19T10:19:09Z

@vara-bonthu updated as requested.

ZeroDeth · 2022-04-20T08:08:55Z

Facing the same issue with multiple add-ons: pending-installs

tetrate_istio
cert-manager
traefik
aws_load_balancer_controller

bobdoah · 2022-04-20T14:48:25Z

Facing the same issue with multiple add-ons: pending-installs

tetrate_istio

cert-manager

traefik

aws_load_balancer_controller

Applied the same try/true default to cert-manager.

The tetrate_istio module is a completely separate repository. It does appear to support a create_namespace argument on the Helm config https://github.com/tetratelabs/terraform-eksblueprints-tetrate-istio-addon/blob/5027357081f0e62dc8fb0c135197c908b6463e4b/locals.tf#L13. @ZeroDeth, I think you should be able to make that work by supplying:

module "kubernetes_addons" {
 source = "github.com/aws-ia/terraform-aws-eks-blueprints//modules/kubernetes-addons"
  
  ...
  tetrate_istio_base_helm_config = {
    create_namespace = false
  }
}

The traefik module is more complicated, as the namespace resource is created directly in the module. It could be put behind a count like so:

 resource "kubernetes_namespace_v1" "this" {
+  count = try(local.helm_config["create_namespace"], true) ? 1 : 0
   metadata {
     name = local.helm_config["namespace"]

If that would suffice. It would, however potentially remove the resources of anyone upgrading. A moved block would help with this, but that's only supported in Terraform >1.1.

bobdoah · 2022-04-21T08:10:21Z

@vara-bonthu @kcoleman731 @askulkarni2 any chance of getting this merged, now that @vara-bonthu is happy.

Thanks

vara-bonthu · 2022-04-21T12:36:48Z

@bobdoah Happy to merge now. Just checking to see if this PR is working for you?

ZeroDeth · 2022-04-21T13:11:44Z

@bobdoah Testing now. Thanks

bobdoah · 2022-04-22T16:11:04Z

@vara-bonthu Yep, this is working for me.

ZeroDeth · 2022-04-23T09:40:21Z

cert-manager & ingress-nginx failed. probably I am doing it wrong.

Warning: Helm release "cert-manager" was created but has a failed status. Use the `helm` command to investigate the error, correct it, then run Terraform again.

with module.eks_addons_0.module.cert_manager[0].module.helm_addon.helm_release.addon[0], on .terraform/modules/eks_addons_0/modules/kubernetes-addons/helm-addon/main.tf line 1, in resource "helm_release" "addon":1: resource "helm_release" "addon" {

Warning: Helm release "ingress-nginx" was created but has a failed status. Use the `helm` command to investigate the error, correct it, then run Terraform again.

with module.eks_addons_0.module.ingress_nginx[0].module.helm_addon.helm_release.addon[0], on .terraform/modules/eks_addons_0/modules/kubernetes-addons/helm-addon/main.tf line 1, in resource "helm_release" "addon":1: resource "helm_release" "addon" {

Error: failed post-install: timed out waiting for the condition with module.eks_addons_0.module.cert_manager[0].module.helm_addon.helm_release.addon[0], on .terraform/modules/eks_addons_0/modules/kubernetes-addons/helm-addon/main.tf line 1, in resource "helm_release" "addon":1: resource "helm_release" "addon" {

Error: timed out waiting for the condition with module.eks_addons_0.module.ingress_nginx[0].module.helm_addon.helm_release.addon[0], on .terraform/modules/eks_addons_0/modules/kubernetes-addons/helm-addon/main.tf line 1, in resource "helm_release" "addon":1: resource "helm_release" "addon" {

example 1

  enable_cert_manager = true
  cert_manager_helm_config = {
    name             = "cert-manager"
    chart            = "cert-manager"
    repository       = "https://charts.jetstack.io"
    version          = "v1.7.2"
    namespace        = "cert-manager"
    create_namespace = true
  }

  enable_ingress_nginx = true
  ingress_nginx_helm_config = {
    name       = "ingress-nginx"
    chart      = "ingress-nginx"
    repository = "https://kubernetes.github.io/ingress-nginx"
    version    = "4.0.17"
    values = [templatefile("${path.module}/helm_values/nginx-values.yaml", {
      hostname     = var.hosted_name
      ssl_cert_arn = module.shr_acm.acm_certificate_arn
    })]
  }

example 2

  enable_cert_manager = true

  enable_ingress_nginx = true
  ingress_nginx_helm_config = {
    values = [templatefile("${path.module}/helm_values/nginx-values.yaml", {
      hostname     = var.hosted_name
      ssl_cert_arn = module.shr_acm.acm_certificate_arn
    })]
  }

ZeroDeth · 2022-04-23T09:42:15Z

Facing the same issue with multiple add-ons: pending-installs

tetrate_istio

cert-manager

traefik

aws_load_balancer_controller

Applied the same try/true default to cert-manager.

The tetrate_istio module is a completely separate repository. It does appear to support a create_namespace argument on the Helm config https://github.com/tetratelabs/terraform-eksblueprints-tetrate-istio-addon/blob/5027357081f0e62dc8fb0c135197c908b6463e4b/locals.tf#L13. @ZeroDeth, I think you should be able to make that work by supplying:
module "kubernetes_addons" {
 source = "github.com/aws-ia/terraform-aws-eks-blueprints//modules/kubernetes-addons"
  
  ...
  tetrate_istio_base_helm_config = {
    create_namespace = false
  }
}
The traefik module is more complicated, as the namespace resource is created directly in the module. It could be put behind a count like so:
 resource "kubernetes_namespace_v1" "this" {
+  count = try(local.helm_config["create_namespace"], true) ? 1 : 0
   metadata {
     name = local.helm_config["namespace"]
If that would suffice. It would, however potentially remove the resources of anyone upgrading. A moved block would help with this, but that's only supported in Terraform >1.1.

Same errors with tetrate_istio

bobdoah requested review from askulkarni2, kcoleman731 and vara-bonthu as code owners April 11, 2022 15:23

askulkarni2 approved these changes Apr 11, 2022

View reviewed changes

vara-bonthu requested changes Apr 12, 2022

View reviewed changes

kcoleman731 approved these changes Apr 13, 2022

View reviewed changes

vara-bonthu reviewed Apr 16, 2022

View reviewed changes

vara-bonthu requested changes Apr 16, 2022

View reviewed changes

vara-bonthu reviewed Apr 16, 2022

View reviewed changes

modules/kubernetes-addons/aws-load-balancer-controller/locals.tf Outdated Show resolved Hide resolved

Robert Williams added 3 commits April 19, 2022 11:17

Apply reccommended changes

288ef5a

bobdoah force-pushed the main branch from 9de338f to 288ef5a Compare April 19, 2022 10:18

vara-bonthu approved these changes Apr 19, 2022

View reviewed changes

Apply the fix to cert-manager

0f963c2

bobdoah added 2 commits April 20, 2022 15:48

Merge branch 'main' into main

4e0ffef

Merge branch 'main' into main

7d1eabb

Merge branch 'main' into main

26b5340

vara-bonthu merged commit faf2f9a into aws-ia:main Apr 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure serviceaccount created in correct namespace #398

Ensure serviceaccount created in correct namespace #398

bobdoah commented Apr 11, 2022 •

edited

askulkarni2 left a comment

vara-bonthu left a comment

vara-bonthu Apr 12, 2022

vara-bonthu Apr 12, 2022

bobdoah Apr 12, 2022

vara-bonthu Apr 13, 2022 •

edited

bobdoah Apr 13, 2022

kcoleman731 left a comment

vara-bonthu Apr 16, 2022

vara-bonthu Apr 16, 2022

vara-bonthu Apr 16, 2022

vara-bonthu Apr 16, 2022

vara-bonthu Apr 16, 2022 •

edited

vara-bonthu left a comment •

edited

bobdoah commented Apr 19, 2022

ZeroDeth commented Apr 20, 2022 •

edited

bobdoah commented Apr 20, 2022

bobdoah commented Apr 21, 2022

vara-bonthu commented Apr 21, 2022

ZeroDeth commented Apr 21, 2022 •

edited

bobdoah commented Apr 22, 2022

ZeroDeth commented Apr 23, 2022 •

edited

ZeroDeth commented Apr 23, 2022

Ensure serviceaccount created in correct namespace #398

Ensure serviceaccount created in correct namespace #398

Conversation

bobdoah commented Apr 11, 2022 • edited

What does this PR do?

Motivation

More

For Moderators

Additional Notes

askulkarni2 left a comment

Choose a reason for hiding this comment

vara-bonthu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vara-bonthu Apr 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcoleman731 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vara-bonthu Apr 16, 2022 • edited

Choose a reason for hiding this comment

vara-bonthu left a comment • edited

Choose a reason for hiding this comment

bobdoah commented Apr 19, 2022

ZeroDeth commented Apr 20, 2022 • edited

bobdoah commented Apr 20, 2022

bobdoah commented Apr 21, 2022

vara-bonthu commented Apr 21, 2022

ZeroDeth commented Apr 21, 2022 • edited

bobdoah commented Apr 22, 2022

ZeroDeth commented Apr 23, 2022 • edited

ZeroDeth commented Apr 23, 2022

bobdoah commented Apr 11, 2022 •

edited

vara-bonthu Apr 13, 2022 •

edited

vara-bonthu Apr 16, 2022 •

edited

vara-bonthu left a comment •

edited

ZeroDeth commented Apr 20, 2022 •

edited

ZeroDeth commented Apr 21, 2022 •

edited

ZeroDeth commented Apr 23, 2022 •

edited