semverCompare conditional is incorrect

[45cali]

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

k exec -it ingress-nginx-controller-54f97dfdc-dkrkh --  /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.1.1
  Build:         a17181e43ec85534a6fea968d95d019c5a4bc8cf
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.19.9

-------------------------------------------------------------------------------

Kubernetes version (use kubectl version):

k version
Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.4", GitCommit:"e6c093d87ea4cbb530a7b2ae91e54c0842d8308a", GitTreeState:"clean", BuildDate:"2022-02-16T12:30:48Z", GoVersion:"go1.17.6", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.11-eks-f17b81", GitCommit:"f17b810c9e5a82200d28b6210b458497ddfcf31b", GitTreeState:"clean", BuildDate:"2021-10-15T21:46:21Z", GoVersion:"go1.15.15", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.23) and server (1.20) exceeds the supported minor version skew of +/-1

Environment:

• OS : EKS

• How was the ingress-nginx-controller installed:

  • Helm

  helm list
  NAME         	NAMESPACE    	REVISION	UPDATED                             	STATUS  	CHART               	APP VERSION
  ingress-nginx	ingress-nginx	1       	2022-02-25 14:53:30.943665 -0800 PST	deployed	ingress-nginx-4.0.16	1.1.1
  

• Current State of the controller:

• Current state of ingress object, if applicable:

• Others:

What happened:

Helm chart does not apply appProtocol in the service template if version is v1.20.11-eks-f17b81 What you expected to happen:

add appProtocol

I believe that this can be fixed by changing the conditional

     port: {{ .Values.controller.service.ports.http }}
       protocol: TCP
       targetPort: {{ .Values.controller.service.targetPorts.http }}
-    {{- if and (semverCompare ">=1.20" .Capabilities.KubeVersion.Version) (.Values.controller.service.appProtocol) }}
+    {{- if and (semverCompare ">=1.20.0-0" .Capabilities.KubeVersion.Version) (.Values.controller.service.appProtocol) }}
       appProtocol: http
     {{- end }}
     {{- if (and $setNodePorts (not (empty .Values.controller.service.nodePorts.http))) }}
@@ -66,7 +66,7 @@ spec:
       port: {{ .Values.controller.service.ports.https }}
       protocol: TCP
       targetPort: {{ .Values.controller.service.targetPorts.https }}
-    {{- if and (semverCompare ">=1.20" .Capabilities.KubeVersion.Version) (.Values.controller.service.appProtocol) }}
+    {{- if and (semverCompare ">=1.20.0-0" .Capabilities.KubeVersion.Version) (.Values.controller.service.appProtocol) }}
       appProtocol: https
     {{- end }}
     {{- if (and $setNodePorts (not (empty .Values.controller.service.nodePorts.https))) }}

I forked the chart and it fixed my issue. also in these two spots its already been changed: https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L40 https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L45

Service template section that needs to be updated: https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L57 https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L69

[k8s-ci-robot]

@45cali: 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.

[longwuyuan]

If you look at the docs, then you will see that the project recommends to use a yaml manifest when installing on AWS.

And if you look at https://github.com/kubernetes/ingress-nginx/blob/1e66a54974856cd31fea0e203a7b97b955ebe792/deploy/static/provider/aws/1.20/deploy.yaml#L372 you will see the appProtocol field is set and populated.

While you have a workaround to fork and maintain your own chart, changing the official chart template to match one cloud provider or one user is not a improvement in my opinion. If the majority of users are impacted then a change to the project's published chart makes sense.

/remove-kind bug

[45cali]

But the chart is already using the logic im requesting here:

https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L40
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/controller-service.yaml#L45

[gueux]

I'm agree with @45cali that it should be fixed in helm chart as the semverCompare condition doesn't allow any workaround for EKS cluster and appProtocol should be add regardless Kubernetes distribution

[kxgillispie]

@longwuyuan could you please clarify where this is stated? I'm having difficulty finding this recommendation in the installation docs.

If you look at the docs, then you will see that the project recommends to use a yaml manifest when installing on AWS.

Thanks!

[longwuyuan]

I would assume that the project publishing a command, in the AWS section of the deployment docs, that uses a yaml manifest, implies that. More so because the project actually generates and maintains that yaml manifests in variations for different Kubernetes Version even.

In case that implication is not good enough, you can submit a PR, to improve docs.

[k8s-triage-robot]

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-triage-robot]

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:

• 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 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 rotten

[k8s-triage-robot]

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

This bot triages issues 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:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

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

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

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

This bot triages issues 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:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

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

/close not-planned

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.