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

internal/envoy: Update GZip to valid wellknown name #2926

Closed

Conversation

stevesloka
Copy link
Member

Updates: #2479

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

@stevesloka stevesloka added this to the 1.9.0 milestone Sep 18, 2020
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #2926 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2926   +/-   ##
=======================================
  Coverage   74.88%   74.88%           
=======================================
  Files          83       83           
  Lines        5582     5582           
=======================================
  Hits         4180     4180           
  Misses       1311     1311           
  Partials       91       91           
Impacted Files Coverage Δ
internal/envoy/listener.go 96.99% <100.00%> (ø)

@jpeach
Copy link
Contributor

jpeach commented Sep 19, 2020

IIUC, the name can be anything (at least with current Envoy versions); just specifying the typed config is enough envoy do do the right thing.

@youngnick
Copy link
Member

I don't understand what's happening here. @stevesloka, is this PR just to clear the messages from the Envoy logs? It seems like the better long-term fix is to move the filter config to the typed configs, and to stop using the names altogether?

@stevesloka
Copy link
Member Author

@youngnick yes the goal of this is to update the names as the current one's are deprecated and will stop working at some point.

I'd like to move to the typedconfig, but can't seem to get that to work (with xDS v2 at least): envoyproxy/go-control-plane#354 (comment)

For now this bumps it down the road to avoid compatibility issues until we get it sorted out properly.

@skriss
Copy link
Member

skriss commented Sep 22, 2020

Per this Slack thread (linked in #2479) it seems like the typed-config thing is only supported in v3.

@skriss
Copy link
Member

skriss commented Sep 22, 2020

@stevesloka I'm seeing this error in the Contour log when I test this change:

ERRO[0011] Error adding/updating listener(s) ingress_http: Compressor filter doesn't have compressor_library defined  code=13 connection=6 context=xds node_id=envoy-2s99g node_version=v1.15.0 response_nonce=1 version_info=2

@skriss skriss modified the milestones: 1.9.0, 1.10.0 Oct 6, 2020
@stevesloka
Copy link
Member Author

Closing this since the typed config isn't working with v2 xds types. Will come back when implementing v3 and I think it will work then avoiding this issue in the future.

@stevesloka stevesloka closed this Oct 9, 2020
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