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

Update configuration on add-headers and proxy-set-headers ConfigMap change #5238

Open
hobti01 opened this issue Mar 11, 2020 · 28 comments
Open
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@hobti01
Copy link
Contributor

hobti01 commented Mar 11, 2020

When the ConfigMap specified in add-headers or proxy-set-headers is modified, the nginx configuration should be updated.

Current behavior is to not update nginx configuration when the referenced ConfigMap is modified. This leads to unexpected results when headers are added/modified in the ConfigMap and no change is realized in the running instances. Restarting instances results in a reload of the configuration, but this is not desired.

Previously raised in #3868

/kind feature

@hobti01 hobti01 added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 11, 2020
@aledbf aledbf self-assigned this Mar 11, 2020
@grifx
Copy link
Contributor

grifx commented Apr 19, 2020

Thanks for raising this issue!

What's the appropriate way to handle those kind of issues in a production environment?
Is there a way to manually trigger a configuration build and reload on all the nginx-ingress-controller without restarting the pods?

Thank you!

@longwuyuan
Copy link
Contributor

longwuyuan commented Apr 19, 2020

Does it happen even if you use helm upgrade with a --set for

$ helm inspect all stable/nginx-ingress | grep -i proxySetHeader     
  proxySetHeaders: {}
`controller.proxySetHeaders` | configMap key:value pairs containing [custom headers](https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#proxy-set-headers) added before sending request to the backends| `{}`
 

@hobti01
Copy link
Contributor Author

hobti01 commented Apr 19, 2020

It does not matter how you change the configmap. The functionality does not exist to re-read the configmap. This is a known situation as described in the previous issue where @aledbf requested a new issue (this one). The pods can be restarted, but this feature is to remove that limitation.

Looking forward to 0.31.0!

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 18, 2020
@kmindi
Copy link

kmindi commented Jul 18, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 18, 2020
@ppanyukov
Copy link

Guys, what is the workaround for this? I'm hitting this at the moment, and the only way to effect the change is to restart pods. Which is kinda not cool for production, yes?

@ppanyukov
Copy link

OK for those using terraform, I found a workaround. The idea is run this command if the values of add-headers changed:

kubectl --namespace ingress rollout restart deployment nginx-ingress-controller

Now in Terraform, the hack is like this:

First, declare headers a s var.

locals {
  ingress_add_headers_yaml = jsonencode({
    "X-Cluster" = "${local.cluster_dns_fqdn}"
    "X-Test" = "this is test 4"
  })
}

Then do the regular helm thing:

resource "helm_release" "ingress" {
  name       = "nginx-ingress"
  namespace  = kubernetes_namespace.ingress.metadata[0].name
  repository = "https://kubernetes-charts.storage.googleapis.com"
  chart      = "nginx-ingress"
  version    = "1.41.2"

  # Use addHeaders defined in a var
  values = [
    <<-EOF
    controller:
      addHeaders: ${local.ingress_add_headers_yaml}
    EOF
  ]
}

And finally, do a shell exec to reload things. Note that this will only run if there are changes in the header.

resource "null_resource" "ingress-reload-changes-in-add-headers" {
  depends_on = [
    helm_release.ingress,
  ]

  triggers = {
    run_if_changed       = sha256(local.ingress_add_headers_yaml)
    cluster_context_name = local.cluster_context_name
    comment              = "it is safe to replace this every time in terraform"
    ingress_add_headers_yaml = local.ingress_add_headers_yaml
    ingress_namespace = kubernetes_namespace.ingress.metadata[0].name
  }

  provisioner "local-exec" {
    # kubectl --namespace ingress rollout restart deployment nginx-ingress-controller
    command = "kubectl --context '${self.triggers.cluster_context_name}' --namespace '${self.triggers.ingress_namespace}' rollout restart deployment nginx-ingress-controller"
  }
}

Not ideal but kinda works.

And it would be nice to have config reload as part of the core functionality so people don't have to hack around it.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 3, 2020
@kmindi
Copy link

kmindi commented Nov 4, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 4, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 2, 2021
@robertvanhoesel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 2, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2021
@robertvanhoesel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 18, 2021
@rohitrav33ndran
Copy link

I am having this problem now. I add headers to configmap and the pod restart doesn't update the changes to nginx.conf. Another observation is when multiple add_header statement to configmap and only one is retained on saving the changes.

@strongjz
Copy link
Member

@rohitagarwal003 and @hobti01 what version of k8s and inigress-nginx are you using? Does this issue still exist in v0.48.1?

Thanks

/triage needs-information
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/needs-information Indicates an issue needs more information in order to work on it. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Jul 17, 2021
@hobti01
Copy link
Contributor Author

hobti01 commented Jul 20, 2021

Hi @strongjz , please see the discussion in the PR that was to implement code for this and was not implemented #4030

@aledbf requested that I open a new issue (this one) to capture the result of the previous issue (enhancement request) that has a PR that was not merged.

I'm not aware that the logic has ever been implemented, therefore it would never have been working. Therefore... the versions are irrelevant unless you can point to an implementation :)

@rikatz
Copy link
Contributor

rikatz commented Jul 20, 2021

Hi,

As stated in #4030 the implementation should be more simple than that, but we have no intent right now of prioritizing this due to:

  • Priority to release ingress-nginx v1 dropping API v1beta1 support
  • Fixing long standing time bugs that impact in Ingress performance

I'm open to reviewing some simpler implementation, but right now I don't think we can deal with this, so the issue will be a backlog for medium to long term.

@iamNoah1
Copy link
Contributor

iamNoah1 commented Nov 9, 2021

/help

@k8s-ci-robot
Copy link
Contributor

@iamNoah1:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 9, 2021
@iamNoah1
Copy link
Contributor

/triage-accepted

@iamNoah1
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Dec 15, 2021
jsoref added a commit to jsoref/ingress-nginx that referenced this issue Jan 12, 2022
k8s-ci-robot pushed a commit that referenced this issue Jan 17, 2022
* clarify link

* Add section headers

* console blocks

* grpc example json was not valid

* multi-tls update text

The preceding point 1 related to https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/ingress.yaml
and the deployments referenced in https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/README.md

They are not relevant to the current instructions.

* add whitespace around parens

* grammar

setup would be a proper noun, but it is not the intended concept, which is a state

* grammar

* is-only
* via

* Use bullets for choices

* ingress-controller

nginx is a distinct brand.

generally this repo talks about ingress-controller, although it is quite inconsistent about how...

* drop stray paren

* OAuth is a brand and needs an article here

also GitHub is a brand

* Indent text under numbered lists

* use e.g.

* Document that customer header config maps changes do not trigger updates

This should be removed if
#5238
is fixed.

* article

* period

* infinitive verb + period

* clarify that the gRPC server is responsible for listening for TCP traffic and not some other part of the backend application

* avoid using ; and reword

* whitespace

* brand: gRPC

* only-does is the right form

`for` adds nothing here

* spelling: GitHub

* punctuation

`;` is generally not the right punctuation...

* drop stray `to`

* sentence

* backticks

* fix link

* Improve readability of compare/vs

* Renumber list

* punctuation

* Favor Ingress-NGINX and Ingress NGINX

* Simplify custom header restart text

* Undo typo damage

Co-authored-by: Josh Soref <[email protected]>
@ppanyukov
Copy link

Bump bump. Almost celebrating a 2 year anniversary -- time flies in lockdown! 😀 Would be nice to have a "fix" at some point. 😀

@alokhom
Copy link

alokhom commented May 6, 2022

maybe you want to delete the one pod and check the changes

@iamNoah1
Copy link
Contributor

@alokhom @ppanyukov we are super happy for any contributions :)

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 12, 2022
@hobti01
Copy link
Contributor Author

hobti01 commented Sep 12, 2022

I notice that this is labeled triage/needs-information but it seems that the enhancement is sufficiently described and understood. I think there is a general expectation that all config sources will be re-read when changed. Unfortunately when additional sources (configmaps) are used, this is not the result. While it's great to see the limitation documented, the feature would be even more appreciated. Unfortunately our team hasn't had the capacity to contribute a proposed code change.

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 12, 2022
rchshld pushed a commit to joomcode/ingress-nginx that referenced this issue May 19, 2023
* clarify link

* Add section headers

* console blocks

* grpc example json was not valid

* multi-tls update text

The preceding point 1 related to https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/ingress.yaml
and the deployments referenced in https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/README.md

They are not relevant to the current instructions.

* add whitespace around parens

* grammar

setup would be a proper noun, but it is not the intended concept, which is a state

* grammar

* is-only
* via

* Use bullets for choices

* ingress-controller

nginx is a distinct brand.

generally this repo talks about ingress-controller, although it is quite inconsistent about how...

* drop stray paren

* OAuth is a brand and needs an article here

also GitHub is a brand

* Indent text under numbered lists

* use e.g.

* Document that customer header config maps changes do not trigger updates

This should be removed if
kubernetes#5238
is fixed.

* article

* period

* infinitive verb + period

* clarify that the gRPC server is responsible for listening for TCP traffic and not some other part of the backend application

* avoid using ; and reword

* whitespace

* brand: gRPC

* only-does is the right form

`for` adds nothing here

* spelling: GitHub

* punctuation

`;` is generally not the right punctuation...

* drop stray `to`

* sentence

* backticks

* fix link

* Improve readability of compare/vs

* Renumber list

* punctuation

* Favor Ingress-NGINX and Ingress NGINX

* Simplify custom header restart text

* Undo typo damage

Co-authored-by: Josh Soref <[email protected]>
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jan 18, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests