Skip to content

Specify host port for host networking strategy #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Apr 26, 2022
Merged

Specify host port for host networking strategy #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions assets/router/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,8 @@ spec:
# See https://bugzilla.redhat.com/2007246
allowPrivilegeEscalation: true
terminationMessagePolicy: FallbackToLogsOnError
ports:
- name: http
containerPort: 80
protocol: TCP
- name: https
containerPort: 443
protocol: TCP
- name: metrics
containerPort: 1936
protocol: TCP
# Merged at runtime.
env:
# stats username and password are generated at runtime
- name: STATS_PORT
value: "1936"
- name: ROUTER_SERVICE_NAMESPACE
value: openshift-ingress
- name: DEFAULT_CERTIFICATE_DIR
Expand Down
8 changes: 4 additions & 4 deletions pkg/manifests/bindata.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 38 additions & 4 deletions pkg/operator/controller/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ const (

routerDefaultHeaderBufferSize = 32768
routerDefaultHeaderBufferMaxRewriteSize = 8192
routerDefaultHostNetworkHTTPPort = 80
routerDefaultHostNetworkHTTPSPort = 443
routerDefaultHostNetworkStatsPort = 1936
)

var (
Expand Down Expand Up @@ -385,6 +388,17 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig
if effectiveStrategy.HostNetwork == nil {
effectiveStrategy.HostNetwork = &operatorv1.HostNetworkStrategy{}
}
// explicitly set the default ports if some of them are omitted
if effectiveStrategy.HostNetwork.HTTPPort == 0 {
Copy link
Contributor

@frobware frobware Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

It's not clear why we have defaults here and in the API.
If the API allowed 0 we would have defaults in a single place. How would we know to change the code here if we change the API's current defaults?

Copy link
Contributor

@frobware frobware Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel

I no longer have any concerns about defaults, particularly since openshift/api#1175 merged.

effectiveStrategy.HostNetwork.HTTPPort = routerDefaultHostNetworkHTTPPort
}
if effectiveStrategy.HostNetwork.HTTPSPort == 0 {
effectiveStrategy.HostNetwork.HTTPSPort = routerDefaultHostNetworkHTTPSPort
}
if effectiveStrategy.HostNetwork.StatsPort == 0 {
effectiveStrategy.HostNetwork.StatsPort = routerDefaultHostNetworkStatsPort
}

if effectiveStrategy.HostNetwork.Protocol == operatorv1.DefaultProtocol {
effectiveStrategy.HostNetwork.Protocol = operatorv1.TCPProtocol
}
Expand Down Expand Up @@ -441,16 +455,36 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig
return true
}
case operatorv1.HostNetworkStrategyType:
// Update if PROXY protocol is turned on or off.
if ic.Status.EndpointPublishingStrategy.HostNetwork == nil {
ic.Status.EndpointPublishingStrategy.HostNetwork = &operatorv1.HostNetworkStrategy{}
}

statusHN := ic.Status.EndpointPublishingStrategy.HostNetwork
specHN := effectiveStrategy.HostNetwork
if specHN != nil && specHN.Protocol != statusHN.Protocol {
statusHN.Protocol = specHN.Protocol
return true

var changed bool
if specHN != nil {
// Update if PROXY protocol is turned on or off.
if specHN.Protocol != statusHN.Protocol {
statusHN.Protocol = specHN.Protocol
changed = true
}

// Update if ports have been changed.
if specHN.HTTPPort != statusHN.HTTPPort {
statusHN.HTTPPort = specHN.HTTPPort
changed = true
}
if specHN.HTTPSPort != statusHN.HTTPSPort {
statusHN.HTTPSPort = specHN.HTTPSPort
changed = true
}
if specHN.StatsPort != statusHN.StatsPort {
statusHN.StatsPort = specHN.StatsPort
changed = true
}
}
return changed
}

return false
Expand Down
45 changes: 43 additions & 2 deletions pkg/operator/controller/ingress/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,10 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.HostNetworkStrategyType,
HostNetwork: &operatorv1.HostNetworkStrategy{
Protocol: operatorv1.TCPProtocol,
Protocol: operatorv1.TCPProtocol,
HTTPPort: 80,
HTTPSPort: 443,
StatsPort: 1936,
},
},
},
Expand Down Expand Up @@ -301,7 +304,21 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
return &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.HostNetworkStrategyType,
HostNetwork: &operatorv1.HostNetworkStrategy{
Protocol: proto,
Protocol: proto,
HTTPPort: 80,
HTTPSPort: 443,
StatsPort: 1936,
},
}
}
customHostNetwork = func(httpPort, httpsPort, statsPort int32, protocol operatorv1.IngressControllerProtocol) *operatorv1.EndpointPublishingStrategy {
return &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.HostNetworkStrategyType,
HostNetwork: &operatorv1.HostNetworkStrategy{
Protocol: operatorv1.TCPProtocol,
HTTPPort: httpPort,
HTTPSPort: httpsPort,
StatsPort: statsPort,
},
}
}
Expand Down Expand Up @@ -410,6 +427,30 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
expectedResult: true,
expectedIC: makeIC(spec(hostNetwork(operatorv1.TCPProtocol)), status(hostNetwork(operatorv1.TCPProtocol))),
},
{
name: "hostnetwork ports changed",
ic: makeIC(spec(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol)), status(hostNetwork(operatorv1.TCPProtocol))),
expectedResult: true,
expectedIC: makeIC(spec(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol)), status(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol))),
},
{
name: "hostnetwork ports changed, with status null",
ic: makeIC(spec(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol)), status(hostNetworkWithNull())),
expectedResult: true,
expectedIC: makeIC(spec(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol)), status(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol))),
},
{
name: "hostnetwork ports removed",
ic: makeIC(spec(hostNetwork(operatorv1.TCPProtocol)), status(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol))),
expectedResult: true,
expectedIC: makeIC(spec(hostNetwork(operatorv1.TCPProtocol)), status(hostNetwork(operatorv1.TCPProtocol))),
},
{
name: "hostnetwork ports removed, with spec null",
ic: makeIC(spec(hostNetworkWithNull()), status(customHostNetwork(8080, 8443, 8136, operatorv1.TCPProtocol))),
expectedResult: true,
expectedIC: makeIC(spec(hostNetworkWithNull()), status(hostNetwork(operatorv1.TCPProtocol))),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
Loading