nghttp2: Update to 1.47

[alessandrogario]

Commit Message: nghttp2: Update to 1.47
Fixes #20101

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20381 was opened by alessandrogario.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #20381 was opened by alessandrogario.

see: more, trace.

[phlax]

@alessandrogario this is failing general linting on the patch files (see https://dev.azure.com/cncf/envoy/_build/results?buildId=103969&view=logs&j=c5dd2866-6ab3-5f3c-3a44-4cef0ec909b5&t=a9eb66d6-8944-5769-b3f7-476949dadcb8&l=1223)

im thinking in this case we more want to exclude the patch files as they can correctly have the formatting that this checks for (trailing whitespace, mixed tabs/spaces, no newline)

ill fix the issue upstream to address

[phlax]

the other thing im wondering about is whether we could/should update nghttp2 at the same time (#20101)

[alessandrogario]

the other thing im wondering about is whether we could/should update nghttp2 at the same time (#20101)

Hello @phlax! Would you like me to update the library too?

[phlax]

Would you like me to update the library too?

i reckon - it just means we wont have to redo the patches as quickly i think

[phlax]

seems like you fixed the patch matching anyhow - i raised a PR to improve the matcher here envoyproxy/toolshed#450 - but i wont land it if its not necessary i think

[alessandrogario]

seems like you fixed the patch matching anyhow - i raised a PR to improve the matcher here envoyproxy/pytooling#450 - but i wont land it if its not necessary i think

It seems like a good idea to fix it, so that we can use patch files with the correct file names (as generated by git format-patch). Thanks for looking into it!

[alessandrogario]

Would you like me to update the library too?

i reckon - it just means we wont have to redo the patches as quickly i think

Hello @phlax!

I have updated the nghttp2 library and its patches, but I am now encountering an error with the //test/integration:h1_capture_persistent_fuzz_test test.

I have tried to reproduce it but tests are all failing when I try to run them locally through the CI scripts; what is the preferred way to run them on the development machine?

EDIT: I was able to run the test locally, and it seems to be passing. Is this caused by some kind of timeout on the CI?

$ bazel build //test/integration:h1_capture_persistent_fuzz_test
Starting local Bazel server and connecting to it...
INFO: Analyzed target //test/integration:h1_capture_persistent_fuzz_test (419 packages loaded, 19754 targets configured).
INFO: Found 1 target...
Target //test/integration:h1_capture_persistent_fuzz_test up-to-date:
  bazel-bin/test/integration/h1_capture_persistent_fuzz_test
INFO: Elapsed time: 763.736s, Critical Path: 72.86s
INFO: 2626 processes: 31 internal, 2595 linux-sandbox.
INFO: Build completed successfully, 2626 total actions

[yanavlasov]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20381 (comment) was created by @yanavlasov.

see: more, trace.

[yanavlasov]

/wait

[alyssawilk]

Yeah, AFIK we're doing this for testing of UVH, but don't plan on using UHV in production until we switch to oghttp2 which will allow runtime disable of only header validation (not frame validation). So I think this is fine for an interim test-only solution especially if we comment what the long term plan is.
cc @yanavlasov for a second check

[alessandrogario]

Yeah, AFIK we're doing this for testing of UVH, but don't plan on using UHV in production until we switch to oghttp2 which will allow runtime disable of only header validation (not frame validation). So I think this is fine for an interim test-only solution especially if we comment what the long term plan is. cc @yanavlasov for a second check

Thanks for the explanation! I'll keep removing all the failing/crashing tests with an #if !defined(ENVOY_ENABLE_UHV). Where possible, I'll try to just add a return statement to skip it

[alessandrogario]

Yeah, AFIK we're doing this for testing of UVH, but don't plan on using UHV in production until we switch to oghttp2 which will allow runtime disable of only header validation (not frame validation). So I think this is fine for an interim test-only solution especially if we comment what the long term plan is. cc @yanavlasov for a second check

Thanks for the explanation! I'll keep removing all the failing/crashing tests with an #if !defined(ENVOY_ENABLE_UHV). Where possible, I'll try to just add a return statement to skip it

I am starting to hit additional problems outside the tests due to the additional things that NGHTTP2_OPTMASK_NO_HTTP_MESSAGING disables. The ConnectionManagerImpl class for example is now failing many asserts such as:

  // Verify header sanity checks which should have been performed by the codec.
  ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false);

[alyssawilk]

well if we're doing validation in Envoy we're going to want to not validate the codec is doing said validation. I think these are things that will just have to change as part of UHV :-)

[yanavlasov]

Oh sorry, just realized compile_time_options build failed a bunch of tests. I think for now we can just put in the #ifdef to skip them. You can do in the SetUp() method for the test fixture class and add GTEST_SKIP(); if it fails all (or most) of the tests in the suite. The uring tests use similar method. As we add UHV we can re-enable these tests. For tests suites that fail just a few tests we can use the method suggested below with a function that causes tests to be skipped, so we do not loose too much coverage in the compilet_time_options build

[yanavlasov]

/wait

[yanavlasov]

/wait

[phlax]

@alessandrogario looks like this needs a main merge - and a minor (trailing whitespace) formatting nit

copying out the ci advice

  Please fix your editor to ensure:

      - no trailing whitespace
      - no preceding mixed tabs/spaces
      - all files end with a newline

[alessandrogario]

@alessandrogario looks like this needs a main merge - and a minor (trailing whitespace) formatting nit

copying out the ci advice

  Please fix your editor to ensure:

      - no trailing whitespace
      - no preceding mixed tabs/spaces
      - all files end with a newline

Hello @phlax! I think this PR can be closed, but waiting on @yanavlasov to make sure.

[alessandrogario]

@alessandrogario looks like this needs a main merge - and a minor (trailing whitespace) formatting nit
copying out the ci advice

  Please fix your editor to ensure:

      - no trailing whitespace
      - no preceding mixed tabs/spaces
      - all files end with a newline

Hello @phlax! I think this PR can be closed, but waiting on @yanavlasov to make sure.

I think adding UHV here is no longer required (cc @yanavlasov), so the only commit left in the branch just updates the nghttp2 library to the latest version. Feel free to merge it if it is useful or close it otherwise

[phlax]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20381 (comment) was created by @phlax.

see: more, trace.

[phlax]

cc @moderation i think we can land this as it now only updates the nghttp2 lib and CI is green

[moderation]

/lgtm deps

[phlax]

lgtm, thanks @alessandrogario

Only nghttp2 update remains

[phlax]

/retest