Add portNamePreffix Helm chart parameter

[pauljamm]

Allow user to set custom preffix for TCP and UDP ports

What this PR does / why we need it:

Allow users to add custom preffix in Helm chart for TCP and UDP ports.

This is required as some clouds (e.g. Yandex Cloud) doesn't allow ports with names started with numbers.
E.g. for Yandex Cloud apllicable names are /[a-z][-a-z0-9]{1,61}[a-z0-9]/

This issue was mensioned here #6416
And there were a couple of tries to fix it with a head-on solution:
#7515
#7276

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

Which issue/s this PR fixes

fixes #6416

How Has This Been Tested?

Made sure the load balancer is provisioned successfully after installing the patched chart.
Tested on Yandex Managed Service for Kubernetes.
Also was added test Helm values, and ran e2e Helm tests.

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.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pauljamm / name: Pavel Selivanov (8f4f546f460297fb54f4ae4389453a3a665db185)

[k8s-ci-robot]

@pauljamm: 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]

Welcome @pauljamm!

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 @pauljamm. 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]

Hi @pauljamm ,

Although this PR is targeted for Yandex Cloud users, do you know if the same functionality will work for non-Yandex provider clusters ?

[longwuyuan]

@pauljamm, sorry for asking but which file & line number is mapping a "string" name to a tcp/port number, in this change.

[pauljamm]

Hi @pauljamm ,

Although this PR is targeted for Yonder Cloud users, do you know if the same functionality will work for non-Yandex provider clusters ?

Hi @longwuyuan,

This PR just adds ability to specify preffix for loadbalancer service ports names. So it does not contain any changes that should be "supported" by other clouds or Kubernetes clusters. But anyway it's just a manipulation with a port name, which is supported by Kubernetes by itself.

[pauljamm]

@pauljamm, sorry for asking but which file & line number is mapping a "string" name to a tcp/port number, in this change.

The origin of a port naming in the ingress Helm chart was
take a port number (key of the dict) from user values, eg

tcp: 9000: "default/test:8080"

and set it as port number in service and add port name in the form

<port-number>-tcp(or udp)

My patch adds additional preffix (if specified by user) to all tcp or udp port names, not changing the original logic of port-name mapping.

So eg here
https://github.com/kubernetes/ingress-nginx/pull/8458/files#diff-2f370c167e670927f2304f85fa3919febc2dcc6231d0c56c985b8784092fba53R134-R135

the first line adds a port name in the form

<preffix if specified>-<key of the dict from user values (see above)>-tcp(or udp)

[longwuyuan]

@pauljamm , I think its looking good, but please comment if you will write a test. Just to deploy on a kind cluster and use a name for a tcp port and another name for a udp port, and get a 200 response while using those names.

[pauljamm]

@pauljamm , I think its looking good, but please comment if you will write a test. Just to deploy on a kind cluster and use a name for a tcp port and another name for a udp port, and get a 200 response while using those names.

@longwuyuan, I defenetly will, but could you provide some examples (or may be guide) of such tests for this repo?
I've found test values for Helm charts and added two new files (for deployment and for daemonset) with my parameter, but as I can see, this test only verifies that the values and templates using them are correct and chart can be deployed with them.

[pauljamm]

@pauljamm , I think its looking good, but please comment if you will write a test. Just to deploy on a kind cluster and use a name for a tcp port and another name for a udp port, and get a 200 response while using those names.

@longwuyuan, I defenetly will, but could you provide some examples (or may be guide) of such tests for this repo? I've found test values for Helm charts and added two new files (for deployment and for daemonset) with my parameter, but as I can see, this test only verifies that the values and templates using them are correct and chart can be deployed with them.

@longwuyuan if you mean this kind of tests, it's already being tested under the similar conditions here. It's just another string preffix for service port name.

[longwuyuan]

• This link

  ingress-nginx/test/e2e/tcpudp/tcp.go

  Line 115 in 1a8c70d

  Name: "dns-external-name-svc",

  is the name of a service. Its not a name of a port so it is not testing the change proposed in this PR
• The e2e tests are under the tests directory https://github.com/kubernetes/ingress-nginx/tree/main/test/e2e
• This is the beginning of testing related docs https://kubernetes.github.io/ingress-nginx/developer-guide/getting-started/#testing
• A test for first creating a named TCP port and also a named UDP port and then using that name in the configuration for routing traffic is critically needed for this PR. This is because TCP/UDP traffic is managed differently from http/https traffic

[rikatz]

/ok-to-test

[rikatz]

/approve
/hold
Please just fix the comment, leave it a bit more descriptive on why the field is required, what's the usage case for that field.

@longwuyuan feel free to lgtm as soon as this is fixed!

Thank you all :)

[longwuyuan]

Yeah, we should make progress but if there is no test that shows a TCP/UDP port getting named and then used in a working functional K8S object related to ingress-nginx-controller, then I prefer to hold until the test is available.

[longwuyuan]

@pauljamm , hope you are doing great.
Any update would help make progress.
Are you going to show your test details in the PR or write a test in e2e suite here.

[pauljamm]

@longwuyuan sorry for not answering for a long time.

• This link

  ingress-nginx/test/e2e/tcpudp/tcp.go

  Line 115 in 1a8c70d

  Name: "dns-external-name-svc",

  
  is the name of a service. Its not a name of a port so it is not testing the change proposed in this PR

Just to be clear, this is the name of a port. See above - this is a port object

ingress-nginx/test/e2e/tcpudp/tcp.go

Line 113 in 1a8c70d

Ports: []corev1.ServicePort{

And this is the name of a service

ingress-nginx/test/e2e/tcpudp/tcp.go

Line 107 in 1a8c70d

Name: "dns-external-name-svc",

[pauljamm]

@longwuyuan sorry, but it's not clear for me, why should we create duplicated tests, which besides just tests built in basic Kubernetes functionality.

[longwuyuan]

/lgtm

[pauljamm]

/assign @rikatz @ChiefAlexander @cpanato

[cpanato]

thanks for this PR
lgtm but can you bump the chart version?

[pauljamm]

thanks for this PR lgtm but can you bump the chart version?

@cpanato
Thanks for the review.
Chart version bumped.

[rikatz]

/lgtm
/hold cancel

[rikatz]

Thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: longwuyuan, pauljamm, rikatz

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

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