Merge pull request #531 from openshift-cherrypick-robot/cherry-pick-5…

…28-to-release-4.9 [release-4.9] Bug 2037944: endpoints checker: check only the custom hostname if configured
openshift · Jun 30, 2022 · c6ab777 · c6ab777
2 parents 48a20f5 + 47f6c2f
commit c6ab777
Show file tree

Hide file tree

Showing 2 changed files with 169 additions and 6 deletions.
diff --git a/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go b/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go
@@ -7,6 +7,7 @@ import (
 	"net"
 	"strconv"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 
 	corev1 "k8s.io/api/core/v1"
@@ -47,7 +48,7 @@ func NewOAuthRouteCheckController(
 	ingressInformer := ingressInformerAllNamespaces.Informer()
 
 	endpointListFunc := func() ([]string, error) {
-		return listOAuthRoutes(routeLister)
+		return listOAuthRoutes(ingressLister, routeLister)
 	}
 
 	getTLSConfigFunc := func() (*tls.Config, error) {
@@ -143,6 +144,7 @@ func listOAuthServices(serviceLister corev1listers.ServiceLister) ([]string, err
 	if err != nil {
 		return nil, err
 	}
+
 	for _, port := range service.Spec.Ports {
 		results = append(results, net.JoinHostPort(service.Spec.ClusterIP, strconv.Itoa(int(port.Port))))
 	}
@@ -152,25 +154,52 @@ func listOAuthServices(serviceLister corev1listers.ServiceLister) ([]string, err
 	return toHealthzURL(results), nil
 }
 
-func listOAuthRoutes(routeLister routev1listers.RouteLister) ([]string, error) {
+func listOAuthRoutes(ingressConfigLister configv1lister.IngressLister, routeLister routev1listers.RouteLister) ([]string, error) {
 	var results []string
+	ingressConfig, err := ingressConfigLister.Get("cluster")
+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve ingress from cache: %w", err)
+	}
+
 	route, err := routeLister.Routes("openshift-authentication").Get("oauth-openshift")
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to retrieve route from cache: %w", err)
+	}
+
+	// retrieve all the hostnames we want to be checking, don't care about any others
+	ingressStatus := common.GetComponentRouteStatus(ingressConfig, "openshift-authentication", "oauth-openshift")
+	if ingressStatus == nil || ingressStatus.CurrentHostnames == nil {
+		return nil, fmt.Errorf("ingress.config/cluster does not yet have status for the \"openshift-authentication/oauth-openshift\" route")
+	}
+	wantedHostnames := sets.NewString()
+	for _, host := range ingressStatus.CurrentHostnames {
+		wantedHostnames.Insert(string(host))
 	}
+
 	for _, ingress := range route.Status.Ingress {
-		if len(ingress.Host) > 0 {
+		if ingressHost := ingress.Host; len(ingressHost) > 0 && wantedHostnames.Has(ingressHost) {
 			for _, condition := range ingress.Conditions {
 				if condition.Type == routev1.RouteAdmitted && condition.Status == corev1.ConditionTrue {
 					results = append(results, ingress.Host)
-					break
+
+					wantedHostnames.Delete(ingressHost)
+					if wantedHostnames.Len() == 0 {
+						break
+					}
 				}
 			}
 		}
 	}
 	if len(results) == 0 {
-		return nil, fmt.Errorf("route \"openshift-authentication/oauth-openshift\": status does not have a host address")
+		return nil, fmt.Errorf("route \"openshift-authentication/oauth-openshift\": status does not have a valid host address")
+	}
+	if wantedHostnames.Len() > 0 {
+		return nil, fmt.Errorf(
+			"route \"openshift-authentication/oauth-openshift\": the following hostnames have not yet been admitted: %v",
+			wantedHostnames.List(), // use sorted list here to avoid infinite looping if condition gets created from this error
+		)
 	}
+
 	return toHealthzURL(results), nil
 }
 

diff --git a/pkg/controllers/oauthendpoints/oauth_endpoints_controller_test.go b/pkg/controllers/oauthendpoints/oauth_endpoints_controller_test.go
@@ -3,6 +3,17 @@ package oauthendpoints
 import (
 	"reflect"
 	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/cache"
+
+	configv1 "github.com/openshift/api/config/v1"
+	routev1 "github.com/openshift/api/route/v1"
+	configv1lister "github.com/openshift/client-go/config/listers/config/v1"
+	routev1listers "github.com/openshift/client-go/route/listers/route/v1"
 )
 
 func Test_toHealthzURL(t *testing.T) {
@@ -25,3 +36,126 @@ func Test_toHealthzURL(t *testing.T) {
 		})
 	}
 }
+
+func Test_listOAuthRoutes(t *testing.T) {
+	tests := []struct {
+		name          string
+		ingressConfig *configv1.Ingress
+		route         *routev1.Route
+		want          []string
+		wantErr       bool
+	}{
+		{
+			name:          "no route",
+			ingressConfig: authIngressConfig("hostname.one"),
+			wantErr:       true,
+		},
+		{
+			name:    "no ingress config",
+			route:   authRoute("hostname.one"),
+			wantErr: true,
+		},
+		{
+			name:  "no ingress config status",
+			route: authRoute("hostname.one"),
+			ingressConfig: &configv1.Ingress{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name:          "no admitted ingress in route",
+			ingressConfig: authIngressConfig("hostname.one"),
+			route:         authRoute(),
+			wantErr:       true,
+		},
+		{
+			name:          "admitted ingress in route different than the one in config",
+			ingressConfig: authIngressConfig("hostname.one"),
+			route:         authRoute("hostname.two"),
+			wantErr:       true,
+		},
+		{
+			name:          "admitted ingress from config among other admitted ingresses",
+			ingressConfig: authIngressConfig("hostname.one"),
+			route:         authRoute("hostname.two", "hostname.tree", "hostname.one", "hostname.four"),
+			want:          []string{"https://hostname.one/healthz"},
+		},
+		{
+			name:          "multiple current hostnames, not all admitted yet",
+			ingressConfig: authIngressConfig("hostname.one", "hostname.five"),
+			route:         authRoute("hostname.two", "hostname.tree", "hostname.one", "hostname.four"),
+			wantErr:       true,
+		},
+		{
+			name:          "multiple current hostnames, all admitted already",
+			ingressConfig: authIngressConfig("hostname.one", "hostname.two"),
+			route:         authRoute("hostname.two", "hostname.tree", "hostname.one", "hostname.four"),
+			want:          []string{"https://hostname.two/healthz", "https://hostname.one/healthz"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			routes := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			ingresses := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			if tt.ingressConfig != nil {
+				require.NoError(t, ingresses.Add(tt.ingressConfig))
+			}
+			if tt.route != nil {
+				require.NoError(t, routes.Add(tt.route))
+			}
+
+			got, err := listOAuthRoutes(configv1lister.NewIngressLister(ingresses), routev1listers.NewRouteLister(routes))
+			if (err != nil) != tt.wantErr {
+				t.Errorf("listOAuthRoutes() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("listOAuthRoutes() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func authRoute(admittedIngressHostnames ...string) *routev1.Route {
+	r := &routev1.Route{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "openshift-authentication",
+			Name:      "oauth-openshift",
+		},
+	}
+
+	for _, host := range admittedIngressHostnames {
+		r.Status.Ingress = append(r.Status.Ingress,
+			routev1.RouteIngress{
+				Host: host,
+				Conditions: []routev1.RouteIngressCondition{
+					{
+						Type:   routev1.RouteAdmitted,
+						Status: corev1.ConditionTrue,
+					},
+				},
+			})
+	}
+
+	return r
+}
+
+func authIngressConfig(currentHostnames ...configv1.Hostname) *configv1.Ingress {
+	return &configv1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Status: configv1.IngressStatus{
+			ComponentRoutes: []configv1.ComponentRouteStatus{
+				{
+					Namespace:        "openshift-authentication",
+					Name:             "oauth-openshift",
+					CurrentHostnames: currentHostnames,
+				},
+			},
+		},
+	}
+}