CFE-10: Add host-network-bind-options #1002

Elbehery · 2022-01-10T16:13:57Z

This PR add a proposal for adding Binding Options for the IngressAPI

Elbehery · 2022-01-10T16:14:24Z

/assign @alebedev87
/assign @lmzuccarelli
/assign @tjungblu

lmzuccarelli · 2022-01-11T09:27:06Z

enhancements/ingress/host-network-bind-options.md

+## Summary
+
+This enhancement updates the Ingress Controller API to allow specifying binding options in the HostNetwork strategy and allows running many instances of the ingress controller on the same node using the HostNetwork strategy, therefore avoiding port binding conflicts.
+## Motivation


nit: Maybe add a new line before Motivation

If I can make a suggestion, can we capitalize Ingress through the entire file where applicable . I do see instances of ingress.

lmzuccarelli · 2022-01-11T09:31:36Z

enhancements/ingress/host-network-bind-options.md

+
+While utilising the HostNetwork strategy for ingress controllers, the ports 80, 443, 10443, 10444, 1936 on the host are bound by HAProxy for http, https, no_sni, sni and stats correspondingly. 
+
+Those ports might be occupied by other processes which makes it unfeasible to run multiple ingress controllers on the same node with HostNetwork strategy. 


maybe something like "These ports might be occupied by other processes which will make it impossible to run multiple..."

lmzuccarelli · 2022-01-11T09:41:31Z

enhancements/ingress/host-network-bind-options.md

+
+### User Stories
+
+#### As a cluster administrator, I need to set binding ports for the IngressController


maybe add "so that I can deploy multiple ingress controllers on the same node for the HostNetwork strategy"

lmzuccarelli · 2022-01-11T09:45:51Z

enhancements/ingress/host-network-bind-options.md

+
+Also the deployments should not have ip:port conflicts for `SNI_PORT` and `NO_SNI_PORT`. There are multiple ways to achieve this:
+
+- The cluster ingress operator configures the router to use a unique loopback address for the sni/nosni frontends. This loopback address would be determined deterministically, for example by hashing the ingresscontroller’s name.


nit: not sure about "determined deterministically", I'm not an English grammar expert but maybe something like "the loopback address is configured automatically , for example ...

lmzuccarelli · 2022-01-11T09:50:39Z

@Elbehery - could you check and fix the lint problem ?

lmzuccarelli · 2022-01-11T11:54:21Z

@Elbehery - lets discuss the test plan, and other topics not addressed in the PR

alebedev87 · 2022-01-11T14:52:46Z

enhancements/ingress/host-network-bind-options.md

+    ```go
+    // HostNetworkStrategy holds parameters for the HostNetwork endpoint publishing
+    // strategy.
+    type HostNetworkStrategy struct {
+        // ...
+        // bindingOptions defines parameters for binding haproxy in ingress controller pods.
+        // All fields are optional and will use their respective defaults if not set.
+        // See specific bindingOptions fields for more details.
+        //
+        //
+        // Setting fields within bindingOptions is generally not recommended. The
+        // default values are suitable for most configurations.
+        //
+        // +optional
+        BindingOptions *IngressControllerBindingOptions `json:"bindingOptions,omitempty"`
+    }
+    ```


This and subsequent code blocks got rendered weirdly: no syntax highlight and back tips visible.
Maybe it's the matter of indent in the raw markdown, I think the code block doesn't need the indent.

alebedev87 · 2022-01-11T14:57:07Z

enhancements/ingress/host-network-bind-options.md

+    type IngressControllerPorts struct {
+        // http defines the port number which HAProxy process binds for
+        // http connections. Setting this field is generally not recommended. However in
+        // HostNetwork strategy, default http 80 port might be occupied by other processess


Suggested change

// HostNetwork strategy, default http 80 port might be occupied by other processess

// HostNetwork strategy, default http 80 port might be occupied by other processes

alebedev87 · 2022-01-11T14:57:16Z

enhancements/ingress/host-network-bind-options.md

+        HTTP int32 `json:"http,omitempty"`
+        // https defines the port number which HAProxy process binds for
+        // https connections. Setting this field is generally not recommended. However in
+        // HostNetwork strategy, default https 443 port might be occupied by other processess


Suggested change

// HostNetwork strategy, default https 443 port might be occupied by other processess

// HostNetwork strategy, default https 443 port might be occupied by other processes

alebedev87 · 2022-01-11T14:57:35Z

enhancements/ingress/host-network-bind-options.md

+        // stats is the port number which HAProxy process binds
+        // to expose statistics on it. Setting this field is generally not recommended.
+        // However in HostNetwork strategy, default stats port 1936 might
+        // be occupied by other processess


Suggested change

// be occupied by other processess

// be occupied by other processes

alebedev87 · 2022-01-11T15:00:20Z

enhancements/ingress/host-network-bind-options.md

+If this was the first insertion (L4 load balancer in front of the router) the router's port will be the only one seen by the server (application inside OpenShift).
+And if the server relies on `X-Forwarded-*` headers (like OpenShift image registry for instance) this may give a wrong impression that the client used the port set in `X-Forwarded-Port` which in case of the custom port binding may be different.
+
+A mitigation to this risk is to change the forwarded header policy on the ingress controller or on the route level: [documentation](https://docs.openshift.com/container-platform/4.8/networking/ingress-operator.html#nw-using-ingress-forwarded_configuring-ingress).


Suggested change

A mitigation to this risk is to change the forwarded header policy on the ingress controller or on the route level: [documentation](https://docs.openshift.com/container-platform/4.8/networking/ingress-operator.html#nw-using-ingress-forwarded_configuring-ingress).

A mitigation to this risk is to change the forwarded header policy on the ingress controller or on the route level: [documentation](https://docs.openshift.com/container-platform/4.9/networking/ingress-operator.html#nw-using-ingress-forwarded_configuring-ingress).

Elbehery · 2022-01-12T15:37:12Z

@alebedev87 thanks for your review, I have adjusted all of them

cc @lmzuccarelli @sherine-k

arjunrn · 2022-01-13T12:02:33Z

enhancements/ingress/host-network-configurable-ports.md

+```
+### Validation
+
+Setting `spec.endpointPublishingStrategy.type: HostNetwork` and ommitting 


Suggested change

Setting `spec.endpointPublishingStrategy.type: HostNetwork` and ommitting

Setting `spec.endpointPublishingStrategy.type: HostNetwork` and omitting

@arjunrn adjusted 🙏🏽

Elbehery · 2022-01-13T12:17:55Z

@sherine-k I added you to the authors section 👍🏽

arjunrn · 2022-01-13T12:26:51Z

enhancements/ingress/host-network-configurable-ports.md

+
+Users might define conflicting ports which would cause HAProxy process to fail at startup.
+
+A mitigation to this risk is to implement a validation feature in the reconciliation loop of the Cluster Ingress Operator


So we're going to validate in the reconcile loop. It would be nicer to prevent the creation of IngressController resources with conflicting ports. For instance when a NodePort service is created with a port which is already allocated then the creation fails with an error message, like so:

The Service "service02" is invalid: spec.ports[0].nodePort: Invalid value: 30007: provided port is already allocated

Yeah, would be good to have a validating webhook for the ingress operator. But for the moment the validation is done in the reconcile loop.

arjunrn · 2022-01-13T12:41:26Z

enhancements/ingress/host-network-configurable-ports.md

+
+#### Usage of X-Forwarded headers
+
+The default forwarded header policy of the ingress controller is to append X-Forwarded headers. This results into the insertion of `X-Forwarded-Port` header on OpenShift router side (HAProxy acts as a reverse proxy).


But why is this needed only when a non-default port if used by a reverse-proxy. This condition "And if the server relies on X-Forwarded-* headers (like OpenShift image registry for instance) this may give a wrong impression that the client used the port set in X-Forwarded-Port which in case of the custom port binding may be different." should be true even for the default router.

The default router binds on the standard 80, 443 ports which match the ports of the LB which "frontends" the router (endpointPublishingStrategy == LoadBalancer).
This works well for the applications like the image registry which needs to send the token retrieval URL back to the client. The token retrieval URL uses:

OpenShift cluster's *.apps wildcard DNS which get resolved to the before mentioned LB

original port (the one appended to X-Forwarded-Port by the router)

So, if the router is bound on a port 10443 instead of 443, the image registry will send the following URL:

https://registry.apps.<cluster_name>.<base_domain>.:10443/openshift/token?account=my-sa&client_id=docker&offline_token=true

which will fail as the "frontend" LB listens on the standard port 443 not 10443.

The default policy is append. So each host and port will be appended. The image registry should just use the first host and port in the headers. I don't think having non-standard behavior when one setting is changed is a good idea. It's confusing for a user who is trying to debug an issue.

The image registry does take the first host and port, it's just that the first values are the router ones as the LB in front of the OpenShift cluster is L4 one, so it doesn't append any HTTP headers.

I don't think having non-standard behavior when one setting is changed is a good idea. It's confusing for a user who is trying to debug an issue.

By "non-standard behavior" you mean the disabling of the append of the X-Forwarded headers? And by "one setting is changed" you mean the usage of the custom ports?
If so, the idea was to warn about this potential issue but keep the EP reasonably small moving the responsibility about using X-Forwarded headers to the applications. Do you have any idea of how to avoid the issue at reasonable cost?

Discussed this offline. This mitigation is not to be implemented in the controller but can be applied by the user to workaround any issues which might arise from components which use the X-Forwarded-Port to reconstruct the request URL.

tjungblu · 2022-01-14T05:28:06Z

enhancements/ingress/host-network-configurable-ports.md

@@ -0,0 +1,298 @@
+---
+title: ingress-host-network-configurable-ports
+


that's extremely picky, but what's up with all those newlines here? this seems to also prevent the markdown renderer to show this as a table

arjunrn · 2022-01-14T13:03:52Z

enhancements/ingress/host-network-configurable-ports.md

+
+Update the IngressController API by adding an optional `HostNetworkParameters` field with type `*HostNetworkParameters` to the `HostNetworkStrategy` struct:
+
+```go


This multiple level of structs for the configuration seems redundant since the structs contain a single field. The HTTP, HTTPS and STATS port configuration can be moved into HostNetworkStrategy directly.

I agree with the idea of the simplification. But I think it can be done in many ways.

What Arjun proposed:

endpointPublishingStrategy: type: HostNetwork hostNetwork: httpPort: 10080 httpsPort: 10443 statsPort: 11936

Similar to Arjun's one but with 1 level more to avoid Port suffix:

endpointPublishingStrategy: type: HostNetwork hostNetwork: ports: http: 10080 https: 10443 stats: 11936

What Sherine put into EP:

endpointPublishingStrategy: type: HostNetwork hostNetwork: hostNetworkParameters: httpPort: 10080 httpsPort: 10443 statsPort: 11936

Removing the hostNetworkParameters level makes sense too.
I vote for # 1

I think I'm more for the second option just to logically group similar fields.
However if the majority is for the first option, I'm fine with this.

Miciah · 2022-02-02T06:36:41Z

enhancements/ingress/host-network-configurable-ports.md

+
+## Motivation
+
+In the current implementation, Ingress Controllers in `HostNetwork` strategy use standard ports for HTTP(80), HTTPS(443), NO_SNI(10443), SNI(10444) and STATS(1936) on the underlying nodes. 


We changed the HAProxy configuration not to use TCP ports 10443 and 10444 in OpenShift 4.10: openshift/router#326

Suggested change

In the current implementation, Ingress Controllers in `HostNetwork` strategy use standard ports for HTTP(80), HTTPS(443), NO_SNI(10443), SNI(10444) and STATS(1936) on the underlying nodes.

In the current implementation, Ingress Controllers in `HostNetwork` strategy use standard ports for HTTP (80), HTTPS (443), and stats (1936) on the underlying nodes.

Miciah · 2022-02-02T06:40:17Z

enhancements/ingress/host-network-configurable-ports.md

+Configuring http, https and stats port for the ingress controllers that use the `HostNetwork` endpoint publishing strategy.
+
+
+Update the IngressController API by adding an optional `HostNetwork` field with type `*IngressControllerHostNetwork` to the `HostNetworkStrategy` struct:


A HostNetwork field already exists; you are adding new subfields to the existing HostNetwork field.

Miciah · 2022-02-02T06:44:30Z

enhancements/ingress/host-network-configurable-ports.md

+        //
+        // +kubebuilder:validation:Optional
+        // +kubebuilder:validation:Minimum=1
+        // +kubebuilder:validation:Maximum=30000


Why specify +kubebuilder:validation:Maximum=30000? I'm guessing the intention is to prevent overlap with the nodeport range, correct? The default nodeport range is 30000-32767, but this range is configurable, and anyway, the user may want to use a port above the high end of the nodeport range. I suggest removing this restriction, instead specifying +kubebuilder:validation:Maximum=65535, and adding a note in the godoc that the user would be well advised to avoid specifying a port in the configured nodeport range.

Miciah · 2022-02-02T07:06:12Z

enhancements/ingress/host-network-configurable-ports.md

+        // +kubebuilder:default:=443
+        // +optional
+        HTTPSPort int32 `json:"httpsPort,omitempty"`
+        // stats is the port number that router pods will open on the hosting node for stats.


This would be a good opportunity to explain that the purpose of exposing the metrics port is to facilitate health checks by external load-balancers. We can also provide some guidance on proper configuration of health checks. For example, something like the following could be helpful:

If you configure an external load-balancer to forward connections this ingress controller, the load balancer should use this port for health checks. A load balancer can send HTTP probes to this port on a given node with the path /healthz/ready to determine whether the ingress controller is ready to receive traffic on the node. For proper operation, the load balancer must not forward traffic to a node until /healthz/ready reports ready, and the load balancer must stop forwarding within a maximum of 45 seconds after /healthz/ready starts reporting not-ready. Probing every 5 or 10 seconds, with a 5-second timeout and with a threshold of two successful or failed requests to become healthy or unhealthy, respectively, are well-tested values.

Miciah · 2022-02-02T07:07:01Z

enhancements/ingress/host-network-configurable-ports.md

+
+```
+
+Here's an example of a public and an internal ingress controllers with the HostNetwork strategy that run on the same nodes without port conflicts:


Suggested change

Here's an example of a public and an internal ingress controllers with the HostNetwork strategy that run on the same nodes without port conflicts:

Here's an example of a public ingress controller and an internal ingress controller with the HostNetwork strategy that run on the same nodes without port conflicts:

Miciah · 2022-02-02T07:27:09Z

enhancements/ingress/host-network-configurable-ports.md

+
+#### Failure Modes
+
+N/A.


A router pod could fail to be scheduled if no node had compatible labels and taints and available ports; in this case, the ingresscontroller's status conditions should report the scheduling failure.

A router pod could be scheduled to a node where the configured port were in use by something other than a pod, such as a system daemon (the Kubernetes schedule only checks for conflicts with ports allocated by Kubernetes); in this case, the ingresscontroller's status conditions should reflect the pod's failure to start.

A router pod could be scheduled and start listening on a port inside the nodeport range; in this case, a subsequently created nodeport service could by chance be assigned the in-use port and fail to be configured on the host with the router pod.

Miciah · 2022-02-02T07:28:39Z

enhancements/ingress/host-network-configurable-ports.md

+n
+N/A.


Spurious 'n'.

We could document here how to check the ingresscontroller's status conditions, router pod logs, and SDN/OVN logs to diagnose the aforementioned failure modes, and how to remediate them.

Miciah · 2022-02-02T07:31:54Z

enhancements/ingress/host-network-configurable-ports.md

+
+### Upgrade / Downgrade Strategy
+
+N/A.


There shouldn't be any issues when upgrading to a version of OpenShift with this enhancement, but there could be problems if the user downgraded after creating ingresscontrollers using this enhancement.

Miciah · 2022-02-02T07:33:18Z

enhancements/ingress/host-network-configurable-ports.md

+
+## Drawbacks
+
+N/A.


Additional operational complexity, which will increase the needed test matrix, frustration for users, and volume of customer cases.

Miciah · 2022-02-02T07:54:56Z

enhancements/ingress/host-network-configurable-ports.md

+
+## Alternatives
+
+N/A.


An alternative would be to continue to guide users to the NodePortService endpoint publishing strategy type. Another alternative (or variation on that alternative) would be to augment the NodePortService strategy to allow the user to specify requested ports within the nodeport range. I understand that some users have been less than completely satisfied with NodePortService, but it has been our answer for years, so it would be useful to document why we have decided now to add this enhancement.

How the cluster ingress operator can find an available port from the nodeport range in a race free way?

From the docs:

If you want a specific port number, you can specify a value in the nodePort field. The control plane will either allocate you that port or report that the API transaction failed.

I think @Miciah was saying that the same configuration could be added to the node port strategy instead of trying to expose the ports through the host network.

I see, on the level of the contract with the user of the API, specifying host ports or node ports are equally prone to the conflicts.
Then let me ask this timely question: why this was not considered as a counter proposal to the customer need?
Why customers are more satisfied with the hostNetwork publishing strategy? Performance (fewer hops)?

@Miciah , @arjunrn & @alebedev87
From the information we got from the PMs(@lmzuccarelli + @Deepthidharwar), other customers are still interested in implementing this change for hostNetwork.

Miciah · 2022-02-28T15:07:05Z

enhancements/ingress/host-network-configurable-ports.md

+        // A load balancer can send HTTP probes to this port on a given 
+        // node with the path /healthz/ready to determine whether the 
+        // ingress controller is ready to receive traffic on the node.
+        // For proper operation, the load balancer must not forward traffic // to a node until /healthz/ready reports ready, and the load 


Suggested change

// For proper operation, the load balancer must not forward traffic // to a node until /healthz/ready reports ready, and the load

// For proper operation, the load balancer must not forward traffic

// to a node until /healthz/ready reports ready, and the load

Miciah · 2022-02-28T15:08:00Z

enhancements/ingress/host-network-configurable-ports.md

+        // +kubebuilder:validation:Maximum=65535
+        // +kubebuilder:validation:Minimum=1
+        // +kubebuilder:default=80
+        HTTPPort int `json:"HTTPPort"`


This field and the other port fields should have type int32; see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types.

Miciah · 2022-02-28T15:10:46Z

enhancements/ingress/host-network-configurable-ports.md

+
+Cluster administrators should watch out for the ingress controller status after updates:
+In case the router pod cannot be scheduled due to port unavailability (or any other scheduling concerns), the ingress controller's conditions will report the scheduling failure.
+In such a case, the cluster administrator might need to inspect other pods or daemonsets that use the same port. Router pod logs, OVN/SDN logs might be helpful to identify the root cause for scheduling failure.


I would say "pods and system daemons"; a likely source of conflicts is running daemons on the host that aren't managed by OpenShift. ss or netstat would be an easy way to check for conflicts.

We actually have a host-port registry -- https://github.com/openshift/enhancements/blob/master/dev-guide/host-port-registry.md -- as well as user-facing documentation (https://github.com/openshift/openshift-docs/blob/main/modules/installation-network-user-infra.adoc) for which ports we "claim". So end-user documentation should reference those ranges as being off-limits.

Miciah · 2022-02-28T15:12:43Z

enhancements/ingress/host-network-configurable-ports.md

+In case the router pod cannot be scheduled due to port unavailability (or any other scheduling concerns), the ingress controller's conditions will report the scheduling failure.
+In such a case, the cluster administrator might need to inspect other pods or daemonsets that use the same port. Router pod logs, OVN/SDN logs might be helpful to identify the root cause for scheduling failure.
+
+Cluster administrators might also encounter issues if an ingresscontroller with HostNetwork strategy is using a port from the nodePort range: if a nodeport service attempts to use the same port, the router pod will not be able to get scheduled correctly. The nodePort ingress controller's conditions should report the scheduling failure.


If a nodeport service attempts to use a port that the ingresscontroller is already using, then the nodeport service may not work properly. I know that OVN-Kubernetes will log the conflict in its pods; I'm not sure about openshift-sdn's failure mode.

Miciah · 2022-02-28T15:14:25Z

enhancements/ingress/host-network-configurable-ports.md

+    status: "True"
+    type: PodsScheduled


In case of failure to schedule pods, the operator would report PodsScheduled=False (I hope).

Miciah · 2022-02-28T15:15:58Z

enhancements/ingress/host-network-configurable-ports.md

+Logs reporting the failure to bind to the requested port on the underlying host should be found.
+
+c. Checking SDN/OVN logs
+Check the logs from the ovn/sdn pods on the nodes where the router pods are running for errors related to port conflicts.


It would be helpful to identify the pods to check here. You can ask the SDN team if you need help figuring out the appropriate namespace or pod names.

Miciah · 2022-02-28T15:18:58Z

enhancements/ingress/host-network-configurable-ports.md

+* Advise users to the NodePortService endpoint publishing strategy type. 
+* Augment the NodePortService strategy to allow the user to specify requested ports within the nodeport range. 


Usually this section will include some explanation of why the alternatives were rejected. Do we have that information? Maybe it's as simple as that these alternatives have been rejected by a subset of users who require HostNetwork for unspecified reasons. If we do know a reason why these alternatives were rejected, this would be the place to record it.

alebedev87 · 2022-02-28T15:34:05Z

enhancements/ingress/host-network-configurable-ports.md

+        // +kubebuilder:validation:Maximum=65535
+        // +kubebuilder:validation:Minimum=1
+        // +kubebuilder:default=443
+        HTTPSPort int `json:"HTTPSPort"`


It's not the API definition, so it's less important, however to match with the example below (and with the most API examples actually): JSON tag names are better to be lower camel case.

alebedev87 · 2022-03-02T09:31:07Z

enhancements/ingress/host-network-configurable-ports.md

+oc logs -f ds/ovnkube-node -n openshift-ovn-kubernetes -c ovnkube-node
+W0301 17:17:48.060076    5734 port_claim.go:119] PortClaim for xxxx: xxxx/xxxxxxxx on port: 32634, err: listen tcp :32634: bind: address already in use
+E0301 17:17:48.060097    5734 port_claim.go:145] Error claiming port for service: xxxx/xxxxxxxx: listen tcp :32634: bind: address already in use


This needs to be a shell command sh.

Updated. Thanks

Miciah · 2022-03-22T17:06:57Z

/approve

openshift-ci · 2022-03-22T17:22:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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:

~~OWNERS~~ [Miciah]

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

arjunrn · 2022-03-23T09:33:49Z

/label tide/merge-method-squash

Miciah · 2022-03-23T14:47:10Z

/lgtm

Miciah · 2022-03-23T22:47:01Z

/refresh

openshift-ci bot assigned alebedev87, lmzuccarelli and tjungblu Jan 10, 2022

openshift-ci bot requested review from smarterclayton and squeed January 10, 2022 16:14

lmzuccarelli reviewed Jan 11, 2022

View reviewed changes

Elbehery changed the title ~~CFE-12: Add host-network-bind-options~~ CFE-10: Add host-network-bind-options Jan 11, 2022

Elbehery force-pushed the Add-host-network-bind-options branch 2 times, most recently from 200806e to c4baec2 Compare January 11, 2022 13:27

alebedev87 reviewed Jan 11, 2022

View reviewed changes

Elbehery force-pushed the Add-host-network-bind-options branch from c4baec2 to 8dd3884 Compare January 12, 2022 15:36

arjunrn reviewed Jan 13, 2022

View reviewed changes

tjungblu reviewed Jan 14, 2022

View reviewed changes

arjunrn reviewed Jan 14, 2022

View reviewed changes

Miciah reviewed Feb 2, 2022

View reviewed changes

Miciah reviewed Feb 28, 2022

View reviewed changes

alebedev87 reviewed Feb 28, 2022

View reviewed changes

sherine-k force-pushed the Add-host-network-bind-options branch 2 times, most recently from bb5fe6c to 99593a2 Compare March 1, 2022 17:58

alebedev87 reviewed Mar 2, 2022

View reviewed changes

sherine-k force-pushed the Add-host-network-bind-options branch from 99593a2 to 7da4094 Compare March 14, 2022 16:11

sherine-k force-pushed the Add-host-network-bind-options branch from bc1c8bd to b1d0c6e Compare March 22, 2022 16:31

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2022

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 23, 2022

Configurable ports for Ingress Controller - HostNetwork strategy

55a8336

sherine-k force-pushed the Add-host-network-bind-options branch from b1d0c6e to 55a8336 Compare March 23, 2022 12:53

openshift-ci bot assigned Miciah Mar 23, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2022

openshift-merge-robot merged commit 53e64c2 into openshift:master Mar 23, 2022


		While utilising the HostNetwork strategy for ingress controllers, the ports 80, 443, 10443, 10444, 1936 on the host are bound by HAProxy for http, https, no_sni, sni and stats correspondingly.

		Those ports might be occupied by other processes which makes it unfeasible to run multiple ingress controllers on the same node with HostNetwork strategy.


		### User Stories

		#### As a cluster administrator, I need to set binding ports for the IngressController


		Also the deployments should not have ip:port conflicts for `SNI_PORT` and `NO_SNI_PORT`. There are multiple ways to achieve this:

		- The cluster ingress operator configures the router to use a unique loopback address for the sni/nosni frontends. This loopback address would be determined deterministically, for example by hashing the ingresscontroller’s name.

	// HostNetwork strategy, default http 80 port might be occupied by other processess
	// HostNetwork strategy, default http 80 port might be occupied by other processes

	// HostNetwork strategy, default https 443 port might be occupied by other processess
	// HostNetwork strategy, default https 443 port might be occupied by other processes

	// be occupied by other processess
	// be occupied by other processes

	A mitigation to this risk is to change the forwarded header policy on the ingress controller or on the route level: [documentation](https://docs.openshift.com/container-platform/4.8/networking/ingress-operator.html#nw-using-ingress-forwarded_configuring-ingress).
	A mitigation to this risk is to change the forwarded header policy on the ingress controller or on the route level: [documentation](https://docs.openshift.com/container-platform/4.9/networking/ingress-operator.html#nw-using-ingress-forwarded_configuring-ingress).

	Setting `spec.endpointPublishingStrategy.type: HostNetwork` and ommitting
	Setting `spec.endpointPublishingStrategy.type: HostNetwork` and omitting


		Users might define conflicting ports which would cause HAProxy process to fail at startup.

		A mitigation to this risk is to implement a validation feature in the reconciliation loop of the Cluster Ingress Operator


		#### Usage of X-Forwarded headers

		The default forwarded header policy of the ingress controller is to append X-Forwarded headers. This results into the insertion of `X-Forwarded-Port` header on OpenShift router side (HAProxy acts as a reverse proxy).

		n
		N/A.


		## Drawbacks

		N/A.


		## Alternatives

		N/A.

		@@ -0,0 +1,298 @@
		---
		title: ingress-host-network-configurable-ports


		Update the IngressController API by adding an optional `HostNetworkParameters` field with type `*HostNetworkParameters` to the `HostNetworkStrategy` struct:

		```go


		## Motivation

		In the current implementation, Ingress Controllers in `HostNetwork` strategy use standard ports for HTTP(80), HTTPS(443), NO_SNI(10443), SNI(10444) and STATS(1936) on the underlying nodes.

		Configuring http, https and stats port for the ingress controllers that use the `HostNetwork` endpoint publishing strategy.


		Update the IngressController API by adding an optional `HostNetwork` field with type `*IngressControllerHostNetwork` to the `HostNetworkStrategy` struct:


		```

		Here's an example of a public and an internal ingress controllers with the HostNetwork strategy that run on the same nodes without port conflicts:

	Here's an example of a public and an internal ingress controllers with the HostNetwork strategy that run on the same nodes without port conflicts:
	Here's an example of a public ingress controller and an internal ingress controller with the HostNetwork strategy that run on the same nodes without port conflicts:

	// For proper operation, the load balancer must not forward traffic // to a node until /healthz/ready reports ready, and the load
	// For proper operation, the load balancer must not forward traffic
	// to a node until /healthz/ready reports ready, and the load

		* Advise users to the NodePortService endpoint publishing strategy type.
		* Augment the NodePortService strategy to allow the user to specify requested ports within the nodeport range.

CFE-10: Add host-network-bind-options #1002

CFE-10: Add host-network-bind-options #1002

Conversation

Elbehery commented Jan 10, 2022

Elbehery commented Jan 10, 2022

lmzuccarelli Jan 11, 2022 • edited

Choose a reason for hiding this comment

Amrita42 Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmzuccarelli Jan 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmzuccarelli commented Jan 11, 2022

lmzuccarelli commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elbehery commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elbehery commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Mar 22, 2022

openshift-ci bot commented Mar 22, 2022

arjunrn commented Mar 23, 2022

Miciah commented Mar 23, 2022

Miciah commented Mar 23, 2022

lmzuccarelli Jan 11, 2022 •

edited

Amrita42 Mar 2, 2022 •

edited

lmzuccarelli Jan 11, 2022 •

edited

alebedev87 Feb 2, 2022 •

edited