feat: Add support for bindOptions in IngressController.operator.openshift.io/v1

[m-yosefpor]

fixes: #964

It is required for implementation of openshift/cluster-ingress-operator#633

[openshift-ci]

Hi @m-yosefpor. Thanks for your PR.

I'm waiting for a openshift 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.

[m-yosefpor]

/assign @smarterclayton

[pweil-]

Hi @m-yosefpor. Thanks for your contribution!

I've brought this to the attention of some of our networking folks. Currently we've deferred implementing a change like this because we'd like to encourage usage of non-host networking alternatives. I will circle back with our product folks to see if they'd like to move forward with this feature and, if so, we'll collaborate with you on the enhancement. 👍

[m-yosefpor]

Hi @m-yosefpor. Thanks for your contribution!

I've brought this to the attention of some of our networking folks. Currently we've deferred implementing a change like this because we'd like to encourage usage of non-host networking alternatives. I will circle back with our product folks to see if they'd like to move forward with this feature and, if so, we'll collaborate with you on the enhancement. 👍

Hi, @pweil- . Thank you very much. I've also mentioned in the description of the fields that:

Setting fields within bindOptions is generally not recommended.

However in some cases, such as using PBR rules we need the container routing table to be the same as configured one on the host , otherwise the traffic won't be routed correctly.

In case of using BMs or big VMs as openshift nodes, we can only run default ingressController routers with hostNetworking on the nodes, and running multiple set of routers (for different policies or for sharding, etc) is not currently possible without having control on bind ports.

I believe operator pattern should make the maintenance of the underling resource much easier for users with defaulting/tuning parameters for them, but also be configurable/customizable at any desired level, to be useful for advanced use-cases. IMHO, it should not severely limit the configurability.

Thanks for your considerations.

[pweil-]

@m-yosefpor we'd like to include this feature in a future release. @alebedev87 will be able to help you through the feature process. As part of that, there will likely need to be some discussion in the openshift/enhancements repo that Andrey can help with.

As part of the api design, it has been suggested that the proposed bindOptions live under the HostNetworkStrategy and we also provide defaults like // +kubebuilder:default:=80 and // +kubebuilder:default:=443. Things like that should shake out via PR review and the enhancement proposal.

Thanks again for the contribution. Please reach out to Andrey if you need more assistance.

[alebedev87]

/assign @alebedev87

[m-yosefpor]

@pweil- wow great news. Thanks. Sure I'll start working on the related PRs.

@alebedev87 I'll apply the suggestions today. Also two new variables should be also defined: SNIPort and noSNIPort. They are only used for internal router purposes and are binded only on lo, however to run multiple hostNetwork routers on the same host, they should be different to avoid port conflicts. I will also add them and will create the proposal PR in openshift/enhancement repo.

[alebedev87]

@m-yosefpor yes, you are right SNI and NoSNI too. Also, there is another one which should not be forgotten: StatsPort. It's controlled by STATS_PORT environment variable.

[m-yosefpor]

@alebedev87 suggestions applied

• fixed typos
• moving bindOptions under HostNetworkStrategy
• adding kubebuilder:default markers
• using a pointer to IngressControllerBindOptions
• adding sniPort, noSniPort, statsPort

I was not sure about sniPort and noSniPort camel cases. All IngressController keys starts with small cased even if they are acronyms (such as HTTP in httpHostNetwork), So I came up with these (instead of SNIPort,noSNIPort).

[m-yosefpor]

Enhancement Proposal added:

openshift/enhancements#855

[tjungblu]

/approve

[tjungblu]

/ok-to-test

[tjungblu]

@m-yosefpor could you kindly run make update again on this branch, check-in the result and potentially rebase over the master?

Thanks, and I really have to apologize for the delay - I'll help you to get this over the finish line.

[tjungblu]

/label tide/merge-method-squash

[openshift-ci]

@m-yosefpor: PR needs rebase.

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: m-yosefpor, tjungblu

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

Needs approval from an approver in each of these files:

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

[m-yosefpor]

superseded by #1043

@tjungblu @alebedev87