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: remove deprecated http_parser http1 codec #38950

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ggreenway
Copy link
Member

Fixes #33622

Risk Level: Low

Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38950 was opened by ggreenway.

see: more, trace.

@jmarantz
Copy link
Contributor

I think we still have some uses of it internally that we are trying to track down.

@paul-r-gall
Copy link
Contributor

Actively working on removing internal reliances. @ggreenway I've been on pat leave for several months which is why this is behind schedule.

@abeyad
Copy link
Contributor

abeyad commented Mar 28, 2025

/wait

Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

but waiting on removing uses of this APi

@abeyad
Copy link
Contributor

abeyad commented Mar 28, 2025

/assign @jmarantz

@jmarantz
Copy link
Contributor

We'd like to wait before merging till Paul sorts out our internal dependencies. Can we flip this into WiP mode?

ravenblackx
ravenblackx previously approved these changes Mar 28, 2025
@ravenblackx
Copy link
Contributor

We'd like to wait before merging till Paul sorts out our internal dependencies. Can we flip this into WiP mode?

It's already been more than a year - surely Google can manage to do what anyone else would have to do here, just not update to a newer version until they've fixed their internal dependencies to be compatible with that newer version?

@yanavlasov
Copy link
Contributor

We'd like to wait before merging till Paul sorts out our internal dependencies. Can we flip this into WiP mode?

It's already been more than a year - surely Google can manage to do what anyone else would have to do here, just not update to a newer version until they've fixed their internal dependencies to be compatible with that newer version?

@ravenblackx we had a "code yellow" for one of the products that prevented us from replacing the codec due to its risk. We are not blocked any longer and actively working internally on completing the codec migration. We would appreciate understanding on this issue and give us more time to avoid carrying a large patch internally.

@yanavlasov yanavlasov self-assigned this Mar 28, 2025
@yanavlasov
Copy link
Contributor

/wait-any

@ravenblackx
Copy link
Contributor

ravenblackx commented Mar 28, 2025

We are not blocked any longer and actively working internally on completing the codec migration. We would appreciate understanding on this issue and give us more time to avoid carrying a large patch internally.

I can understand why you'd want that, but anyone else would have to make the choice between "carry a patch, or don't update the version we use until we've fixed our internal issues"; blocking the external fix wouldn't even be an option.

Perhaps as a compromise we could set a date? My view here is that carrying a patch for a short time is really a tiny burden, so treating it as a painful blocker implies that you think you'd be carrying the patch for a long time, which in turn implies that the expectation is to block this for a long time. Once that begins, there will be no incentive to fix it internally rather than "working on something higher priority" because this fix will have no priority, as it costs ~nothing to continue to block the PR (though it's a pain for the PR owner who now has to maintain this PR just as much as Google would have to maintain that hypothetical patch).

If there's a target date at which this cleanup will be committed then the internal fix would at least have a real meaningful priority, as not doing it before the date will cost "now we have to do a patch", and then once it is a patch, if that target isn't met, there'd be an ongoing patch-maintenance cost, ensuring the required updates still have some meaningful priority. (I would still contend that really we should go directly to that latter state like anyone else would have to, but a compromise seems reasonable.)

@ggreenway
Copy link
Member Author

I am fine delaying this, but agree that we should put a date on it.

@ggreenway ggreenway marked this pull request as draft March 28, 2025 18:21
Signed-off-by: Greg Greenway <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

envoy_reloadable_features_http1_use_balsa_parser deprecation
7 participants