Chart: Fix appProtocol semver comparisons.

[karolkieglerski]

What this PR does / why we need it:

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

Installed on GKE and AKS clusters and now configuration work as expected.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: karolkieglerski / name: Karol Kieglerski (150cc14)

[k8s-ci-robot]

Welcome @karolkieglerski!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @karolkieglerski. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	150cc14
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/660e656bd37494000847fd83

[strongjz]

@karolkieglerski can you please sign the CLA?

To fix the CI issue, run helm docs.

 go install github.com/norwoodj/helm-docs/cmd/[email protected]
 helm-docs --chart-search-root=${PATH_TO_INGRESS_CHART}/charts

[strongjz]

/ok-to-test
/priority backlog
/triage accepted

[ubergesundheit]

I'm a little bit torn about this contribution. It seems a little unfortunate that GKE uses uppercase appProtocol values thus making the chart not work as intended.

One one hand it seems easy enough to switch the appProtocol values to uppercase once users set the right cloudProvider but I think design wise the approach is a little bit flawed.

IMO its not obvious for GKE users that they need to set the chart value cloudProvider and others don't. Without searching in the chart templates its also not obvious that this only exists to switch the case of the appProtocol values and nothing else.

I am curious if other cloud providers also would work with uppercase appProtocol values. Could you test this @karolkieglerski ?

[karolkieglerski]

This was tested, other clouds use lowercase. Only Google require uppercase.

https://cloud.google.com/kubernetes-engine/docs/how-to/secure-gateway

The appProtocol field in the Service configuration only accepts capital letters for the protocol value (HTTP, HTTPS or HTTP2). Using lowercase letters results in using HTTP as the protocol with the backends.

This fix is reference to this bugs:
#8271
https://gitlab.com/gitlab-org/charts/gitlab/-/issues/4937

I'm agree that it isn't easy to find and understand that. We spend a couple of time to find and fix it.
Solution which I propose is working on AWS, Azure and Google.

[Gacko]

We already have two other PRs about that:

• Chart: Configure appProtocol case. #10214
• Chart: Implement HTTP2 appProtocol. #10171

Both of them are not really the best way to solve that. We should rather make the app protocol configurable as a string per port but that would need some breaking changes in the values schema.

I'd rather not implement any provider-specific stuff here and keep that project provider-agnostic.

[Gacko]

/hold
/assign

[Gacko]

Also I agree on using this PR to just fix the semver bug, but please do not change the app protocol itself for now.

[Gacko]

/retitle Chart: Fix appProtocol semver comparisons.

[Gacko]

/kind bug

[Gacko]

Unfortunately Helm Unit Test does not cover the full Kubernetes version string to be configured per test suite, only major and minor versions: https://github.com/helm-unittest/helm-unittest/blob/main/DOCUMENT.md#test-suite. So you can safely skip the unit test implementation for that... 🙄

[Gacko]

Thanks for updating your PR! I think we can get that merged asap. I'll also push forward on the other PRs and create a new one for properly fixing the app protocol setup.

[Gacko]

/unhold
/lgtm
/cherry-pick release-1.10

[k8s-infra-cherrypick-robot]

@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

In response to this:

/unhold
/lgtm
/cherry-pick release-1.10

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.

[ubergesundheit]

/lgtm

thanks for your contribution

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, karolkieglerski, ubergesundheit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/ingress-nginx/OWNERS [Gacko,ubergesundheit]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Gacko]

CI isn't failing because of your change, it's because of some issue with node ports being re-used / already allocated. Trying to rerun the tests.

[Gacko]

/retest

[Gacko]

/test

[k8s-ci-robot]

@Gacko: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

• /test pull-ingress-nginx-boilerplate
• /test pull-ingress-nginx-codegen
• /test pull-ingress-nginx-lualint
• /test pull-ingress-nginx-test-lua

In response to this:

/test

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.

[Gacko]

/test all

[k8s-ci-robot]

@Gacko: No jobs can be run with /test all.
The following commands are available to trigger required jobs:

• /test pull-ingress-nginx-boilerplate
• /test pull-ingress-nginx-codegen
• /test pull-ingress-nginx-lualint
• /test pull-ingress-nginx-test-lua

In response to this:

/test all

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.

[Gacko]

@strongjz @rikatz Can you re-run the failed tests? Guess CI Bot can't do that.

[k8s-infra-cherrypick-robot]

@Gacko: new pull request created: #11199

In response to this:

/unhold
/lgtm
/cherry-pick release-1.10

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.