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

pkg/wellknown: Update gzip wellknown name #354

Closed
wants to merge 1 commit into from

Conversation

stevesloka
Copy link
Member

This bumps the the Gzip HTTP filter wellknown name.

Signed-off-by: Steve Sloka [email protected]

@stevesloka
Copy link
Member Author

As an aside, I was trying to create the filter via a typedConfig to avoid updating these all the time, but I get this error: Proto constraint validation failed (HttpConnectionManagerValidationError.HttpFilters[i]: ["embedded message failed validation"] | caused by HttpFilterValidationError.Name: ["value length must be at least " '\x01' " bytes"])

I think this is resolved in the v3 api though?

&http.HttpFilter{
  ConfigType: &http.HttpFilter_TypedConfig{
	TypedConfig: &any.Any{
		TypeUrl: "type.googleapis.com/envoy.config.filter.http.gzip.v2.Gzip",
	},
   },
},

@jpeach
Copy link
Contributor

jpeach commented Sep 19, 2020

IIUC you have to put a name but it can be anything.

Doesn't changing the well known names have compatibility issues? If an app upgrades this package but not envoy, it would break right?

@fxposter
Copy link
Contributor

Doesn't changing the well known names have compatibility issues? If an app upgrades this package but not envoy, it would break right?

well, last modification of this file already broke envoy 1.12 support

@jpeach
Copy link
Contributor

jpeach commented Sep 20, 2020

Doesn't changing the well known names have compatibility issues? If an app upgrades this package but not envoy, it would break right?

well, last modification of this file already broke envoy 1.12 support

#293

@stevesloka
Copy link
Member Author

IIUC you have to put a name but it can be anything.

If I put anything in for the name I get this error:

[source/common/config/grpc_subscription_impl.cc:100] gRPC config for type.googleapis.com/envoy.api.v2.Listener rejected: Error adding/updating listener(s) ingress_http: Didn't find a registered implementation for name: 'gzip'

This is the config I'm using:

&http.HttpFilter{
  Name: "gzip",
  ConfigType: &http.HttpFilter_TypedConfig{
	TypedConfig: &any.Any{
		TypeUrl: "type.googleapis.com/envoy.extensions.filters.http.compressor",
	},
    },
},

From another Slack thread (can't find the link now), this should work in v3 api's but not v2.

Is there something I'm missing? Or should this only work with v3?

@jpeach
Copy link
Contributor

jpeach commented Sep 22, 2020

@htuch is the well-known filter name still required in the v2 API?

@htuch
Copy link
Member

htuch commented Sep 23, 2020

@jpeach as long as you are using some extension config wrapper that has explicit type URLs, you shouldn't need to use an explicit well known name.

@stevesloka
Copy link
Member Author

@htuch can you look at my last comment in this thread? I tried to do that, but still get errors unless the name is something that matches.

@htuch
Copy link
Member

htuch commented Sep 23, 2020

It looks like we are registering both v2 and v3 type URLs in the registry, so not sure why it wouldn't work. @kyessenov any thoughts?

@kyessenov
Copy link
Contributor

I think it has something to do with HCM being v2 here. Let me try it out locally.

@kyessenov
Copy link
Contributor

BTW the type should be envoy.extensions.filters.http.compressor.v3.Compressor.

@kyessenov
Copy link
Contributor

Re: well known names: yes, it does break updates of this library in non-trivial ways. Istio has this arbitrary patching mechanism (EnvoyFilter) that can refer to parts of xDS by name, so when we bump this library, the output changes slightly and breaks some EnvoyFilters. Unless you really have to use the name, I'd just use the type since it takes precedence anyways.

@kyessenov
Copy link
Contributor

This works:

          - name: gzip
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor
              compressor_library:
                name: "text_optimized"
                typed_config:
                  "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
                  memory_level: 3
                  window_bits: 10
                  compression_level: "best_compression"
                  compression_strategy: "default_strategy"

@stevesloka
Copy link
Member Author

Thanks for the snippet @kyessenov, but I'm trying to get this to work with v2 at the moment. I'll just leave this as-is since we're in the middle of upgrading to v3. Once there I think this will work then. Thanks!

@stevesloka stevesloka closed this Oct 9, 2020
@stevesloka
Copy link
Member Author

Just an update, we finally moved to xDS v3 and with the following config it works. Thanks for the tips @kyessenov!

&http.HttpFilter{
			Name: wellknown.Gzip,
			ConfigType: &http.HttpFilter_TypedConfig{
				TypedConfig: protobuf.MustMarshalAny(&compressor.Compressor{
					CompressorLibrary: &envoy_core_v3.TypedExtensionConfig{
						Name: "gzip",
						TypedConfig: &any.Any{
							TypeUrl: "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip",
						},
					},
				}),
			},
		},

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.

5 participants