OCPBUGS-3356: E2E test for cookie length truncation #871

rfredette · 2022-12-21T23:43:05Z

This PR adds a new test, TestCookieLen, which verifies that cookies in spec.logging.access.httpCaptureCookies are truncated to the specified max length.

This tests the fix in openshift/router#436

openshift-ci-robot · 2022-12-21T23:43:08Z

@rfredette: This pull request references Jira Issue OCPBUGS-3356, which is invalid:

expected the bug to target the "4.13.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

When access logging is enabled and a cookie is specified to be captured (with spec.logging.access.httpCaptureCookies), if max length is greater than 63 bytes, the haproxy global option tune.http.cookielen needs to be updated to be one byte larger than the specified max length, or the cookie will be truncated in the logs, and a warning will be logged on each router restart.

This PR, along with openshift/router#436, automatically sets tune.http.cookielen to the required value when max length exceeds 63 bytes.

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.

rfredette · 2022-12-21T23:46:59Z

With this and the related router PR loaded on a cluster, I don't see the truncation warning, but I am seeing this message popping up every few seconds in the router logs:

E1221 23:24:41.426700       1 webhook.go:90] Failed to make webhook authenticator request: the server could not find the requested resource

If I revert the ingress operator to the version without this change, I see the truncation warning but not this webhook message, so I don't think it's a configuration issue, but I need to dig some more to figure out what this error message even really means...

rfredette · 2022-12-21T23:56:36Z

/jira refresh

openshift-ci-robot · 2022-12-21T23:56:44Z

@rfredette: This pull request references Jira Issue OCPBUGS-3356, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

In response to this:

/jira refresh

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.

pkg/operator/controller/ingress/deployment.go

openshift-bot · 2023-04-18T09:00:52Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2023-05-19T00:30:59Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

rfredette · 2023-06-05T18:30:34Z

/remove-lifecycle rotten

rfredette · 2023-06-09T21:14:23Z

some tests failed due to an issue building the image, but since other tests didn't hit that issue, I'm assuming it was a timing thing.
/retest

rfredette · 2023-06-13T22:36:33Z

The new test, TestCookieLen, won't pass in CI until openshift/router#436 merges. I ran the test on a cluster with both this PR and openshift/router#436, and it passes:

$ TEST="TestCookieLen" make test-e2e
CGO_ENABLED=1 GO111MODULE=on GOFLAGS=-mod=vendor go test -timeout 1h -count 1 -v -tags e2e -run "TestCookieLen" ./test/e2e
=== RUN   TestCookieLen                                                                                                                                                       
=== PAUSE TestCookieLen                                                                
=== CONT  TestCookieLen                                                                                                                                                       
    tuning_options_test.go:516: Deleted cookielen-curl-6qf4c                   
    tuning_options_test.go:516: deleted ingresscontroller test-cookielen-2kt9d       
--- PASS: TestCookieLen (85.55s)                                                                                                                                              
PASS                                                                                   
ok      github.com/openshift/cluster-ingress-operator/test/e2e  86.000s

rfredette · 2023-06-14T18:04:53Z

After looking at this again, it's simpler to have the router figure out when to set tune.http.cookielen from the existing environment variables, rather than having the operator add a new environment variable. I've updated openshift/router#436 to do that, and now this PR only includes a new e2e test. Like before, the e2e test won't pass in CI until openshift/router#436 merges, but I've run it against a cluster with openshift/router#436 deployed, and it passes:

$ TEST="TestCookieLen" make test-e2e
CGO_ENABLED=1 GO111MODULE=on GOFLAGS=-mod=vendor go test -timeout 1h -count 1 -v -tags e2e -run "TestCookieLen" ./test/e2e
=== RUN   TestCookieLen
=== PAUSE TestCookieLen
=== CONT  TestCookieLen
    tuning_options_test.go:523: Deleted cookielen-curl-vcmdx
    tuning_options_test.go:523: deleted ingresscontroller test-cookielen-2llcg
--- PASS: TestCookieLen (84.51s)
PASS
ok      github.com/openshift/cluster-ingress-operator/test/e2e  84.808s

candita · 2023-06-28T15:40:05Z

/assign @frobware

frobware

I'd like to see tests for where the maxLength for the cookie capture is 63 and 1024. The API forces a value so the first would exercise the default, and the latter tests for the API's maximum. In both cases we should assert that cookie values that are 1 more than the specified max get truncated.

test/e2e/tuning_options_test.go

test/e2e/client_tls_test.go

test/e2e/tuning_options_test.go

test/e2e/util_test.go

test/e2e/tuning_options_test.go

candita · 2023-08-17T20:16:47Z

test/e2e/tuning_options_test.go

+			}
+			if !foundExpectedCookie {
+				t.Fatalf("did not find expected truncated cookie string: %v", expectedTruncatedCookieString)
+			}


Add a message here so that if there are some failures and some successes, you will know which succeeded.

Suggested change

}

}

t.Logf("found expected cookie truncated to %d", maxLength)

Each test case is run with t.Run(), so they show up separately in the test output. To induce a failure and to test out a question you asked in another comment, I changed the 1024 byte case to 1026 bytes and ran the test. Here's a snippet of the output:

=== NAME TestCookieLen/1026_bytes tuning_options_test.go:379: Failed to create ingresscontroller openshift-ingress-operator/test-cookielen-hv8vz: IngressController.operator.openshift.io "test-cookielen-hv8vz" is invalid: spec.logging.access.httpCaptureCookies[0].maxLength: Invalid value: 1026: spec.logging.access.httpCaptureCookies[0].maxLength in body should be less than or equal to 1024 ... cut for brevity ... === NAME TestCookieLen/128_bytes tuning_options_test.go:541: deleted ingresscontroller test-cookielen-j5lxj === NAME TestCookieLen/63_bytes tuning_options_test.go:541: deleted ingresscontroller test-cookielen-kh5rl --- FAIL: TestCookieLen (0.00s) --- FAIL: TestCookieLen/1026_bytes (0.07s) --- PASS: TestCookieLen/64_bytes (90.57s) --- PASS: TestCookieLen/128_bytes (91.78s) --- PASS: TestCookieLen/63_bytes (93.00s) FAIL FAIL github.com/openshift/cluster-ingress-operator/test/e2e 93.475s FAIL

The output does get interleaved due to the parallel execution, but I think it's clear enough which test case is which.

I was asking about adding a positive message to match the negative message: "did not find expected truncated cookie string". It's a nit.

But the error message I see above for 1026 bytes, that seems weird. It starts with "Failed to create ingresscontroller". Can you add a test that you expect to fail and print out the reason why it fails rather than the reason why the ingress controller doesn't get created?

The error message in the 1026 case was the expected one. httpCaptureCookies.maxLength is a field of the ingresscontroller object, and the openshift API limits it to a number between 1 and 1024, so trying to set it to a number outside that range will cause the ingresscontroller to be rejected

test/e2e/tuning_options_test.go

rfredette · 2023-08-28T15:36:07Z

Test failures are unrelated.
/retest

test/e2e/tuning_options_test.go

candita · 2023-08-28T20:35:16Z

test/e2e/tuning_options_test.go

+			}
+
+			logsContainerName := "logs"
+			readCloser, err := client.CoreV1().Pods(routerPod.Namespace).GetLogs(routerPod.Name, &corev1.PodLogOptions{


The pattern that is used for scanning logs usually has a poll loop, like https://github.com/openshift/cluster-ingress-operator/blame/master/test/e2e/http_header_buffer_test.go#L134-L163. If it isn't there the first time you check, it may not have arrived yet.

openshift-ci · 2023-09-05T21:09:30Z

@rfredette: 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	`2a4ff9d`	link	true	`/test e2e-gcp-ovn-serial`

Full PR test history. Your PR dashboard.

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.

rfredette · 2023-09-13T17:18:24Z

/retest

frobware · 2023-09-14T14:20:26Z

/lgtm

candita · 2023-09-14T15:21:00Z

/approve

openshift-ci · 2023-09-14T15:21:21Z

[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

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

candita · 2023-09-14T18:57:02Z

/acknowledge-critical-fixes-only

candita · 2023-09-14T19:34:53Z

/label acknowledge-critical-fixes-only

openshift-ci-robot · 2023-09-14T19:39:14Z

@rfredette: Jira Issue OCPBUGS-3356: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-3356 has been moved to the MODIFIED state.

In response to this:

This PR adds a new test, TestCookieLen, which verifies that cookies in spec.logging.access.httpCaptureCookies are truncated to the specified max length.

This tests the fix in openshift/router#436

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.

openshift-merge-robot · 2023-09-16T01:32:58Z

Fix included in accepted release 4.14.0-0.nightly-2023-09-11-201102

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Dec 21, 2022

openshift-ci bot requested review from Miciah and miheer December 21, 2022 23:43

candita reviewed Jan 17, 2023

View reviewed changes

pkg/operator/controller/ingress/deployment.go Outdated Show resolved Hide resolved

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2023

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 19, 2023

openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jun 5, 2023

rfredette force-pushed the ocpbugs-3356-tune-cookielen branch from 2a4ff9d to 961a614 Compare June 9, 2023 16:46

rfredette force-pushed the ocpbugs-3356-tune-cookielen branch 2 times, most recently from 642b3e1 to c11ca02 Compare June 14, 2023 18:01

rfredette force-pushed the ocpbugs-3356-tune-cookielen branch from c11ca02 to 29b1632 Compare June 22, 2023 21:27

rfredette changed the title ~~OCPBUGS-3356: Automatically adjust tune.http.cookielen when necessary~~ OCPBUGS-3356: E2E test for cookie length truncation Jun 28, 2023

openshift-ci bot assigned frobware Jun 28, 2023

frobware requested changes Jul 10, 2023

View reviewed changes

openshift-ci-robot mentioned this pull request Jul 10, 2023

OCPBUGS-3356: Add support for tuning tune.http.cookielen openshift/router#436

Merged

candita reviewed Aug 17, 2023

View reviewed changes