Merge pull request #28783 from adambkaplan/unauth-webhook-on-upgrade

OCPBUGS-33378: Verify Build Webhooks on Upgrade

go.mod

@@ -44,6 +44,7 @@ require (
 	go.etcd.io/etcd/client/pkg/v3 v3.5.10
 	go.etcd.io/etcd/client/v3 v3.5.10
 	golang.org/x/crypto v0.21.0
+	golang.org/x/mod v0.14.0
 	golang.org/x/net v0.23.0
 	golang.org/x/oauth2 v0.10.0
 	golang.org/x/sync v0.5.0
@@ -251,7 +252,6 @@ require (
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.24.0 // indirect
 	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect
-	golang.org/x/mod v0.14.0 // indirect
 	golang.org/x/sys v0.18.0 // indirect
 	golang.org/x/term v0.18.0 // indirect
 	golang.org/x/text v0.14.0 // indirect

pkg/clusterversion/doc.go

@@ -0,0 +1,2 @@
+// Package clusterversion contains utitlities to access version information for the cluster.
+package clusterversion

pkg/clusterversion/upgrade.go

@@ -0,0 +1,45 @@
+package clusterversion
+
+import (
+	"strings"
+
+	"golang.org/x/mod/semver"
+
+	configv1 "github.com/openshift/api/config/v1"
+)
+
+// IsUpgradedFromMinorVersion returns true if the cluster has been upgraded from or through the given version.
+// This will only check for X.Y version upgrades - it will ignore patch/z-stream versions.
+// Returns false if the input version is not a semver.
+func IsUpgradedFromMinorVersion(version string, cv *configv1.ClusterVersion) bool {
+	fromMajorMinor := majorMinorVersion(version)
+	if !semver.IsValid(fromMajorMinor) {
+		return false
+	}
+
+	beforeOrAtVersionFound := false
+	atOrLaterVersionFound := false
+
+	// History is always ordered from most recent to oldest.
+	for _, history := range cv.Status.History {
+		historyMajorMinor := majorMinorVersion(history.Version)
+		// Version in history can be empty or not a semver. Skip in this case.
+		if !semver.IsValid(historyMajorMinor) {
+			continue
+		}
+		if semver.Compare(historyMajorMinor, fromMajorMinor) >= 0 {
+			atOrLaterVersionFound = true
+		}
+		if semver.Compare(historyMajorMinor, fromMajorMinor) <= 0 {
+			beforeOrAtVersionFound = true
+		}
+	}
+	return beforeOrAtVersionFound && atOrLaterVersionFound
+}
+
+func majorMinorVersion(version string) string {
+	if !strings.HasPrefix(version, "v") {
+		version = "v" + version
+	}
+	return semver.MajorMinor(version)
+}

pkg/clusterversion/upgrade_test.go

@@ -0,0 +1,71 @@
+package clusterversion
+
+import (
+	"testing"
+
+	configv1 "github.com/openshift/api/config/v1"
+)
+
+func TestIsUpgradedFromMinorVersion(t *testing.T) {
+	cases := []struct {
+		Name               string
+		UpgradeFromVersion string
+		VersionHistory     []string
+		Expected           bool
+	}{
+		{
+			Name:               "no history",
+			UpgradeFromVersion: "4.15",
+			Expected:           false,
+		},
+		{
+			Name:               "upgraded to 4.16 from 4.15",
+			UpgradeFromVersion: "4.15",
+			VersionHistory:     []string{"4.16.0", "4.15.9", "4.15.7", "4.15.2"},
+			Expected:           true,
+		},
+		{
+			Name:               "upgraded to 4.16 from 4.14",
+			UpgradeFromVersion: "4.15",
+			VersionHistory:     []string{"4.16.0", "4.15.9", "4.15.7", "4.15.2", "4.14.9"},
+			Expected:           true,
+		},
+		{
+			Name:               "skip odd minor version",
+			UpgradeFromVersion: "4.15",
+			VersionHistory:     []string{"4.16.0", "4.14.9", "4.14.2", "4.12.14", "4.12.8"},
+			Expected:           true,
+		},
+		{
+			Name:               "not reached upgrade",
+			UpgradeFromVersion: "4.15",
+			VersionHistory:     []string{"4.14.0", "4.13.9", "4.13.2", "4.12.14", "4.12.8"},
+			Expected:           false,
+		},
+		{
+			Name:               "invalid version",
+			UpgradeFromVersion: "bad-data",
+			VersionHistory:     []string{"4.16.0", "4.15.9", "4.15.7"},
+			Expected:           false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			history := []configv1.UpdateHistory{}
+			for _, version := range tc.VersionHistory {
+				history = append(history, configv1.UpdateHistory{
+					Version: version,
+				})
+			}
+			cv := &configv1.ClusterVersion{
+				Status: configv1.ClusterVersionStatus{
+					History: history,
+				},
+			}
+			upgraded := IsUpgradedFromMinorVersion(tc.UpgradeFromVersion, cv)
+			if upgraded != tc.Expected {
+				t.Errorf("expected %v, got %v", tc.Expected, upgraded)
+			}
+		})
+	}
+}

test/extended/builds/start.go

@@ -14,6 +14,8 @@ import (
 	o "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	e2e "k8s.io/kubernetes/test/e2e/framework"
@@ -409,6 +411,43 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] starting a build using CL
 			})
 
 			g.Describe("start a build via a webhook", func() {
+
+				// AUTH-509: Webhooks do not allow unauthenticated requests by default.
+				// Create a role binding which allows unauthenticated webhooks.
+				g.BeforeEach(func() {
+					ctx := context.Background()
+					adminRoleBindingsClient := oc.AdminKubeClient().RbacV1().RoleBindings(oc.Namespace())
+					_, err := adminRoleBindingsClient.Get(ctx, "webooks-unauth", metav1.GetOptions{})
+					if apierrors.IsNotFound(err) {
+						unauthWebhooksRB := &rbacv1.RoleBinding{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "webooks-unauth",
+							},
+							RoleRef: rbacv1.RoleRef{
+								APIGroup: "rbac.authorization.k8s.io",
+								Kind:     "ClusterRole",
+								Name:     "system:webhook",
+							},
+							Subjects: []rbacv1.Subject{
+								{
+									APIGroup: "rbac.authorization.k8s.io",
+									Kind:     "Group",
+									Name:     "system:authenticated",
+								},
+								{
+									APIGroup: "rbac.authorization.k8s.io",
+									Kind:     "Group",
+									Name:     "system:unauthenticated",
+								},
+							},
+						}
+						_, err = adminRoleBindingsClient.Create(ctx, unauthWebhooksRB, metav1.CreateOptions{})
+						o.Expect(err).NotTo(o.HaveOccurred(), "creating webhook role binding")
+						return
+					}
+					o.Expect(err).NotTo(o.HaveOccurred(), "checking if webhook role binding exists")
+				})
+
 				g.It("should be able to start builds via the webhook with valid secrets and fail with invalid secrets [apigroup:build.openshift.io]", func() {
 					g.By("clearing existing builds")
 					_, err := oc.Run("delete").Args("builds", "--all").Output()