Making Kube service appProtocol field optional

[admgolovin]

What this PR does / why we need it:

This PR is making the appProtocol field of the Nginx ingress controller definition optional.

That is needed as not all Cloud providers could correctly work with it.
For example here is what happens when we expose our Nginx ingress-controller Kubernetes service through IBM Cloud NLB instance:

Events:
  Type     Reason                           Age                From                Message
  ----     ------                           ----               ----                -------
  Normal   EnsuringLoadBalancer             13s (x3 over 28s)  service-controller  Ensuring load balancer
  Warning  CreatingCloudLoadBalancerFailed  13s (x3 over 28s)  ibm-cloud-provider  Error on cloud load balancer afd6a32934ebe4cc89b782a88c9dea3a for service kube-system/nginx-ingress-dal10-3157072-controller-exp with UID fd6a3293-4ebe-4cc8-9b78-2a88c9dea3a8: Service configuration is not supported: application protocol
  Warning  SyncLoadBalancerFailed           13s (x3 over 28s)  service-controller  Error syncing load balancer: failed to ensure load balancer: Error on cloud load balancer afd6a32934ebe4cc89b782a88c9dea3a for service kube-system/nginx-ingress-dal10-3157072-controller-exp with UID fd6a3293-4ebe-4cc8-9b78-2a88c9dea3a8: Service configuration is not supported: application protocol

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

How Has This Been Tested?

Here is an example of the Nginx ingress Kubernetes service that is failing:

apiVersion: v1
kind: Service
metadata:
  annotations:
    meta.helm.sh/release-name: nginx-ingress
    meta.helm.sh/release-namespace: kube-system
    service.kubernetes.io/ibm-load-balancer-cloud-provider-enable-features: ipvs
    service.kubernetes.io/ibm-load-balancer-cloud-provider-ip-type: public
    service.kubernetes.io/ibm-load-balancer-cloud-provider-scheduler: rr
    service.kubernetes.io/ibm-load-balancer-cloud-provider-vlan: <vlan>
    service.kubernetes.io/ibm-load-balancer-cloud-provider-zone: <zone>
  labels:
    app.kubernetes.io/component: controller
    app.kubernetes.io/instance: nginx-ingress
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/version: 1.0.4
    helm.sh/chart: ingress-nginx-4.0.6
    loadBalancerType: main
    nlbVersion: "2"
  name: nginx-ingress-dal10-3157072-controller-exp
  namespace: kube-system
spec:
  externalTrafficPolicy: Local
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: http
    appProtocol: http
  - name: https
    port: 443
    protocol: TCP
    targetPort: https
    appProtocol: https
  selector:
    app.kubernetes.io/component: controller
    app.kubernetes.io/instance: nginx-ingress-dal10-3157072
    app.kubernetes.io/name: ingress-nginx
  sessionAffinity: None
  type: LoadBalancer

If we remove the appProtocol field, an IBM Cloud NLB instance will be created successfully for the Nginx ingress-controller service.
We also tried to roll back into the 4.0.0 version of the Nginx ingress-controller chart (before the appProtocol field was released). And it is working for us.

Checklist:

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

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

[k8s-ci-robot]

Welcome @admgolovin!

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 @admgolovin. 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.

[longwuyuan]

Does "optional" imply that the field is not set (by default to http/https) ?

[allen-servedio]

This feature was added in #7493. Even the author of that feature stated:

This is a minor change to make the deployment more semantic - although there is no particular reason to require this at this current time, having ports identify themselves makes life easier for any extensions / metrics scrapers / projects in future.

We have identified an edge case where this actually breaks things and so need to turn it off. So, in this case, the optionality of the feature is to have it on by default per the original author's intent with the ability to turn it off if needed @longwuyuan

[longwuyuan]

Can this be tested without IBM-Cloud. Say at least on a kind cluster. It would be appropriate to run at least some kind of tests, that assured non IBM-Cloud users don't see any impact at all.

[longwuyuan]

Also, if it is not too much trouble, then request that you create a issue and link it here. so that the issue tracks the details of the related information, for future searches. thanks. Sometimes directly creating PR seems ok but in my opinion, this one should have a issue that details the entire story with full specifics

[admgolovin]

Closes #7885

[admgolovin]

@longwuyuan That chart has been tested in the AWS EKS cluster as well. There are no breaking changes since those appProtocol fields are included in the Kubernetes service definition by default. We are just adding the ability to disable them if they don't needed.

[strongjz]

/check-cla
/triage accepted
/kind feature
/area helm
/priority important-longterm
/ok-to-test

[k8s-ci-robot]

@admgolovin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-ingress-nginx-boilerplate	7ef34b357fb17a7a29b5f2e643fb658efd08eb3f	link	true	/test pull-ingress-nginx-boilerplate

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[admgolovin]

@cpanato The rebase is done. All excessive commits have been squashed.

[rikatz]

argh sorry @admgolovin my bad!

We have just released a new version and there is another conflict.

Please fix both, and ping me (here or in Slack) so we can release this and fix the problem in IBM Cloud ASAP.

Thanks

[admgolovin]

@cpanato @rikatz The rebase is complete. PR is ready for merge.

[cpanato]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: admgolovin, cpanato

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 [cpanato]

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