Merge pull request #585 from gcs278/OCPBUGS-26498-upgradeable-status-4.15

[release-4.15] OCPBUGS-28928: Upgrade Validation plugin for SHA1 certs

Author: openshift-merge-bot[bot]

.gitleaks.toml

@@ -0,0 +1,11 @@
+[allowlist]
+  description = "Global Allowlist"
+
+  # Ignore based on any subset of the file path
+  paths = [
+    # Ignore all example certs used for testing
+    '''pkg\/router\/routeapihelpers\/validation_test.go$''',
+    '''pkg\/router\/template\/plugin_test.go$''',
+    '''pkg\/router\/template\/router_test.go$''',
+  ]
+       

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/gocarina/gocsv v0.0.0-20190927101021-3ecffd272576
 	github.com/google/go-cmp v0.5.9
 	github.com/haproxytech/config-parser/v4 v4.0.0-rc1
-	github.com/openshift/api v0.0.0-20230807132801-600991d550ac
+	github.com/openshift/api v0.0.0-20240522145529-93d6bda14341
 	github.com/openshift/client-go v0.0.0-20230120202327-72f107311084
 	github.com/openshift/library-go v0.0.0-20230120202744-256994f916c4
 	github.com/prometheus/client_golang v1.16.0

go.sum

@@ -245,8 +245,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
 github.com/onsi/ginkgo/v2 v2.9.4 h1:xR7vG4IXt5RWx6FfIjyAtsoMAtnc3C/rFXBBd2AjZwE=
 github.com/onsi/gomega v1.27.6 h1:ENqfyGeS5AX/rlXDd/ETokDz93u0YufY1Pgxuy/PvWE=
-github.com/openshift/api v0.0.0-20230807132801-600991d550ac h1:HqT8MmYGXiUGUW0BjygTGOOvqO2wIsTaG3q8nboJyPY=
-github.com/openshift/api v0.0.0-20230807132801-600991d550ac/go.mod h1:yimSGmjsI+XF1mr+AKBs2//fSXIOhhetHGbMlBEfXbs=
+github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 h1:JQpzgk+p24rkgNbNsrNR0yLm63WTKapuT60INU5BqT8=
+github.com/openshift/api v0.0.0-20240522145529-93d6bda14341/go.mod h1:qNtV0315F+f8ld52TLtPvrfivZpdimOzTi3kn9IVbtU=
 github.com/openshift/client-go v0.0.0-20230120202327-72f107311084 h1:66uaqNwA+qYyQDwsMWUfjjau8ezmg1dzCqub13KZOcE=
 github.com/openshift/client-go v0.0.0-20230120202327-72f107311084/go.mod h1:M3h9m001PWac3eAudGG3isUud6yBjr5XpzLYLLTlHKo=
 github.com/openshift/library-go v0.0.0-20230120202744-256994f916c4 h1:cFYg18OROQMHlrGWL9HpV1elDKbnRFLz/ED5VvP3qvw=

pkg/cmd/infra/router/router.go

@@ -68,6 +68,11 @@ type RouterSelection struct {
 
 	ExtendedValidation bool
 
+	UpgradeValidation bool
+
+	UpgradeValidationForceAddCondition    bool
+	UpgradeValidationForceRemoveCondition bool
+
 	ListenAddr string
 
 	// WatchEndpoints when true will watch Endpoints instead of
@@ -94,7 +99,10 @@ func (o *RouterSelection) Bind(flag *pflag.FlagSet) {
 	flag.StringSliceVar(&o.AllowedDomains, "allowed-domains", envVarAsStrings("ROUTER_ALLOWED_DOMAINS", "", ","), "List of comma separated domains to allow in routes. If specified, only the domains in this list will be allowed routes. Note that domains in the denied list take precedence over the ones in the allowed list")
 	flag.BoolVar(&o.AllowWildcardRoutes, "allow-wildcard-routes", isTrue(env("ROUTER_ALLOW_WILDCARD_ROUTES", "")), "Allow wildcard host names for routes")
 	flag.BoolVar(&o.DisableNamespaceOwnershipCheck, "disable-namespace-ownership-check", isTrue(env("ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK", "")), "Disables the namespace ownership checks for a route host with different paths or for overlapping host names in the case of wildcard routes. Please be aware that if namespace ownership checks are disabled, routes in a different namespace can use this mechanism to 'steal' sub-paths for existing domains. This is only safe if route creation privileges are restricted, or if all the users can be trusted.")
-	flag.BoolVar(&o.ExtendedValidation, "extended-validation", isTrue(env("EXTENDED_VALIDATION", "true")), "If set, then an additional extended validation step is performed on all routes admitted in by this router. Defaults to true and enables the extended validation checks.")
+	flag.BoolVar(&o.ExtendedValidation, "extended-validation", isTrue(env("EXTENDED_VALIDATION", "true")), "If set, then an additional extended validation step is performed on all routes processed by this router. Defaults to true and enables the extended validation checks.")
+	flag.BoolVar(&o.UpgradeValidation, "upgrade-validation", isTrue(env("UPGRADE_VALIDATION", "true")), "If set, then an additional upgrade validation step is performed on all routes processed by this router. Defaults to true and enables the upgrade validation checks.")
+	flag.BoolVar(&o.UpgradeValidationForceAddCondition, "debug-upgrade-validation-force-add-condition", isTrue(env("DEBUG_UPGRADE_VALIDATION_FORCE_ADD_CONDITION", "")), "If set, then the upgrade validation plugin will forcibly add the UnservableInFutureVersions condition. For testing purposes only.")
+	flag.BoolVar(&o.UpgradeValidationForceRemoveCondition, "debug-upgrade-validation-force-remove-condition", isTrue(env("DEBUG_UPGRADE_VALIDATION_FORCE_REMOVE_CONDITION", "")), "If set, then the upgrade validation plugin will forcibly remove the UnservableInFutureVersions condition. For testing purposes only.")
 	flag.Bool("enable-ingress", false, "Enable configuration via ingress resources.")
 	flag.MarkDeprecated("enable-ingress", "Ingress resources are now synchronized to routes automatically.")
 	flag.StringVar(&o.ListenAddr, "listen-addr", env("ROUTER_LISTEN_ADDR", ""), "The name of an interface to listen on to expose metrics and health checking. If not specified, will not listen. Overrides stats port.")

pkg/cmd/infra/router/template.go

@@ -785,7 +785,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 	factory.RouteModifierFn = o.RouteUpdate
 
 	var plugin router.Plugin = templatePlugin
-	var recorder controller.RejectionRecorder = controller.LogRejections
+	var recorder controller.RouteStatusRecorder = controller.LogRejections
 	if o.UpdateStatus {
 		lease := writerlease.New(time.Minute, 3*time.Second)
 		go lease.Run(stopCh)
@@ -798,6 +798,9 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 		recorder = status
 		plugin = status
 	}
+	if o.UpgradeValidation {
+		plugin = controller.NewUpgradeValidation(plugin, recorder, o.UpgradeValidationForceAddCondition, o.UpgradeValidationForceRemoveCondition)
+	}
 	if o.ExtendedValidation {
 		plugin = controller.NewExtendedValidator(plugin, recorder)
 	}

pkg/router/controller/contention.go

@@ -7,9 +7,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/cache"
 
-	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
-
 	routev1 "github.com/openshift/api/route/v1"
 )
 
@@ -24,14 +21,15 @@ type ContentionTracker interface {
 	// expected state of the object at this time and may be used by the tracker to
 	// determine if the most recent update was a contention. This method does not
 	// update the state of the tracker.
-	IsChangeContended(id string, now time.Time, current *routev1.RouteIngress) bool
+	IsChangeContended(id contentionKey, now time.Time, current *routev1.RouteIngress) bool
 	// Clear informs the tracker that the provided ingress state was confirmed to
 	// match the current state of this process. If a subsequent call to IsChangeContended
 	// is made within the expiration window, the object will be considered as contended.
-	Clear(id string, current *routev1.RouteIngress)
+	Clear(id contentionKey, current *routev1.RouteIngress)
 }
 
 type elementState int
+type contentionKey string
 
 const (
 	stateCandidate elementState = iota
@@ -57,7 +55,7 @@ type SimpleContentionTracker struct {
 
 	lock        sync.Mutex
 	contentions int
-	ids         map[string]trackerElement
+	ids         map[contentionKey]trackerElement
 }
 
 // NewSimpleContentionTracker creates a ContentionTracker that will prevent writing
@@ -74,7 +72,7 @@ func NewSimpleContentionTracker(informer cache.SharedInformer, routerName string
 		expires:        interval,
 		maxContentions: 5,
 
-		ids: make(map[string]trackerElement),
+		ids: make(map[contentionKey]trackerElement),
 	}
 }
 
@@ -104,7 +102,7 @@ func (t *SimpleContentionTracker) Run(stopCh <-chan struct{}) {
 				return
 			}
 			if ingress := ingressChanged(oldRoute, route, t.routerName); ingress != nil {
-				t.Changed(string(route.UID), ingress)
+				t.Changed(contentionKey(route.UID), ingress)
 			}
 		},
 	})
@@ -160,7 +158,7 @@ func (t *SimpleContentionTracker) flush() {
 // a separate goroutine and may have seen newer events than the current route controller
 // plugins, so we don't do direct time comparisons. Instead we count edge transitions on
 // a given id.
-func (t *SimpleContentionTracker) Changed(id string, current *routev1.RouteIngress) {
+func (t *SimpleContentionTracker) Changed(id contentionKey, current *routev1.RouteIngress) {
 	t.lock.Lock()
 	defer t.lock.Unlock()
 
@@ -207,7 +205,7 @@ func (t *SimpleContentionTracker) Changed(id string, current *routev1.RouteIngre
 	}
 }
 
-func (t *SimpleContentionTracker) IsChangeContended(id string, now time.Time, current *routev1.RouteIngress) bool {
+func (t *SimpleContentionTracker) IsChangeContended(id contentionKey, now time.Time, current *routev1.RouteIngress) bool {
 	t.lock.Lock()
 	defer t.lock.Unlock()
 
@@ -232,7 +230,7 @@ func (t *SimpleContentionTracker) IsChangeContended(id string, now time.Time, cu
 	return false
 }
 
-func (t *SimpleContentionTracker) Clear(id string, current *routev1.RouteIngress) {
+func (t *SimpleContentionTracker) Clear(id contentionKey, current *routev1.RouteIngress) {
 	t.lock.Lock()
 	defer t.lock.Unlock()
 
@@ -245,11 +243,48 @@ func (t *SimpleContentionTracker) Clear(id string, current *routev1.RouteIngress
 	t.ids[id] = last
 }
 
+// ingressEqual compares two route ingresses and returns a bool if they are equivalent.
 func ingressEqual(a, b *routev1.RouteIngress) bool {
-	// In addition to the RouteIngress' string fields, compare the available admission condition to determine
-	// if the given ingress' are equal. See https://bugzilla.redhat.com/show_bug.cgi?id=1908389.
-	return a.Host == b.Host && a.RouterCanonicalHostname == b.RouterCanonicalHostname && a.WildcardPolicy == b.WildcardPolicy && a.RouterName == b.RouterName &&
-		cmp.Equal(findCondition(a, routev1.RouteAdmitted), findCondition(b, routev1.RouteAdmitted), cmpopts.IgnoreFields(routev1.RouteIngressCondition{}, "LastTransitionTime"))
+	// In addition to the RouteIngress' string fields, compare all conditions to determine
+	// if the given ingress' are equal.
+	return a.Host == b.Host &&
+		a.RouterCanonicalHostname == b.RouterCanonicalHostname &&
+		a.WildcardPolicy == b.WildcardPolicy &&
+		a.RouterName == b.RouterName &&
+		ingressConditionsEqual(a.Conditions, b.Conditions)
+}
+
+// ingressConditionsEqual determines if the route ingress conditions are equal,
+// while ignoring LastTransitionTime.
+func ingressConditionsEqual(a, b []routev1.RouteIngressCondition) bool {
+	if len(a) != len(b) {
+		return false
+	}
+
+	// Compare each condition in a with every condition in b.
+	// Given the current max of only two conditions, nested loops are more efficient than sorting.
+	for i := 0; i < len(a); i++ {
+		matchFound := false
+		for j := 0; j < len(b); j++ {
+			if conditionsEqual(&a[i], &b[j]) {
+				matchFound = true
+				break
+			}
+		}
+		if !matchFound {
+			return false
+		}
+	}
+
+	return true
+}
+
+// conditionsEqual compares two RouteIngressConditions, ignoring LastTransitionTime.
+func conditionsEqual(a, b *routev1.RouteIngressCondition) bool {
+	return a.Type == b.Type &&
+		a.Status == b.Status &&
+		a.Reason == b.Reason &&
+		a.Message == b.Message
 }
 
 func ingressConditionTouched(ingress *routev1.RouteIngress) *metav1.Time {

pkg/router/controller/extended_validator.go

@@ -19,14 +19,14 @@ type ExtendedValidator struct {
 	// plugin is the next plugin in the chain.
 	plugin router.Plugin
 
-	// recorder is an interface for indicating route rejections.
-	recorder RejectionRecorder
+	// recorder is an interface for indicating route status.
+	recorder RouteStatusRecorder
 }
 
 // NewExtendedValidator creates a plugin wrapper that ensures only routes that
 // pass extended validation are relayed to the next plugin in the chain.
-// Recorder is an interface for indicating why a route was rejected.
-func NewExtendedValidator(plugin router.Plugin, recorder RejectionRecorder) *ExtendedValidator {
+// Recorder is an interface for indicating route status updates.
+func NewExtendedValidator(plugin router.Plugin, recorder RouteStatusRecorder) *ExtendedValidator {
 	return &ExtendedValidator{
 		plugin:   plugin,
 		recorder: recorder,
@@ -45,6 +45,7 @@ func (p *ExtendedValidator) HandleEndpoints(eventType watch.EventType, endpoints
 
 // HandleRoute processes watch events on the Route resource.
 func (p *ExtendedValidator) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
+	log.V(10).Info("HandleRoute: ExtendedValidator")
 	// Check if previously seen route and its Spec is unchanged.
 	routeName := routeNameKey(route)
 	if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil {

pkg/router/controller/host_admitter.go

@@ -78,7 +78,7 @@ type HostAdmitter struct {
 	admitter RouteAdmissionFunc
 
 	// recorder is an interface for indicating route rejections.
-	recorder RejectionRecorder
+	recorder RouteStatusRecorder
 
 	// allowWildcardRoutes enables wildcard route support.
 	allowWildcardRoutes bool
@@ -98,8 +98,8 @@ type HostAdmitter struct {
 
 // NewHostAdmitter creates a plugin wrapper that checks whether or not to
 // admit routes and relay them to the next plugin in the chain.
-// Recorder is an interface for indicating why a route was rejected.
-func NewHostAdmitter(plugin router.Plugin, fn RouteAdmissionFunc, allowWildcards, disableNamespaceCheck bool, recorder RejectionRecorder) *HostAdmitter {
+// Recorder is an interface for indicating route status updates..
+func NewHostAdmitter(plugin router.Plugin, fn RouteAdmissionFunc, allowWildcards, disableNamespaceCheck bool, recorder RouteStatusRecorder) *HostAdmitter {
 	return &HostAdmitter{
 		plugin:   plugin,
 		admitter: fn,
@@ -126,6 +126,7 @@ func (p *HostAdmitter) HandleEndpoints(eventType watch.EventType, endpoints *kap
 
 // HandleRoute processes watch events on the Route resource.
 func (p *HostAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
+	log.V(10).Info("HandleRoute: HostAdmitter")
 	if p.allowedNamespaces != nil && !p.allowedNamespaces.Has(route.Namespace) {
 		// Ignore routes we don't need to "service" due to namespace
 		// restrictions (ala for sharding).

pkg/router/controller/host_admitter_test.go

@@ -22,19 +22,28 @@ const (
 	BlockedTestDomain = "domain.blocked.test"
 )
 
-type rejectionRecorder struct {
-	rejections map[string]string
+type routeStatusRecorder struct {
+	rejections                 map[string]string
+	unservableInFutureVersions map[string]string
 }
 
-func (_ rejectionRecorder) rejectionKey(route *routev1.Route) string {
+func (_ routeStatusRecorder) rejectionKey(route *routev1.Route) string {
 	return route.Namespace + "-" + route.Name
 }
 
-func (r rejectionRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) {
+func (r routeStatusRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) {
 	r.rejections[r.rejectionKey(route)] = reason
 }
 
-func (r rejectionRecorder) Clear() {
+func (r routeStatusRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) {
+	delete(r.unservableInFutureVersions, r.rejectionKey(route))
+}
+
+func (r routeStatusRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) {
+	r.unservableInFutureVersions[r.rejectionKey(route)] = reason
+}
+
+func (r routeStatusRecorder) Clear() {
 	r.rejections = make(map[string]string)
 }
 
@@ -248,7 +257,7 @@ func TestWildcardHostDeny(t *testing.T) {
 func TestWildcardSubDomainOwnership(t *testing.T) {
 	p := &fakePlugin{}
 
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, wildcardAdmitter, true, false, recorder)
 
 	oldest := metav1.Time{Time: time.Now()}
@@ -505,7 +514,7 @@ func TestValidRouteAdmissionFuzzing(t *testing.T) {
 	p := &fakePlugin{}
 
 	admitAll := func(route *routev1.Route) error { return nil }
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, RouteAdmissionFunc(admitAll), true, false, recorder)
 
 	oldest := metav1.Time{Time: time.Now()}
@@ -602,7 +611,7 @@ func TestInvalidRouteAdmissionFuzzing(t *testing.T) {
 	p := &fakePlugin{}
 
 	admitAll := func(route *routev1.Route) error { return nil }
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, RouteAdmissionFunc(admitAll), true, false, recorder)
 
 	oldest := metav1.Time{Time: time.Now()}
@@ -787,7 +796,7 @@ func TestStatusWildcardPolicyNoOp(t *testing.T) {
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
 	p := &fakePlugin{}
 	c := fake.NewSimpleClientset()
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, wildcardAdmitter, true, false, recorder)
 	err := admitter.HandleRoute(watch.Added, &routev1.Route{
 		ObjectMeta: metav1.ObjectMeta{Name: "wild", Namespace: "thing", UID: types.UID("uid8")},
@@ -825,7 +834,7 @@ func TestStatusWildcardPolicyNotAllowedNoOp(t *testing.T) {
 	touched := metav1.Time{Time: now.Add(-time.Minute)}
 	p := &fakePlugin{}
 	c := fake.NewSimpleClientset()
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, wildcardAdmitter, false, false, recorder)
 	err := admitter.HandleRoute(watch.Added, &routev1.Route{
 		ObjectMeta: metav1.ObjectMeta{Name: "wild", Namespace: "thing", UID: types.UID("uid8")},
@@ -862,7 +871,7 @@ func TestDisableOwnershipChecksFuzzing(t *testing.T) {
 	p := &fakePlugin{}
 
 	admitAll := func(route *routev1.Route) error { return nil }
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	uniqueHostPlugin := NewUniqueHost(p, true, recorder)
 	admitter := NewHostAdmitter(uniqueHostPlugin, RouteAdmissionFunc(admitAll), true, true, recorder)
 
@@ -1026,7 +1035,7 @@ func TestDisableOwnershipChecksFuzzing(t *testing.T) {
 
 func TestHandleNamespaceProcessing(t *testing.T) {
 	p := &fakePlugin{}
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, wildcardAdmitter, true, false, recorder)
 
 	// Set namespaces handled in the host admitter plugin, the fakePlugin in
@@ -1148,7 +1157,7 @@ func TestHandleNamespaceProcessing(t *testing.T) {
 func TestWildcardPathRoutesWithoutNSCheckResyncs(t *testing.T) {
 	p := &fakePlugin{}
 
-	recorder := rejectionRecorder{rejections: make(map[string]string)}
+	recorder := routeStatusRecorder{rejections: make(map[string]string)}
 	admitter := NewHostAdmitter(p, wildcardAdmitter, true, true, recorder)
 
 	oldest := metav1.Time{Time: time.Now()}