NE-1140, NE-1145: Set/delete HTTP request/response headers via IngressController API #872

miheer · 2022-12-25T07:49:53Z

Set/Replace/Append HTTP Request/Response Headers via Ingress Controller
API
pkg/operator/controller/ingress/deployment.go Fetches the headers from
the ingress controller CR and then sets the router deployment with
environment vars with the header value and action.
pkg/operator/controller/ingress/deployment_test.go Units tests for
setting enviroment var in thr router deployment.

miheer · 2022-12-26T05:44:57Z

/test verify

miheer · 2022-12-26T06:04:29Z

/test verify

candita · 2023-08-10T18:04:14Z

test/e2e/set_delete_test.go

+			t.Errorf("failed to get logs from pod %s: %v", clientPod.Name, err)
+		}
+
+		t.Fatalf("failed to observe the expected output: %v\nclient pod spec: %#v\nclient pod logs:\n%s", err, pod, logs)


err has been redefined, no longer relevant and could be nil. Just print out the logs.

Suggested change

t.Fatalf("failed to observe the expected output: %v\nclient pod spec: %#v\nclient pod logs:\n%s", err, pod, logs)

t.Errorf("client pod spec: %#v\nclient pod logs:\n%s", pod, logs)

Better yet, rename the inner err to err2 so that it doesn't clobber err.

Actually, I see that Miheer added a t.Errorf statement earlier on that prints err before it gets clobbered, so no need to print it here anyway.

candita · 2023-08-10T22:51:59Z

/retest-required

Miciah · 2023-08-11T00:34:19Z

test/e2e/set_delete_test.go

+	echoRoute := buildRoute(echoPod.Name, echoPod.Namespace, echoService.Name)
+	if err := kclient.Create(context.TODO(), echoRoute); err != nil {
+		t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)
+	}
+	defer func() {
+		if err := kclient.Delete(context.TODO(), echoRoute); err != nil {
+			t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)
+		}
+	}()


This should make the test a little more reliable:

Suggested change

echoRoute := buildRoute(echoPod.Name, echoPod.Namespace, echoService.Name)

if err := kclient.Create(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

defer func() {

if err := kclient.Delete(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

}()

echoRoute := buildRoute(echoPod.Name, echoPod.Namespace, echoService.Name)

if err := kclient.Create(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

defer func() {

if err := kclient.Delete(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

}()

cond := routev1.RouteIngressCondition{

Type: routev1.RouteAdmitted,

Status: corev1.ConditionTrue,

}

echoRouteName := types.NamespacedName{

Namespace: echoRoute.Namespace,

Name: echoRoute.Name,

}

waitForRouteIngressConditions(t, kclient, echoRouteName, ic.Name, cond)

Miciah · 2023-08-11T00:34:41Z

test/e2e/set_delete_test.go

+	echoRoute := buildRouteWithHTTPResponseHeaders(echoPod.Name, echoPod.Namespace, echoService.Name, headerNameXFrame, "DENY")
+	if err := kclient.Create(context.TODO(), echoRoute); err != nil {
+		t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)
+	}
+	defer func() {
+		if err := kclient.Delete(context.TODO(), echoRoute); err != nil {
+			t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)
+		}
+	}()


Suggested change

echoRoute := buildRouteWithHTTPResponseHeaders(echoPod.Name, echoPod.Namespace, echoService.Name, headerNameXFrame, "DENY")

if err := kclient.Create(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

defer func() {

if err := kclient.Delete(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

}()

echoRoute := buildRouteWithHTTPResponseHeaders(echoPod.Name, echoPod.Namespace, echoService.Name, headerNameXFrame, "DENY")

if err := kclient.Create(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to create route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

defer func() {

if err := kclient.Delete(context.TODO(), echoRoute); err != nil {

t.Fatalf("failed to delete route %s/%s: %v", echoRoute.Namespace, echoRoute.Name, err)

}

}()

cond := routev1.RouteIngressCondition{

Type: routev1.RouteAdmitted,

Status: corev1.ConditionTrue,

}

echoRouteName := types.NamespacedName{

Namespace: echoRoute.Namespace,

Name: echoRoute.Name,

}

waitForRouteIngressConditions(t, kclient, echoRouteName, ic.Name, cond)

Miciah · 2023-08-11T00:37:37Z

test/e2e/set_delete_test.go

+		for scanner.Scan() {
+			line := scanner.Text()
+			if strings.Contains(strings.ToLower(line), expectedResponse) {
+				t.Logf("found %d expected: %s", expectedMatches, line)


You aren't checking the number of matches anymore, so why do you need expectedMatches?

Miciah · 2023-08-11T00:38:58Z

test/e2e/set_delete_test.go

+			}
+		}
+	}()
+	expectedResponse = strings.ToLower(expectedResponse)


Why do you use strings.ToLower here and below? Just delcare headerNameXFrame to be lower-case x-frame-options below, and then you don't need any strings.ToLower.

Miciah · 2023-08-11T00:40:57Z

test/e2e/set_delete_test.go

+
+func TestSetIngressControllerResponseHeaders(t *testing.T) {
+	t.Parallel()
+	var headerNameXFrame string = "X-Frame-Options"


Better yet:

Suggested change

var headerNameXFrame string = "X-Frame-Options"

const (

headerName = "x-frame-options"

headerValue = "DENY"

)

And replace "DENY" with headerValue below.

Miciah · 2023-08-11T00:42:09Z

test/e2e/set_delete_test.go

+
+var podCount int
+
+func testHeaders(t *testing.T, image string, route *routev1.Route, address string, headers []string, expectedResponse string, expectedMatches int) {


You can delete the headers parameter.

Miciah · 2023-08-11T00:54:30Z

@miheer, the commits are backwards. Do the vendor bump first, then the implementation.

… used to set the HTTP header request/reponse in Ingress Controller CR's spec. The other files are generated files after running `go mod tidy` followed by `go mod vendor`.

miheer · 2023-08-11T05:12:57Z

/retest

miheer · 2023-08-11T09:21:40Z

/retest

miheer · 2023-08-11T09:25:55Z

https://github.com/openshift/cluster-ingress-operator/compare/5ceabd73033d098a2b3006370c1309b346ee1deb..29c86b4ca01dc3738c87018f6cfef2bfa6565bbc -first iteration RAW push had comments just had pushed to test. This had @candita 's suggestion for changing the block with return true and false and not using number of matches logic.

https://github.com/openshift/cluster-ingress-operator/compare/29c86b4ca01dc3738c87018f6cfef2bfa6565bbc..4dc8310c9d3698bf41ffd7b391a6ffa38951b60b --- comments removed and code was cleaned up.

https://github.com/openshift/cluster-ingress-operator/compare/4dc8310c9d3698bf41ffd7b391a6ffa38951b60b..32a6380e633d74a67db6f3007eccee5830e2d5a6 ---- consists of @Miciah suggestions and also some @candita suggestions to fix log statements.

The last push was just changing the sequence of the commits as
@Miciah proposed.

miheer · 2023-08-11T09:31:17Z

@miheer, the commits are backwards. Do the vendor bump first, then the implementation.

done

API pkg/operator/controller/ingress/deployment.go Fetches the headers from the ingress controller CR and then sets the router deployment with environment vars with the header value and action. pkg/operator/controller/ingress/deployment_test.go Units tests for setting enviroment var in thr router deployment.

miheer · 2023-08-11T09:55:39Z

some more nits: @Miciah and @candita
https://github.com/openshift/cluster-ingress-operator/compare/6d7f0639dac085f71fa158e3ad996a6f07e264cb..7230a6c2e5fc9bceca82ca41fbeeb3491dabff8c

openshift-ci · 2023-08-11T12:44:54Z

@miheer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn-serial	`a3a6318`	link	true	`/test e2e-gcp-ovn-serial`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

candita · 2023-08-11T13:26:10Z

Two hypershift unit tests failed:
=== FAIL: TestNodePool/NodePool_Tests_Group/TestNTOMachineConfigGetsRolledOut/EnsureNoCrashingPods (0.01s)
util.go:464: Container controller in pod cloud-network-config-controller-5d5b8949b-kgtk5 has a restartCount > 0 (1)
--- FAIL: TestNodePool/NodePool_Tests_Group/TestNTOMachineConfigGetsRolledOut/EnsureNoCrashingPods (0.01s)
=== FAIL: TestNodePool/NodePool_Tests_Group (0.00s)
--- FAIL: TestNodePool/NodePool_Tests_Group (0.00s)

/test e2e-hypershift

candita · 2023-08-11T14:12:51Z

/approve

openshift-ci · 2023-08-11T14:15:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [candita]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-08-11T15:54:33Z

/test remaining-required

candita · 2023-08-11T18:48:15Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 25, 2022

openshift-ci bot requested review from alebedev87 and gcs278 December 25, 2022 07:50

miheer force-pushed the headers branch 2 times, most recently from bbffb85 to 40e1904 Compare December 26, 2022 05:35

miheer force-pushed the headers branch 3 times, most recently from 928f19a to 704ce4e Compare December 27, 2022 13:13

miheer force-pushed the headers branch 14 times, most recently from e90540a to a3a6318 Compare January 13, 2023 03:42

miheer force-pushed the headers branch 2 times, most recently from f4cb835 to 477e41d Compare January 13, 2023 09:18

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2023

miheer force-pushed the headers branch from 477e41d to f55c835 Compare February 14, 2023 03:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023

miheer force-pushed the headers branch from f55c835 to 42360c7 Compare February 14, 2023 03:45

candita reviewed Aug 10, 2023

View reviewed changes

miheer force-pushed the headers branch 2 times, most recently from 29c86b4 to 4dc8310 Compare August 11, 2023 00:03

Miciah reviewed Aug 11, 2023

View reviewed changes

miheer force-pushed the headers branch from 4dc8310 to 32a6380 Compare August 11, 2023 03:04

Miciah mentioned this pull request Aug 11, 2023

OCPBUGS-17359: test/e2e: Don't use openshift/origin-node #970

Merged

go.mod Adds openshift/api containing IngressController API which is…

481f811

… used to set the HTTP header request/reponse in Ingress Controller CR's spec. The other files are generated files after running `go mod tidy` followed by `go mod vendor`.

miheer force-pushed the headers branch from 32a6380 to 6d7f063 Compare August 11, 2023 03:10

miheer force-pushed the headers branch from 6d7f063 to 7230a6c Compare August 11, 2023 09:54

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2023

openshift-merge-robot merged commit 56a00a7 into openshift:master Aug 11, 2023

	t.Fatalf("failed to observe the expected output: %v\nclient pod spec: %#v\nclient pod logs:\n%s", err, pod, logs)
	t.Errorf("client pod spec: %#v\nclient pod logs:\n%s", pod, logs)

-	var headerNameXFrame string = "X-Frame-Options"
+	const (
+		headerName  = "x-frame-options"
+		headerValue = "DENY"
+	)


		var podCount int

		func testHeaders(t testing.T, image string, route routev1.Route, address string, headers []string, expectedResponse string, expectedMatches int) {

NE-1140, NE-1145: Set/delete HTTP request/response headers via IngressController API #872

NE-1140, NE-1145: Set/delete HTTP request/response headers via IngressController API #872

Uh oh!

Conversation

miheer commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miheer commented Dec 26, 2022

Uh oh!

miheer commented Dec 26, 2022

Uh oh!

candita Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

candita commented Aug 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah commented Aug 11, 2023

Uh oh!

miheer commented Aug 11, 2023

Uh oh!

miheer commented Aug 11, 2023

Uh oh!

miheer commented Aug 11, 2023

Uh oh!

miheer commented Aug 11, 2023

Uh oh!

miheer commented Aug 11, 2023

Uh oh!

openshift-ci bot commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

candita commented Aug 11, 2023

Uh oh!

candita commented Aug 11, 2023

Uh oh!

openshift-ci bot commented Aug 11, 2023

Uh oh!

openshift-ci bot commented Aug 11, 2023

Uh oh!

candita commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

miheer commented Dec 25, 2022 •

edited

Loading

candita Aug 10, 2023 •

edited

Loading

openshift-ci bot commented Aug 11, 2023 •

edited

Loading