Commit d6dcbfd7 authored by Miciah Masters's avatar Miciah Masters Committed by Miciah Dashiel Butler Masters
Browse files

desiredNodePortService: Add port 1936

When creating the NodePort-type service for an ingresscontroller that
specifies the "NodePortService" endpoint publishing strategy type, include
a "metrics" port with port 1936 so that external load balancers can use the
/healthz/ready endpoint.

This commit fixes bug 1881210.

https://bugzilla.redhat.com/show_bug.cgi?id=1881210

* pkg/operator/controller/ingress/nodeport_service.go
(ensureNodePortService): Pass an argument to desiredNodePortService to tell
it not to add a "metrics" port if the service already exists and omits the
port.
(desiredNodePortService): Add a "wantMetricsPort" parameter.  Add port 1936
to the service's ports spec if wantMetricsPort is true.
* pkg/operator/controller/ingress/nodeport_service_test.go
(TestDesiredNodePortService): Add a test case where wantMetricsPort is
true.  Verify that desiredNodePortService returns the expected service.
(TestNodePortServiceChanged): Update the service used for tests to be
consistent with what desiredNodePortService returns.
* test/e2e/operator_test.go
(TestNodePortServiceEndpointPublishingStrategy): Verify that the operator
creates the NodePort-type service with the expected ports.  Verify that the
operator does not add the "metrics" port if it is is removed from the
service.
parent aec00ef3
......@@ -24,13 +24,30 @@ import (
// indicating whether the NodePort service exists, the current NodePort service
// if it does exist, and an error value.
func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (bool, *corev1.Service, error) {
wantService, desired := desiredNodePortService(ic, deploymentRef)
haveService, current, err := r.currentNodePortService(ic)
if err != nil {
return false, nil, err
}
// For compatibility, omit the "metrics" port iff the service already
// exists and doesn't have a "metrics" port. This serves two purposes:
// (1) It avoids exhausting the nodeport range on upgrades if the
// cluster has many nodeport services and few available nodeports.
// (2) It enables the cluster administrator to remove the metrics port
// from an existing nodeport service to avoid exposing the port.
wantMetricsPort := false
if !haveService {
wantMetricsPort = true
} else {
for _, port := range current.Spec.Ports {
if port.Name == "metrics" {
wantMetricsPort = true
}
}
}
wantService, desired := desiredNodePortService(ic, deploymentRef, wantMetricsPort)
switch {
case !wantService && !haveService:
return false, nil, nil
......@@ -63,7 +80,7 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
// desiredNodePortService returns a Boolean indicating whether a NodePort
// service is desired, as well as the NodePort service if one is desired.
func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (bool, *corev1.Service) {
func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, wantMetricsPort bool) (bool, *corev1.Service) {
if ic.Status.EndpointPublishingStrategy.Type != operatorv1.NodePortServiceStrategyType {
return false, nil
}
......@@ -95,11 +112,20 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
Port: int32(443),
TargetPort: intstr.FromString("https"),
},
{
Name: "metrics",
Protocol: corev1.ProtocolTCP,
Port: int32(1936),
TargetPort: intstr.FromString("metrics"),
},
},
Selector: controller.IngressControllerDeploymentPodSelector(ic).MatchLabels,
Type: corev1.ServiceTypeNodePort,
},
}
if !wantMetricsPort {
service.Spec.Ports = service.Spec.Ports[0:2]
}
return true, service
}
......
package ingress
import (
"reflect"
"testing"
operatorv1 "github.com/openshift/api/operator/v1"
......@@ -12,9 +13,20 @@ import (
)
func TestDesiredNodePortService(t *testing.T) {
trueVar := true
deploymentRef := metav1.OwnerReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: "router-default",
UID: "1",
Controller: &trueVar,
}
testCases := []struct {
strategyType operatorv1.EndpointPublishingStrategyType
expect bool
strategyType operatorv1.EndpointPublishingStrategyType
wantMetricsPort bool
expect bool
expectService corev1.Service
}{
{
strategyType: operatorv1.LoadBalancerServiceStrategyType,
......@@ -23,6 +35,83 @@ func TestDesiredNodePortService(t *testing.T) {
{
strategyType: operatorv1.NodePortServiceStrategyType,
expect: true,
expectService: corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "openshift-ingress",
Name: "router-nodeport-default",
Labels: map[string]string{
"app": "router",
"router": "router-nodeport-default",
"ingresscontroller.operator.openshift.io/owning-ingresscontroller": "default",
},
OwnerReferences: []metav1.OwnerReference{deploymentRef},
},
Spec: corev1.ServiceSpec{
ExternalTrafficPolicy: "Local",
Ports: []corev1.ServicePort{
{
Name: "http",
Protocol: "TCP",
Port: int32(80),
TargetPort: intstr.FromString("http"),
},
{
Name: "https",
Protocol: "TCP",
Port: int32(443),
TargetPort: intstr.FromString("https"),
},
},
Selector: map[string]string{
"ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default",
},
Type: "NodePort",
},
},
},
{
strategyType: operatorv1.NodePortServiceStrategyType,
wantMetricsPort: true,
expect: true,
expectService: corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "openshift-ingress",
Name: "router-nodeport-default",
Labels: map[string]string{
"app": "router",
"router": "router-nodeport-default",
"ingresscontroller.operator.openshift.io/owning-ingresscontroller": "default",
},
OwnerReferences: []metav1.OwnerReference{deploymentRef},
},
Spec: corev1.ServiceSpec{
ExternalTrafficPolicy: "Local",
Ports: []corev1.ServicePort{
{
Name: "http",
Protocol: "TCP",
Port: int32(80),
TargetPort: intstr.FromString("http"),
},
{
Name: "https",
Protocol: "TCP",
Port: int32(443),
TargetPort: intstr.FromString("https"),
},
{
Name: "metrics",
Protocol: "TCP",
Port: int32(1936),
TargetPort: intstr.FromString("metrics"),
},
},
Selector: map[string]string{
"ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default",
},
Type: "NodePort",
},
},
},
}
......@@ -37,18 +126,11 @@ func TestDesiredNodePortService(t *testing.T) {
},
},
}
trueVar := true
deploymentRef := metav1.OwnerReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: "router-default",
UID: "1",
Controller: &trueVar,
}
want, svc := desiredNodePortService(ic, deploymentRef)
want, svc := desiredNodePortService(ic, deploymentRef, tc.wantMetricsPort)
if want != tc.expect {
t.Errorf("expected desiredNodePortService to return %t for endpoint publishing strategy type %v, got %t, with service %#v", tc.expect, tc.strategyType, want, svc)
} else if tc.expect && !reflect.DeepEqual(svc, &tc.expectService) {
t.Errorf("expected desiredNodePortService to return %#v, got %#v", &tc.expectService, svc)
}
}
}
......@@ -176,6 +258,13 @@ func TestNodePortServiceChanged(t *testing.T) {
Protocol: corev1.ProtocolTCP,
TargetPort: intstr.FromString("https"),
},
{
Name: "metrics",
NodePort: int32(33336),
Port: int32(1936),
Protocol: corev1.ProtocolTCP,
TargetPort: intstr.FromString("metrics"),
},
},
Selector: map[string]string{
"foo": "bar",
......
......@@ -589,7 +589,12 @@ func TestInternalLoadBalancer(t *testing.T) {
// TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller
// with the "NodePortService" endpoint publishing strategy type and verifies
// that the operator creates a router and that the router becomes available.
// that the operator creates a router, that the router becomes available, and
// that the operator creates the expected NodePort-type service.
//
// The test then removes the "metrics" port from the NodePort-type service and
// verifies that the operator does not add the port back. See
// <https://bugzilla.redhat.com/show_bug.cgi?id=1881210>.
func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) {
name := types.NamespacedName{Namespace: operatorNamespace, Name: "nodeport"}
ing := newNodePortController(name, name.Name+"."+dnsConfig.Spec.BaseDomain)
......@@ -608,6 +613,54 @@ func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) {
if err != nil {
t.Errorf("failed to observe expected conditions: %v", err)
}
// Make sure the ingresscontroller has a nodeport service
// with the expected ports.
svcName := controller.NodePortServiceName(ing)
service := &corev1.Service{}
err = wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) {
if err := kclient.Get(context.TODO(), svcName, service); err != nil {
t.Logf("failed to get service %q: %v", svcName, err)
return false, nil
}
return true, nil
})
ports := sets.String{}
for _, port := range service.Spec.Ports {
ports.Insert(port.Name)
}
expectedPorts := sets.NewString("http", "https", "metrics")
if !ports.Equal(expectedPorts) {
t.Fatalf("expected service %q to have ports %v, got %v", svcName, expectedPorts.List(), ports.List())
}
// Delete the "metrics" port and verify that the operator
// does not restore it.
for i, port := range service.Spec.Ports {
if port.Name == "metrics" {
ports := service.Spec.Ports
ports = append(ports[:i], ports[i+1:]...)
service.Spec.Ports = ports
}
}
if err := kclient.Update(context.TODO(), service); err != nil {
t.Fatalf("failed to update service %q: %v", svcName, err)
}
expectedPorts = sets.NewString("http", "https")
wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) {
if err := kclient.Get(context.TODO(), svcName, service); err != nil {
t.Logf("failed to get service %q: %v", svcName, err)
return false, nil
}
ports = sets.String{}
for _, port := range service.Spec.Ports {
ports.Insert(port.Name)
}
return ports.Equal(expectedPorts), nil
})
if !ports.Equal(expectedPorts) {
t.Fatalf("after deleting the \"metrics\" port, expected service %q to have ports %v, got %v", svcName, expectedPorts.List(), ports.List())
}
}
// TestTLSSecurityProfile creates an ingresscontroller with no explicit TLS
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment