Skip to content

Commit

Permalink
Revert "Merge pull request openshift#28750 from adambkaplan/unauthent…
Browse files Browse the repository at this point in the history 
…icated-webhook-bc"

This reverts commit 75f7e06, reversing
changes made to 15c3521.
  • Loading branch information
@stbenjam
stbenjam committed May 8, 2024
1 parent 3f62412 commit e3c844d
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 97 deletions.
39 changes: 0 additions & 39 deletions test/extended/builds/start.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ 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"
Expand Down Expand Up @@ -411,43 +409,6 @@ 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()
Expand Down
73 changes: 15 additions & 58 deletions test/extended/builds/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,10 @@ package builds
import (
"bytes"
"context"
"crypto/tls"
"encoding/json"
"fmt"
"io"
"io/ioutil"
"net/http"
"os"
"time"

g "github.com/onsi/ginkgo/v2"
Expand All @@ -27,10 +25,7 @@ import (

var _ = g.Describe("[sig-builds][Feature:Builds][webhook]", func() {
defer g.GinkgoRecover()

var (
oc = exutil.NewCLIWithPodSecurityLevel("build-webhooks", admissionapi.LevelBaseline)
)
oc := exutil.NewCLIWithPodSecurityLevel("build-webhooks", admissionapi.LevelBaseline)

g.It("TestWebhook [apigroup:build.openshift.io][apigroup:image.openshift.io]", func() {
TestWebhook(g.GinkgoT(), oc)
Expand All @@ -48,7 +43,6 @@ var _ = g.Describe("[sig-builds][Feature:Builds][webhook]", func() {

func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client

// create buildconfig
buildConfig := mockBuildConfigImageParms("originalimage", "imagestream", "validtag")
Expand All @@ -59,12 +53,10 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
// Bug #1752581: reduce number of URLs per test case
// OCP 4.2 tests on GCP had high flake levels because namespaces took too long to tear down
tests := []struct {
Name string
Payload string
HeaderFunc func(*http.Header)
URLs []string
client *http.Client
expectedStatus int
Name string
Payload string
HeaderFunc func(*http.Header)
URLs []string
}{
{
Name: "generic",
Expand All @@ -73,8 +65,6 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
URLs: []string{
"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret200/generic",
},
client: adminHTTPClient,
expectedStatus: http.StatusOK,
},
{
Name: "github",
Expand All @@ -83,8 +73,6 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
URLs: []string{
"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret100/github",
},
client: adminHTTPClient,
expectedStatus: http.StatusOK,
},
{
Name: "gitlab",
Expand All @@ -93,8 +81,6 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
URLs: []string{
"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret300/gitlab",
},
client: adminHTTPClient,
expectedStatus: http.StatusOK,
},
{
Name: "bitbucket",
Expand All @@ -103,29 +89,6 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
URLs: []string{
"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret400/bitbucket",
},
client: adminHTTPClient,
expectedStatus: http.StatusOK,
},
{
// AUTH-509: Webhooks do not allow unauthenticated requests by default.
// Test will verify that an unauthenticated request fails with 403 Forbidden.
Name: "unauthenticated forbidden",
Payload: "generic/testdata/push-generic.json",
HeaderFunc: genericHeaderFunc,
URLs: []string{
"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret200/generic",
},
// Need client to skip TLS verification - CI clusters have self-signed certificates.
// Transport also needs to accept proxy information from *_PROXY environment variables.
client: &http.Client{
Transport: &http.Transport{
Proxy: http.ProxyFromEnvironment,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
},
},
expectedStatus: http.StatusForbidden,
},
}

Expand All @@ -136,12 +99,8 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
clusterAdminClientConfig := oc.AdminConfig()

g.By("executing the webhook to get the build object")
body := postFile(test.client, test.HeaderFunc, test.Payload, clusterAdminClientConfig.Host+s, test.expectedStatus, t, oc)
body := postFile(clusterAdminBuildClient.RESTClient(), test.HeaderFunc, test.Payload, clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
o.Expect(body).NotTo(o.BeEmpty())
// If expected HTTP status is not 200 OK, continue as we will not receive a Build object in the response body.
if test.expectedStatus != http.StatusOK {
continue
}

g.By("Unmarshalling the build object")
returnedBuild := &buildv1.Build{}
Expand All @@ -165,7 +124,6 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
clusterAdminClientConfig := oc.AdminConfig()
clusterAdminImageClient := oc.AdminImageClient().ImageV1()
clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client

// create imagerepo
imageStream := &imagev1.ImageStream{
Expand Down Expand Up @@ -215,7 +173,7 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
} {

// trigger build event sending push notification
body := postFile(adminHTTPClient, githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
body := postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
if len(body) == 0 {
t.Errorf("Webhook did not return Build in body")
}
Expand Down Expand Up @@ -243,6 +201,7 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
if actual.Spec.Strategy.DockerStrategy.From.Name != "originalimage" {
t.Errorf("Expected %s, got %s", "originalimage", actual.Spec.Strategy.DockerStrategy.From.Name)
}

if actual.Name != returnedBuild.Name {
t.Errorf("Build returned in response body does not match created Build. Expected %s, got %s", actual.Name, returnedBuild.Name)
}
Expand All @@ -255,7 +214,6 @@ func TestWebhookGitHubPushWithImageStream(t g.GinkgoTInterface, oc *exutil.CLI)
clusterAdminClientConfig := oc.AdminConfig()
clusterAdminImageClient := oc.AdminImageClient().ImageV1()
clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client

// create imagerepo
imageStream := &imagev1.ImageStream{
Expand Down Expand Up @@ -307,7 +265,7 @@ func TestWebhookGitHubPushWithImageStream(t g.GinkgoTInterface, oc *exutil.CLI)
s := "/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret101/github"

// trigger build event sending push notification
postFile(adminHTTPClient, githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)

var build *buildv1.Build

Expand All @@ -333,7 +291,6 @@ Loop:

func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client

// create buildconfig
buildConfig := mockBuildConfigImageParms("originalimage", "imagestream", "validtag")
Expand All @@ -355,7 +312,7 @@ func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
// trigger build event sending push notification
clusterAdminClientConfig := oc.AdminConfig()

postFile(adminHTTPClient, githubHeaderFuncPing, "github/testdata/pingevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFuncPing, "github/testdata/pingevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)

// TODO: improve negative testing
timer := time.NewTimer(time.Second * 5)
Expand All @@ -369,9 +326,9 @@ func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
}
}

func postFile(client *http.Client, headerFunc func(*http.Header), filename, url string, expStatusCode int, t g.GinkgoTInterface, oc *exutil.CLI) []byte {
func postFile(client rest.Interface, headerFunc func(*http.Header), filename, url string, expStatusCode int, t g.GinkgoTInterface, oc *exutil.CLI) []byte {
path := exutil.FixturePath("testdata", "builds", "webhook", filename)
data, err := os.ReadFile(path)
data, err := ioutil.ReadFile(path)
if err != nil {
t.Fatalf("Failed to open %s: %v", filename, err)
}
Expand All @@ -380,11 +337,11 @@ func postFile(client *http.Client, headerFunc func(*http.Header), filename, url
t.Fatalf("Error creating POST request: %v", err)
}
headerFunc(&req.Header)
resp, err := client.Do(req)
resp, err := client.(*rest.RESTClient).Client.Do(req)
if err != nil {
t.Fatalf("Failed posting webhook: %v", err)
}
body, _ := io.ReadAll(resp.Body)
body, _ := ioutil.ReadAll(resp.Body)
if resp.StatusCode != expStatusCode {
t.Errorf("Wrong response code, expecting %d, got %d: %s!", expStatusCode, resp.StatusCode, string(body))
}
Expand Down

0 comments on commit e3c844d

Please sign in to comment.