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

Removes usage of deprecated http2_protocol_options from Cluster in configs #15563

Merged
merged 2 commits into from
Mar 19, 2021

Conversation

davinci26
Copy link
Member

@davinci26 davinci26 commented Mar 19, 2021

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Commit Message:

Removes usage of deprecated http2_protocol_options from Cluster in configs. See #15559

Additional Description:

This patches the underlying issue that these configs should have caused tests to fail. This PR does not address this.
Required for #15461

Risk Level: Low (test only)
Testing: Tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26 davinci26 changed the title fixes deprecated configs Removes usage of deprecated http2_protocol_options from Cluster in configs Mar 19, 2021
@davinci26
Copy link
Member Author

davinci26 commented Mar 19, 2021

@phlax @alyssawilk @mattklein123 all of you are involved in the issue discussion so you might want to also do the review.

Important: this does not fix the underlying issue, the tests should have been failing when we use a deprecated property. I don't fix it in this PR because I want to unblock #15461 which I want to get for 1.18 since it is important for Windows.

Fixing the underlying issue is a test only change so we could fix it after the release.

Signed-off-by: davinci26 <sotirisnan@gmail.com>
@phlax
Copy link
Member

phlax commented Mar 19, 2021

hi @davinci26 this pr raises a few issues

firstly the example configs. I did a quick check and i think in most case we can just use HTTP/1.1 for the examples. I think this is preferable as the config to switch is complicated and mostly the examples want to just focus on what is exemplary.

im guessing some time in the future http2 will become the default and we wont need to set this at all to use HTTP/2.0

the other issue is not addressed in (or caused by) this PR - but it does raise the issue.

replacing http2_protocol_options with

    typed_extension_protocol_options:
      envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
        "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
        explicit_http_config:
          http2_protocol_options: {}

doesnt seem very user-friendly to me.

Especially as with this config the user is not actually setting any http2 options - they are switching http protocol. Despite the word options appearing in the config 4 times, the options are actually {} (which may mean "use the defaults" or could be "blank the defaults")

@phlax
Copy link
Member

phlax commented Mar 19, 2021

the other issue is docs - reading the http2_protocol_option here:

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/cluster.proto.html

its not clear/obvous that this has been deprecated, or how to upgrade

@davinci26
Copy link
Member Author

davinci26 commented Mar 19, 2021

firstly the example configs. I did a quick check and i think in most case we can just use HTTP/1.1 for the examples. I think this is preferable as the config to switch is complicated and mostly the examples want to just focus on what is exemplary.

I see what you are saying. No strong opinion here. I just wanted my change to not affect the behavior. Also from a quick search in the repository, I see that there are many configs that already use the new verbose way of setting it.

replacing http2_protocol_options with doesnt seem very user-friendly to me.

I can see your argument but I am not well versed here to have an opinion. Would an issue be better to discuss this?

the other issue is docs - reading the http2_protocol_option here: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/cluster.proto.html its not clear/obvous that this has been deprecated, or how to upgrade

The link in there is broken, that makes it really confusing. We should patch this (I can do this in a separate PR)

@phlax
Copy link
Member

phlax commented Mar 19, 2021

Also from a quick search in the repository, I see that there are many configs that already use the new verbose way of setting it.

in terms of reverting to http1, i think im mostly concerned about the configs in the examples/sandboxes - just because they should be kept as simple as possible

The link in there is broken, that makes it really confusing. We should patch this (I can do this in a separate PR)

yep. i think we also need a really standout way of flagging deprecation and perhaps new/old ways of doing things - beyond the scope of this PR tho, so sorry for hijacking it 8/

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Let's merge this to unblock @davinci26. I agree with @phlax that we can improve the documentation and simplify things, and I also agree that this new config is not very user friendly, but so it goes.

In terms of fixing the underlying reason it wasn't caught previously, we should have a way of not allowing parsing as v2 at all, but I'm not sure it's worth investing in that as we are about to delete v2 anyway.

So, let's merge and figure out doc improvements separately.

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.

4 participants