search

kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.52k stars 8.26k forks source link

Termination of TLS in Load Balancer with Helm chart config #8017

Closed brsolomon-deloitte closed 6 months ago

brsolomon-deloitte commented 2 years ago

https://kubernetes.github.io/ingress-nginx/deploy/#aws shows the direct application of a YAML manifest using kubectl apply -f. It is confusing / unspecified whether applying this file will function as a patch on existing resources that have been deployed with the Helm chart.

What is the suggested workflow for configuring termination of TLS in the Load Balancer, when the ingress-nginx Helm chart is used? What does an MCVE config set of values for Helm look like?

There are several scattered threads such as https://github.com/kubernetes/ingress-nginx/issues/1624#issuecomment-342680491 that allude to this, but https://github.com/kubernetes/ingress-nginx/pull/5374 and https://github.com/kubernetes/ingress-nginx/pull/5360 contain very little prescriptive guidance on the proper configuration with the ingress-nginx Helm chart.

Looking at https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.1.0/deploy/static/provider/aws/deploy-tls-termination.yaml, it looks like that has been generated with something like helm template in the first place, so it would be useful to see what inputs resulted in that output.

brsolomon-deloitte commented 2 years ago

There is also no mention of the change to a port named tohttps on 2443, which seems like it must be specified in .Values.controller.containerPort.

brsolomon-deloitte commented 2 years ago

Edit: Just a little scary to see it is actually done through a shell script

longwuyuan commented 2 years ago

Recommend https://kubernetes.github.io/ingress-nginx/deploy/#tls-termination-in-aws-load-balancer-nlb

/remove-kind bug /kind documentation /area docs

brsolomon-deloitte commented 2 years ago

@longwuyuan your reply includes the first link used in my original question, which is the very part of the documentation that this issue is about in the first place.

longwuyuan commented 2 years ago

Need some clarification here.

These words 

It is confusing / unspecified whether applying this file will function as a patch on existing resources that have been deployed with the Helm chart.

seem to imply, that there are some possibilities, of that yaml file, being a patch of sorts, over and above and after a helm based installation of the ingress-nginx controller ?

If I am reading it wrong, I apologise. But if otherwise, then just want to state the obvious that installing using helm and installing using the yaml file are independent alternative ways to install the ingress-nginx controller and there is no patching of resources.

You can run helm template against the chart and sort of get a view of the manifests for the resources involved in running the ingress-controller. And you can peruse the yaml file I linked also to get a view of the resources involved in running the ingress-controller.

Hope there is a solution to this issue soon.

brsolomon-deloitte commented 2 years ago

seem to imply, that there are some possibilities, of that yaml file, being a patch of sorts, over and above and after a helm based installation of the ingress-nginx controller ?

Correct. So I now understand that it is not intended to function as an idempotent patch. Thank you for clarifying.

Considering that information, it seems like the suggested approach is not idiomatic and falls victim to several anti-patterns:

longwuyuan commented 2 years ago

The recommendation begins and ends with that yaml. That's the curated part and it works.

Users with insight opt to not use that curated yaml. I don't see ambiguity yet but please feel free to submit a PR.

Thanks, ; Long

On Tue, 7 Dec, 2021, 8:37 PM Brad Solomon, @.***> wrote:

seem to imply, that there are some possibilities, of that yaml file, being a patch of sorts, over and above and after a helm based installation of the ingress-nginx controller ?

Correct. So understood that it is not intended to function as an idempotent patch. Thank you for clarifying.

Considering that information, it seems like the suggested approach is not idiomatic and falls victim to several anti-patterns:

  • Using the already-templated deploy-tls-termination.yaml is incompatible with an existing Helm-based deployment
  • The docs suggest / imply that I should manually find/replace a couple of specific parameters in the template such as "XX.XX.XX.XX/XX". This is not the programatic way that I would hope to use Kubernetes/Helm
  • The docs leave out mention entirely of what Helm chart parameters would be required to achieve this same effect - namely, those found halfway down a shell script https://github.com/kubernetes/ingress-nginx/blob/987a721723d9a7849aa25a40e48bd6cad5ac2dc7/hack/generate-deploy-scripts.sh that itself is not mentioned at all in the docs

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8017#issuecomment-988012840, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWSYE4ZVU2BMQ6I2AEDUPYPMLANCNFSM5JP2DUZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

gijsdpg commented 2 years ago

I'm also confused by the structure of the manual. I'm also trying to set up TLS termination in AWS load balancer, but currently, the documentation seems to read as if the only option is downloading and editing a YAML file.

gijsdpg commented 2 years ago

Ok, I think I figured it out. I can replicate the deploy-tls-termination.yaml configuration with helm with the following procedure:

$ helm repo add ingress-nginx https://kubernetes.github.io/ingress-nginx
$ helm repo update
$ helm template -n ingress-nginx --create-namespace ingress-nginx -f values.yaml ingress-nginx/ingress-nginx manifest.yaml

where values.yaml is a file containing:

controller:
  service:
    externalTrafficPolicy: "Local"
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout: '60'
      service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: 'true'
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:us-west-2:XXXXXXXX:certificate/XXXXXX-XXXXXXX-XXXXXXX-XXXXXXXX
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: https
      service.beta.kubernetes.io/aws-load-balancer-type: nlb
  config:
    http-snippet: |
      server{
        listen 2443;
        return 308 https://$host$request_uri;
      }
    proxy-real-ip-cidr: XXX.XXX.XXX/XX
    use-forwarded-headers: 'true'
    type: "LoadBalancer"
    ports:
      http:  80
      https: 443

    targetPorts:
      http: tohttps
      https: http

you can use the values file to set your appropriate values.

brsolomon-deloitte commented 2 years ago

@gijsdpg if you check out the shell script that is used to generate deploy-tls-termination.yaml, you will see a few very slight differences:

controller:
  service:
    type: LoadBalancer
    externalTrafficPolicy: Local
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: "true"
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "https"
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: "arn:aws:acm:us-west-2:XXXXXXXX:certificate/XXXXXX-XXXXXXX-XXXXXXX-XXXXXXXX"
      service.beta.kubernetes.io/aws-load-balancer-type: nlb
      service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout: "60"
    targetPorts:
      http: tohttps
      https: http
  # Configures the ports the nginx-controller listens on
  containerPort:
    http: 80
    https: 80
    tohttps: 2443
  config:
    proxy-real-ip-cidr: XXX.XXX.XXX/XX
    use-forwarded-headers: "true"
    http-snippet: |
      server {
        listen 2443;
        return 308 https://\$host\$request_uri;
      }

(Note the backslashes are only there because they are used in a Bash heredoc and should go away if you are editing YAML directly.)

What threw us off is the need to use a NLB rather than ALB for this.

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle stale

k8s-triage-robot commented 2 years ago

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

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

You can:

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

/lifecycle rotten

mtb-xt commented 2 years ago

/remove-lifecycle rotten

dwelch2344 commented 2 years ago

Ok, I think I figured it out. I can replicate the deploy-tls-termination.yaml configuration with helm with the following procedure:

$ helm repo add ingress-nginx https://kubernetes.github.io/ingress-nginx
$ helm repo update
$ helm template -n ingress-nginx --create-namespace ingress-nginx -f values.yaml ingress-nginx/ingress-nginx manifest.yaml

where values.yaml is a file containing:

controller:
  service:
    externalTrafficPolicy: "Local"
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout: '60'
      service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: 'true'
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:us-west-2:XXXXXXXX:certificate/XXXXXX-XXXXXXX-XXXXXXX-XXXXXXXX
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: https
      service.beta.kubernetes.io/aws-load-balancer-type: nlb
  config:
    http-snippet: |
      server{
        listen 2443;
        return 308 https://$host$request_uri;
      }
    proxy-real-ip-cidr: XXX.XXX.XXX/XX
    use-forwarded-headers: 'true'
    type: "LoadBalancer"
    ports:
      http:  80
      https: 443

    targetPorts:
      http: tohttps
      https: http

you can use the values file to set your appropriate values.

THANK YOU! I've been wrestling this all night (complicated by the fact we needed ARM64 support, which slightly breaks the manual install methods) and this finally got us there :)

alanlekah commented 2 years ago

I don't understand why a working version of SSL termination with helm was removed from the README during the migration from the old repo. This is the only thing that I got working:

https://github.com/helm/charts/blob/master/stable/nginx-ingress/README.md#aws-l4-nlb-with-ssl-redirection

  config:
    ssl-redirect: "false" # we use `special` port to control ssl redirection
    server-snippet: |
      listen 8000;
      if ( $server_port = 80 ) {
         return 308 https://$host$request_uri;
      }
  containerPort:
    http: 80
    https: 443
    special: 8000
  service:
    targetPorts:
      http: http
      https: special
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "tcp"
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: "your-arn"
      service.beta.kubernetes.io/aws-load-balancer-type: "nlb"

and installed with: helm upgrade --install -n ingress-nginx --create-namespace ingress-nginx -f values.yaml ingress-nginx/ingress-nginx

The current docs with L7 and SSL termination did not work, only the above. With the L7 solution in the current README, I was only led to more 308 redirects. Hope this helps someone else that got stuck.

longwuyuan commented 2 years ago
alanlekah commented 2 years ago
longwuyuan commented 2 years ago

@alanlekah now someone has to test it and so involves aws account and don't know if free tier works etc. But what is critical now is ;

Once again thank you very much for reporting this and thank you very much for helping fix the doc

I will take your word and label the issue as well as your PR accordingly. But I still don't understand, what precisely is broken in the current NLB Termination doc. If you could describe that in a way that any reader can understand, it will help the much needed stabilization work in progress, for the project

/triage accepted /area stabilization /area docs /priority important-soon /kind bug /assign @alanlekah

longwuyuan commented 2 years ago

@tao12345666333 @rikatz @strongjz this latest comment by @alanlekah says our deployment doc for AWS is broken. Do we just use our own personal accounts to test on AWS ?

alanlekah commented 2 years ago

@longwuyuan I'm not suggesting its the deployment doc thats broken. I may be suggesting that the deploy.yml at:

https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.3.0/deploy/static/provider/aws/nlb-with-tls-termination/deploy.yaml

may still have issues. I'm not terribly familiar with what the difference is between the helm chart that generates the working raw yaml and the file at the link above but here's what I noticed and what worked for me:

I generated the working yaml with the following helm template command: helm template -n ingress-nginx --create-namespace ingress-nginx -f values.yaml ingress-nginx/ingress-nginx

The problem:

I created a simple API with a helm deployment listening on port 5000. The helm chart created an ingress with the nginx annotations and a ClusterIP service (also on port 5000 for each). When using the installation guide AS-IS, I get a 308 permanent redirect error in postman.

When using the values.yaml and the helm template using the values.yaml mentioned in the comment above (https://github.com/kubernetes/ingress-nginx/issues/8017#issuecomment-1226542186), I had no issues accessing the endpoint in HTTPS and HTTP redirect was working correctly as well. I have no idea why.

I'm going to test each of those changes in the file listed in the main yml in this repo and see which one actually fixes the issue.

rikatz commented 2 years ago

I know this may be cliche, but I don't have access or ideas on how AWS works. Can someone jump into our next call to show the problem? Maybe we can discuss what can be done there :)

alanlekah commented 2 years ago

I know this may be cliche, but I don't have access or ideas on how AWS works. Can someone jump into our next call to show the problem? Maybe we can discuss what can be done there :)

Frankly I've tested all the combinations of changes that i've made from this values.yaml applied against the stock deploy.yml and don't know where the issue is in the file from the repo. I just know that what helm generates works.

alanlekah commented 2 years ago

Attaching the working yml for comparison ( against the non-working one )

deploy.yml.zip

longwuyuan commented 2 years ago

With the latest updates from @alanlekah I think we some idea of the next action item. We need to diff the yaml that works for him and the manifest we publish. Maybe we paid attention to the vanilla aws manifest and messed up the nlb-termination one, as it is in a subdir.

@alanlekah that descriptive update from you was helpful as as I was typing this, I saw you posted the manifest you used. thanks. Please just clarify this. You said you did not use the proxy-real-ip-cidr and you also said you replaced that snippet. I want to confirm that first you generated a yaml with helm template and then you did these actions of deleting proxy-real-ip-cidr annotations & modifying snippet etc. in that file you generated.

alanlekah commented 2 years ago

Something seems very off. For example: https://github.com/kubernetes/ingress-nginx/blob/main/deploy/static/provider/aws/nlb-with-tls-termination/deploy.yaml#L473

See this section in controller-v1.3.0/deploy/static/provider/aws/nlb-with-tls-termination/deploy.yaml:

        ports:
        - containerPort: 80
          name: http
          protocol: TCP
        - containerPort: 80
          name: https
          protocol: TCP

Looks like that was fixed in the parent one but not in the TLS termination yaml?

To create the working yaml, I only did the following (I did not replace or add anything). I created the values.yaml:

controller:
  config:
    ssl-redirect: "false" # we use `special` port to control ssl redirection
    server-snippet: |
      listen 8000;
      if ( $server_port = 80 ) {
         return 308 https://$host$request_uri;
      }
  containerPort:
    http: 80
    https: 443
    special: 8000
  service:
    targetPorts:
      http: http
      https: special
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "tcp"
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: "<arn>"
      service.beta.kubernetes.io/aws-load-balancer-type: "nlb"

helm upgrade --install -n ingress-nginx --create-namespace ingress-nginx -f values.yaml ingress-nginx/ingress-nginx

That comparison was just a rough diff I did on the files.

longwuyuan commented 2 years ago

i was about to post the diff so thanks, that is a great catch.

This confirms that our generate-deploy-script needs improvement. I suspect the root cause is the subdir and the script traverses parent dirs and not subdirs.

We will fix it. Thanks again for reporting this and if you find other differences, please let us know.

@Volatus when you get a chance, can you please ping me on slack about this. I think we missed traversing the subdir in generate-deploy-script because aws is a unique case in the sense of having a subdir. We can talk about stuff like making 2 providers of aws or adding a special if condition for aws provider etc.

longwuyuan commented 2 years ago

@alanlekah Does port 8443 ring any bells ?

I don't recall seeing port 8000 before but must have missed things so just asking. We will check anyways.

alanlekah commented 2 years ago

@longwuyuan Yes it does. Port 8000 is something I changed (from 2443). I'm not sure if it makes any difference though.

See the old docs for nginx.. https://github.com/helm/charts/tree/master/stable/nginx-ingress#aws-l4-nlb-with-ssl-redirection

8443 is the webhook one AFAIK. I believe the special: 8000 is the same as the current tohttps: 2443 that's in the repo and serves the same purpose.

longwuyuan commented 2 years ago

ok. thanks. once again good pointer on the old doc. the generate-deploy-script went through 3-4 iterations so using that history will be a rabbit hole. I think we are better off basing our fix on the old doc for sanity sake.

alanlekah commented 2 years ago

Something interesting is that turning the helm values.yml file into this also works without issue, so it doesn't make a difference on the name or port number (seems like):

controller:
  config:
    ssl-redirect: "false" # we use `special` port to control ssl redirection
    server-snippet: |
      listen 2443;
      if ( $server_port = 80 ) {
         return 308 https://$host$request_uri;
      }
  containerPort:
    http: 80
    https: 443
    tohttps: 2443
  service:
    targetPorts:
      http: http
      https: tohttps
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "tcp"
      service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
      service.beta.kubernetes.io/aws-load-balancer-ssl-cert: "<arn>"
      service.beta.kubernetes.io/aws-load-balancer-type: "nlb"
longwuyuan commented 2 years ago

that is pretty useful info. saves testing that aspect at least. thank you very much. the task now is to instrument that automated generation of that manifest with these good configs we are learning here.

alanlekah commented 2 years ago

Yep. That tohttps is basically already what you have in the parent one. Interestingly, if you change this:

server-snippet: |
      listen 2443;
      if ( $server_port = 80 ) {
         return 308 https://$host$request_uri;
      }

to:

server-snippet: |
      listen 2443;
      return 308 https://$host$request_uri;

the 308 permanent redirect issue returns. Hopefully that helps narrow it down even more.

Edit: The issue being: you get stuck in a 308 loop when calling a service thats using nginx: Exceeded maxRedirects. Probably stuck in a redirect loop

longwuyuan commented 2 years ago

got it. can't do 80 so instrumented another port but 308 should only be sent one time, the first time (or something like that)

alanlekah commented 2 years ago

I believe it has to be 80 since its an issue with forwarding HTTP to HTTPS traffic. I was under the impression thats why the port 80 check was one part of the fix.

k8s-triage-robot commented 1 year ago

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:

You can:

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

/lifecycle stale

StepanKuksenko commented 1 year ago

@alanlekah could you explain please why we can't use just this ?

if ( $server_port = 80 ) {
   return 308 https://$host$request_uri;
}

why we even need 2443 port ? also why we need this 

service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "tcp"
service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"

instead of 

service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "http"
service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "https"
alanlekah commented 1 year ago

@StepanKuksenko I'm not sure why nginx would use the 2443 port in the deploy.yml from here which is part of the install doc for AWS NLB installation here

From master, this doc has the correct working settings.

I tried the following:

service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "http"
service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "https"

without issues as well.

k8s-triage-robot commented 1 year ago

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

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

/remove-triage accepted

longwuyuan commented 6 months ago

From reports on other AWS related issues, it seems that the only change that is not documented in this project is the use of the new AWS-LoadBalancer-Controller being a requirement.

While it will be of interest to mention that in the deployment docs of this project explicitly, its also assumed that the users of AWS know that the requirement of installating the AWS-LoadBalancer-Controller is a pre-requisite for creating NLBs.

SInce there is no traction on this issue and there is a permanent-open-issue about improvements possible to docs, I will close this issue for now. If the original reporter of this issue wants to discuss a docs PR or finds and posts data related to this issue that lead to a problem to be solved in the controller, please re-open the issue. There are too many inactive issues open and hence cluttering the project management.

/close

k8s-ci-robot commented 6 months ago

@longwuyuan: Closing this issue.

In response to [this](https://github.com/kubernetes/ingress-nginx/issues/8017#issuecomment-2084661529): >From reports on other AWS related issues, it seems that the only change that is not documented in this project is the use of the new AWS-LoadBalancer-Controller being a requirement. > >While it will be of interest to mention that in the deployment docs of this project explicitly, its also assumed that the users of AWS know that the requirement of installating the AWS-LoadBalancer-Controller is a pre-requisite for creating NLBs. > >SInce there is no traction on this issue and there is a permanent-open-issue about improvements possible to docs, I will close this issue for now. If the original reporter of this issue wants to discuss a docs PR or finds and posts data related to this issue that lead to a problem to be solved in the controller, please re-open the issue. There are too many inactive issues open and hence cluttering the project management. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
jbarrio commented 5 days ago

Hello gents,

I've tested different solutions from here and there and after a lot of back and forth, I am just doing these changes:

            service:
              loadBalancerClass: service.k8s.aws/nlb
              ports:
                http: 80
                https: 443
              targetPorts:
                http: 80
                https: 80
              annotations:
                [...]
                service.beta.kubernetes.io/aws-load-balancer-ssl-cert: "ARNs"
                service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
                service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "http"

So basically registering HTTP on the TLS listener does it, without needing any redirect or special port block.