Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: proxying 104s Upload Resumption Supported #34738

Merged

Conversation

Nealsoni00
Copy link
Contributor

@Nealsoni00 Nealsoni00 commented Jun 13, 2024

Proxying 104s from upstream.

104 Upload Resumption Supported is introduced in draft-ietf-httpbis-resumable-upload spec.

Continuation of the discussion in #19044
Modeled after: #19023

Risk Level: Low (minor refactor)
Testing: new integration test
Docs Changes: N/A

Release Notes:

http: envoy will now proxy 104 headers from upstream, though as with 100s only the first 1xx response headers will be sent. This behavior can be temporarily reverted by setting runtime guard envoy.reloadable_features.proxy_104 to false. 104 headers are designated by ietf's draft-ietf-httpbis-resumable-upload rfc.

Runtime guard:

envoy.reloadable_features.proxy_104

Copy link

Hi @Nealsoni00, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #34738 was opened by Nealsoni00.

see: more, trace.

@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch 4 times, most recently from 422ba43 to 27935a4 Compare June 16, 2024 22:42
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
…ponses

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from 5a706e9 to 9471370 Compare June 17, 2024 13:29
@RyanTheOptimist
Copy link
Contributor

Looks like this fails to build

# Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
source/common/http/header_utility.cc:239:57: error: expected ')'
          (response_headers.Status()->value() == "104") {
                                                        ^
source/common/http/header_utility.cc:238:6: note: to match this '('
  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.proxy_104") &&
     ^
1 error generated.
INFO: Elapsed time: 51.685s, Critical Path: 17.41s
INFO: 34267 processes: 20413 remote cache hit, 13851 internal, 1 local, 2 processwrapper-sandbox.
//test/common/common:base64_fuzz_test                           FAILED TO BUILD

Please update the release notes.

/wait

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from 303783b to c9bd729 Compare June 18, 2024 17:33
@Nealsoni00
Copy link
Contributor Author

Nealsoni00 commented Jun 18, 2024

@RyanTheOptimist Build fix incoming.

Where should I add release notes (other than in the PR description)?

http: envoy will now proxy 104 headers from upstream, though as with 100s only the first 1xx response headers will be sent. his behavioral can be temporarily reverted by setting runtime guard envoy.reloadable_features.proxy_104 to false. 104 headers are designated by ietf's draft-ietf-httpbis-resumable-upload rfc.

@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from a8e0ed2 to d4afe75 Compare June 18, 2024 17:51
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch 3 times, most recently from ef13198 to ad1eddd Compare June 18, 2024 22:18
@RyanTheOptimist
Copy link
Contributor

/wait

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from 2d100da to b86a01a Compare June 28, 2024 18:49
@Nealsoni00
Copy link
Contributor Author

@RyanTheOptimist Is the change I made sufficient to test the propagation of the Upload-Draft-Interop-Version header?

RyanTheOptimist
RyanTheOptimist previously approved these changes Jul 2, 2024
@RyanTheOptimist
Copy link
Contributor

/wait
Looks like it needs a main merge?

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00
Copy link
Contributor Author

@RyanTheOptimist not sure why tests are failing — verified those same tests are failing on master.

Screenshot 2024-07-02 at 3 46 58 PM

@RyanTheOptimist
Copy link
Contributor

/wait
Try doing a main merge and see if that resolves the issues?

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from 7733ce7 to ada8151 Compare July 3, 2024 18:19
Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00 Nealsoni00 force-pushed the 104-upload-resumption-supported branch from 88c7fc9 to 12344f2 Compare July 3, 2024 23:13
@Nealsoni00
Copy link
Contributor Author

@RyanTheOptimist It should be good now!

Signed-off-by: Neal Soni <Nealsoni00@gmail.com>
@Nealsoni00
Copy link
Contributor Author

Nealsoni00 commented Jul 8, 2024

@RyanTheOptimist sorry to re-ping. Would love to get this into master before it becomes out of date!

Once in, what's the process to deprecate the runtime flag? Should I create a PR to remove it and leave it to y'alls discretion to merge when appropriate?

@alyssawilk
Copy link
Contributor

I've sent you an invite to the "assignable" group - in ~6m when we cut the release where this is no longer needed the tool will file an issue your way that removal is appropriate.

@RyanTheOptimist RyanTheOptimist merged commit a1214c8 into envoyproxy:main Jul 8, 2024
51 checks passed
@Nealsoni00
Copy link
Contributor Author

@alyssawilk happy to help clean up the runtime flag here — is there a ticket i should associate it to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants