Allow ports on host to be specified for HostNetwork Ingresses #1097

arjunrn · 2022-01-14T13:10:47Z

This changes adds fields to the HostNetworkStrategy to specify the ports on the host which should be bound to ports in the router pods.

alebedev87

Remarks to align with the proposal which used the api conventions.

operator/v1/types_ingress.go

alebedev87 · 2022-03-28T13:11:26Z

operator/v1/types_ingress.go

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


Shouldn't we stick with the lower camel case convention for the JSON tags?

You're right. It looks like all the fields are in lower case even for words like HTTP.

alebedev87 · 2022-03-28T13:51:11Z

/lgtm

operator/v1/types_ingress.go

Miciah · 2022-03-28T22:50:47Z

operator/v1/types_ingress.go

 	Protocol IngressControllerProtocol `json:"protocol,omitempty"`
+
+	// HTTPPort is the port on the host which should be used to listen for
+	// HTTP requests. This field should be set when port 80 is already in use.


For each new field, please add a sentence describing the default value. The default value that is specified by the kubebuilder tag doesn't show in generated Swagger or the OpenAPI schema.

This changes adds fields to the `HostNetworkStrategy` to specify the ports on the host which should be bound to ports in the router pods.

alebedev87 · 2022-03-29T08:13:14Z

/lgtm

deads2k · 2022-03-29T14:46:50Z

operator/v1/types_ingress.go

+	// unhealthy respectively, are well-tested values. When not specified the
+	// value defaults to 1936.
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Maximum=65535


do we know where the nodeport range starts in openshift clusters?

It appears to be configurable. The default is 30000-32767 but a user could configure it to start at a lower port number than 30000. Also the user might want to use a port outside the nodeport range, both higher or lower.

deads2k · 2022-03-29T14:52:28Z

operator/v1/types_ingress.go

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


CRD changes can be observed on a delay. What does the operator do when this field is zero-value, not 80, because the kube-apiserver has not observed the default.

We will have the same defaults also in the operator so if a 0 is observed it is defaulted there as well.

https://github.com/openshift/cluster-ingress-operator/pull/694/files#L392-L404

deads2k · 2022-03-30T13:09:54Z

/approve
/hold

holding for @Miciah .
/assign @Miciah

@Miciah feel free to remove the hold when you're happy with it.

Miciah · 2022-03-31T14:15:22Z

Thanks!
/lgtm
/hold cancel

arjunrn · 2022-03-31T14:24:29Z

/label px-approved
/label qe-approved
/label docs-approved

openshift-ci · 2022-03-31T14:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87, arjunrn, deads2k, 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~~ [deads2k]

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-31T14:52:59Z

/retest-required

openshift-ci · 2022-03-31T14:59:24Z

@arjunrn: all tests passed!

Full PR test history. Your PR dashboard.

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.

The e2e test `TestHostNetworkEndpointPublishingStrategy` is always failing. We directly need openshift/api#1097 but I bumped openshift/api to current latest. go get -u github.com/openshift/api@c3bb724c282a1ac34d7c769da99543a453ae90b9 go mod vendor go mod tidy

The API change in openshift/api#1097 means that the set of configurable ports on type HostNetworkStrategy are not marked as optional. It's not clear whether that was deliberate but without specifying these the e2e tests will always fail.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2022

openshift-ci bot requested review from knobunc and sttts January 14, 2022 13:11

arjunrn changed the title ~~[WIP] Allow ports on host to be specified for HostNetwork Ingresses~~ Allow ports on host to be specified for HostNetwork Ingresses Jan 27, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2022

arjunrn force-pushed the ingress-host-bind-ports branch from 09a8743 to cd04d6a Compare January 27, 2022 12:39

arjunrn force-pushed the ingress-host-bind-ports branch from cd04d6a to a9c2727 Compare March 25, 2022 09:22

alebedev87 reviewed Mar 28, 2022

View reviewed changes

operator/v1/types_ingress.go Outdated Show resolved Hide resolved

operator/v1/types_ingress.go Outdated Show resolved Hide resolved

operator/v1/types_ingress.go Outdated Show resolved Hide resolved

arjunrn force-pushed the ingress-host-bind-ports branch from a9c2727 to 9773c8b Compare March 28, 2022 11:47

alebedev87 reviewed Mar 28, 2022

View reviewed changes

arjunrn force-pushed the ingress-host-bind-ports branch from 9773c8b to 6a80c26 Compare March 28, 2022 13:32

openshift-ci bot assigned alebedev87 Mar 28, 2022

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

Miciah reviewed Mar 28, 2022

View reviewed changes

Allow ports on host to be specified for HostNetwork Ingresses

19dea10

This changes adds fields to the `HostNetworkStrategy` to specify the ports on the host which should be bound to ports in the router pods.

arjunrn force-pushed the ingress-host-bind-ports branch from 6a80c26 to 19dea10 Compare March 29, 2022 07:42

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

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

deads2k reviewed Mar 29, 2022

View reviewed changes

openshift-ci bot assigned Miciah Mar 30, 2022

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 30, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Mar 31, 2022

openshift-ci bot added qe-approved Signifies that QE has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Mar 31, 2022

openshift-merge-robot merged commit 6ff3e35 into openshift:master Mar 31, 2022

arjunrn deleted the ingress-host-bind-ports branch March 31, 2022 17:43

frobware mentioned this pull request Apr 12, 2022

Fix TestHostNetworkEndpointPublishingStrategy openshift/cluster-ingress-operator#738

Closed

Allow ports on host to be specified for HostNetwork Ingresses #1097

Allow ports on host to be specified for HostNetwork Ingresses #1097

Uh oh!

Conversation

arjunrn commented Jan 14, 2022

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Mar 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Mar 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 30, 2022

Uh oh!

Miciah commented Mar 31, 2022

Uh oh!

arjunrn commented Mar 31, 2022

Uh oh!

openshift-ci bot commented Mar 31, 2022

Uh oh!

arjunrn commented Mar 31, 2022

Uh oh!

openshift-ci bot commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants