Specify host port for host networking strategy by arjunrn · Pull Request #694 · openshift/cluster-ingress-operator

arjunrn · 2022-01-14T13:50:45Z

When the load balancer strategy is hostnetworking allow the user to
specify which ports on the host the pods should use. Also fixup the
container ports.

Miciah

routerDefaultHostNetworkStatsPort needs to be fixed. My other comments are all minor suggestions.

pkg/operator/controller/ingress/controller.go

pkg/operator/controller/ingress/deployment.go

Miciah · 2022-03-29T02:03:20Z

pkg/operator/controller/ingress/deployment_test.go

+	// verify the default ports are used
+	checkContainerPort(t, deployment, "http", 80)
+	checkContainerPort(t, deployment, "https", 443)
+	checkContainerPort(t, deployment, "metrics", 1936)
+


Might as well check the corresponding environment variables too.

The environment variables for the HTTP and HTTPS have defaults and currently those defaults are used. I've added a check for the stats port environment variable because that's always set regardless of the strategy. Should I update the code to always set those environment variables as well?

Ah, I see what you're saying—the environment variables are only set if the endpoint publishing strategy is "HostNetwork". Never mind then.

alebedev87

LGTM

pkg/operator/controller/ingress/controller.go

alebedev87 · 2022-04-01T10:13:23Z

/lgtm

alebedev87 · 2022-04-01T10:27:43Z

/hold
@Miciah unhold if it's fine with you

Miciah · 2022-04-02T00:01:10Z

pkg/operator/controller/ingress/deployment.go

+	var statsPort int32 = routerDefaultHostNetworkStatsPort
+	var httpPort int32 = routerDefaultHostNetworkHTTPPort
+	var httpsPort int32 = routerDefaultHostNetworkHTTPSPort


Suggested change

var statsPort int32 = routerDefaultHostNetworkStatsPort

var httpPort int32 = routerDefaultHostNetworkHTTPPort

var httpsPort int32 = routerDefaultHostNetworkHTTPSPort

var (

statsPort int32 = routerDefaultHostNetworkStatsPort

httpPort int32 = routerDefaultHostNetworkHTTPPort

httpsPort int32 = routerDefaultHostNetworkHTTPSPort

)

Miciah · 2022-04-02T00:07:30Z

pkg/operator/controller/ingress/deployment_test.go

+	// verify the default ports are used
+	checkContainerPort(t, deployment, "http", 80)
+	checkContainerPort(t, deployment, "https", 443)
+	checkContainerPort(t, deployment, "metrics", 1936)
+


Ah, I see what you're saying—the environment variables are only set if the endpoint publishing strategy is "HostNetwork". Never mind then.

Miciah

You need to rebase for #720. Sorry about that.

Miciah · 2022-04-02T00:12:32Z

pkg/operator/controller/ingress/deployment.go

+		// append the environment variables for the HTTP and HTTPS ports
+		env = append(env,
+			corev1.EnvVar{
+				Name:  RouterServiceHTTPSPort,
+				Value: strconv.Itoa(int(config.HTTPSPort)),
+			},
+			corev1.EnvVar{
+				Name:  RouterServiceHTTPPort,
+				Value: strconv.Itoa(int(config.HTTPPort)),
+			},
+		)
+
+		// set the ports to the values from the host network configuration
+		httpPort = config.HTTPPort
+		httpsPort = config.HTTPSPort
+		statsPort = config.StatsPort


Slightly simpler:

Suggested change

// append the environment variables for the HTTP and HTTPS ports

env = append(env,

corev1.EnvVar{

Name: RouterServiceHTTPSPort,

Value: strconv.Itoa(int(config.HTTPSPort)),

},

corev1.EnvVar{

Name: RouterServiceHTTPPort,

Value: strconv.Itoa(int(config.HTTPPort)),

},

)

// set the ports to the values from the host network configuration

httpPort = config.HTTPPort

httpsPort = config.HTTPSPort

statsPort = config.StatsPort

// Set the ports to the values from the host network configuration.

httpPort = config.HTTPPort

httpsPort = config.HTTPSPort

statsPort = config.StatsPort

// Append the environment variables for the HTTP and HTTPS ports.

env = append(env,

corev1.EnvVar{

Name: RouterServiceHTTPSPort,

Value: strconv.Itoa(int(httpsPort)),

},

corev1.EnvVar{

Name: RouterServiceHTTPPort,

Value: strconv.Itoa(int(httpPort)),

},

)

Miciah · 2022-04-02T00:25:53Z

pkg/operator/controller/ingress/deployment.go

+	deployment.Spec.Template.Spec.Containers[0].Ports = append(
+		deployment.Spec.Template.Spec.Containers[0].Ports,
+		corev1.ContainerPort{
+			Name:          HTTPPortName,
+			ContainerPort: httpPort,
+		},
+		corev1.ContainerPort{
+			Name:          HTTPSPortName,
+			ContainerPort: httpsPort,
+		},
+		corev1.ContainerPort{
+			Name:          StatsPortName,
+			ContainerPort: statsPort,
+		},
+	)


I missed this before, but you need to add some logic to handle updates:

diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go index 3aca7adc..5b581376 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -1305,6 +1305,7 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv Image: container.Image, ImagePullPolicy: container.ImagePullPolicy, Name: container.Name, + Ports: container.Ports, LivenessProbe: hashableProbe(container.LivenessProbe), ReadinessProbe: hashableProbe(container.ReadinessProbe), StartupProbe: hashableProbe(container.StartupProbe), @@ -1520,6 +1521,7 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv updated.Spec.Template.Spec.NodeSelector = expected.Spec.Template.Spec.NodeSelector updated.Spec.Template.Spec.Containers[0].Env = expected.Spec.Template.Spec.Containers[0].Env updated.Spec.Template.Spec.Containers[0].Image = expected.Spec.Template.Spec.Containers[0].Image + updated.Spec.Template.Spec.Containers[0].Ports = expected.Spec.Template.Spec.Containers[0].Ports updated.Spec.Template.Spec.Containers[0].LivenessProbe = expected.Spec.Template.Spec.Containers[0].LivenessProbe updated.Spec.Template.Spec.Containers[0].ReadinessProbe = expected.Spec.Template.Spec.Containers[0].ReadinessProbe updated.Spec.Template.Spec.Containers[0].StartupProbe = expected.Spec.Template.Spec.Containers[0].StartupProbe diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go index 7e5ba9c8..df176ce2 100644 --- a/pkg/operator/controller/ingress/deployment_test.go +++ b/pkg/operator/controller/ingress/deployment_test.go @@ -1163,6 +1163,15 @@ func TestDeploymentConfigChanged(t *testing.T) { }, expect: false, }, + { + description: "if ports are changed", + mutate: func(deployment *appsv1.Deployment) { + deployment.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort = int32(8080) + deployment.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort = int32(8443) + deployment.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort = int32(8936) + }, + expect: true, + }, { description: "if probe values are set to default values", mutate: func(deployment *appsv1.Deployment) { @@ -1335,6 +1344,20 @@ func TestDeploymentConfigChanged(t *testing.T) { }, }, Image: "openshift/origin-cluster-ingress-operator:v4.0", + Ports: []corev1.ContainerPort{ + { + Name: "http", + ContainerPort: int32(80), + }, + { + Name: "https", + ContainerPort: 443, + }, + { + Name: "metrics", + ContainerPort: 1936, + }, + }, LivenessProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{

arjunrn · 2022-04-04T12:01:10Z

/retest-required

alebedev87 · 2022-04-04T13:25:49Z

/unhold

openshift-bot · 2022-04-25T13:19:50Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-04-25T15:43:48Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-04-25T16:07:48Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-04-25T16:19:50Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2022-04-25T16:25:49Z

/hold

alebedev87 · 2022-04-25T20:23:30Z

Doc PR: openshift/openshift-docs#44435

/label docs-approved

FYI @Amrita42

alebedev87 · 2022-04-25T20:25:05Z

Reviewed the PR after the rebase, LGTM
/unhold

CFields651 · 2022-04-25T21:36:24Z

/label px-approved

openshift-bot · 2022-04-26T01:26:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

When the load balancer strategy is hostnetworking allow the user to specify which ports on the host the pods should use. Also fixup the container ports.

alebedev87 · 2022-04-26T11:24:48Z

/lgtm

openshift-ci · 2022-04-26T11:25:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87, arjunrn, Miciah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,alebedev87]

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

openshift-bot · 2022-04-26T11:44:31Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-04-26T12:46:32Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-04-26T14:05:44Z

@arjunrn: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

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 frobware and rfredette January 14, 2022 13:51

arjunrn force-pushed the ingress-host-bind-ports branch 2 times, most recently from 56e039c to e96b234 Compare January 19, 2022 10:03

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2022

arjunrn force-pushed the ingress-host-bind-ports branch from e96b234 to b23aa47 Compare January 27, 2022 13:10

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2022

arjunrn changed the title ~~[WIP] Updated dependencies to upstream k/k v1.23.0~~ Specify host port for host networking strategy 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 3 times, most recently from 2783f49 to 92f526e Compare March 28, 2022 18:28

Miciah reviewed Mar 29, 2022

View reviewed changes

arjunrn force-pushed the ingress-host-bind-ports branch from 92f526e to 9c9c52c Compare March 29, 2022 09:02

alebedev87 reviewed Mar 29, 2022

View reviewed changes

pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved

arjunrn force-pushed the ingress-host-bind-ports branch 2 times, most recently from 7a57f2d to 916a2d5 Compare April 1, 2022 09:36

openshift-ci bot assigned alebedev87 Apr 1, 2022

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 1, 2022

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

Miciah reviewed Apr 2, 2022

View reviewed changes

arjunrn force-pushed the ingress-host-bind-ports branch from 916a2d5 to dff1967 Compare April 4, 2022 08:56

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

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 25, 2022

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

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Apr 25, 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 Apr 25, 2022

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

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2022

Specify host port for host networking strategy

af653f9

When the load balancer strategy is hostnetworking allow the user to specify which ports on the host the pods should use. Also fixup the container ports.

arjunrn force-pushed the ingress-host-bind-ports branch from 10abb90 to af653f9 Compare April 26, 2022 08:02

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 26, 2022

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

openshift-merge-robot merged commit 8f11619 into openshift:master Apr 26, 2022

arjunrn deleted the ingress-host-bind-ports branch April 26, 2022 15:00

Miciah mentioned this pull request May 3, 2022

Bug 2081447: desiredRouterDeployment: Fix for port defaulting #753

Merged

Miciah mentioned this pull request Jun 14, 2022

Bug 2095229: desiredRouterDeployment: Check for nil hostNetwork #780

Merged

Miciah mentioned this pull request Dec 10, 2022

OCPBUGS-4573: Target metrics port by name in internal service #864

Merged

Conversation

arjunrn commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Miciah Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

arjunrn Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alebedev87 commented Apr 1, 2022

Uh oh!

alebedev87 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

arjunrn commented Apr 4, 2022

Uh oh!

alebedev87 commented Apr 4, 2022

Uh oh!

openshift-bot commented Apr 25, 2022

Uh oh!

openshift-bot commented Apr 25, 2022

Uh oh!

openshift-bot commented Apr 25, 2022

Uh oh!

openshift-bot commented Apr 25, 2022

Uh oh!

Miciah commented Apr 25, 2022

Uh oh!

alebedev87 commented Apr 25, 2022

Uh oh!

alebedev87 commented Apr 25, 2022

Uh oh!

CFields651 commented Apr 25, 2022

Uh oh!

openshift-bot commented Apr 26, 2022

Uh oh!

alebedev87 commented Apr 26, 2022

Uh oh!

openshift-ci bot commented Apr 26, 2022

Uh oh!

openshift-bot commented Apr 26, 2022

Uh oh!

openshift-bot commented Apr 26, 2022

Uh oh!

openshift-ci bot commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

arjunrn commented Jan 14, 2022 •

edited

Loading

alebedev87 commented Apr 1, 2022 •

edited

Loading