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

[FEATURE] Allow namespaces to be over-ridden in all addons (prometheus and others) #498

Closed
spkane opened this issue May 3, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@spkane
Copy link
Contributor

spkane commented May 3, 2022

Please describe your quesiton here

The namespace for the prometheus addon is hardcoded. Why is this?

I have spent a bit of time trying to change this, before realizing why I couldn't get it to change as expected.

Provide link to the example related to the question

Additional context

Similar issue/fix: #287

More

  • [X ] Yes, I have checked the repo for existing issues before raising this question
@spkane spkane added the question Further information is requested label May 3, 2022
@bryantbiggs bryantbiggs added enhancement New feature or request addon and removed question Further information is requested labels May 3, 2022
@bryantbiggs
Copy link
Contributor

I can't answer the why but I think instead we should default to what the associated addon's helm chart provides while offering the users the ability to change to suite. Changing this from a question to an enhancement

@spkane
Copy link
Contributor Author

spkane commented May 3, 2022

Agreed. I just wasn't sure if there was a reason for this, so I figured I'd start by asking.

@spkane spkane changed the title [QUESTION] Why is the namespace hardcoded in the prometheus addon (and maybe others)? [FEATURE] Allow namespaces to be over-ridden in all addons (prometheus and others) May 3, 2022
@vara-bonthu
Copy link
Contributor

@spkane What you are seeing in locals.tf are the default values for each Add-on. All the Add-ons can be configured by the users to pass their own values.

Did you try passing your own config with prometheus_helm_config={} ?

Here is the doc https://aws-ia.github.io/terraform-aws-eks-blueprints/add-ons/prometheus/

@spkane
Copy link
Contributor Author

spkane commented May 13, 2022

@vara-bonthu I had this setup in my code when I was testing it:

  enable_prometheus                   = true
  prometheus_helm_config = {
    version          = "15.8.5"
    create_namespace = true
    namespace        = "monitoring"
    values           = [templatefile("${path.module}/helm_values/prometheus-values.yaml.tftpl", {nodeSelector = local.primaryNodeSelector})]
  }

but since it has been a week, let me test it one more time and double check.

The values template looks like this:

# See:
# https://github.com/prometheus-community/helm-charts/blob/main/charts/prometheus/values.yaml
#
# This is a template, so we can get values from terraform.
#

alertmanager:
  nodeSelector: ${jsonencode({ for ns_key, ns_value in nodeSelector : ns_key => ns_value })}

server:
  nodeSelector: ${jsonencode({ for ns_key, ns_value in nodeSelector : ns_key => ns_value })}
  retention: 1h

  resources:
    requests:
      cpu: 500m
      memory: 512Mi

  global:
    scrape_interval: 15s

pushgateway:
  nodeSelector: ${jsonencode({ for ns_key, ns_value in nodeSelector : ns_key => ns_value })}

@spkane
Copy link
Contributor Author

spkane commented May 13, 2022

So, it looks like the monitoring namespace is created, but the ArgoCD application/helm is not targeting the right namespace.

CleanShot 2022-05-13 at 08 27 07

Digging in just a bit more...

@spkane
Copy link
Contributor Author

spkane commented May 13, 2022

Ahhh. I think the error might actually be here:

https://github.com/aws-samples/eks-blueprints-add-ons/blob/main/chart/templates/prometheus.yaml#L33

It looks like the destination namespace is hard-coded here (and maybe in other examples) and does not respect what was set in the Terraform code.

@vara-bonthu
Copy link
Contributor

Ahhh. I think the error might actually be here:

https://github.com/aws-samples/eks-blueprints-add-ons/blob/main/chart/templates/prometheus.yaml#L33

It looks like the destination namespace is hard-coded here (and maybe in other examples) and does not respect what was set in the Terraform code.

Yes, you are spot on here. Fix needs to go in this repo and the eks-blueprints-add-ons to pass the namespace to all the Argo enabled add-ons

@bryantbiggs
Copy link
Contributor

Resolved here with #595

Similar issue exists for addons repo aws-samples/eks-blueprints-add-ons#43

@bryantbiggs bryantbiggs moved this from Prioritized to Done in Old EKS Blueprints (Archived) Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

3 participants