Converting envoy configs to V2 #2957
Conversation
srikailash
changed the title
|
@srikailash this looks on the right track. Can you ping us when this is ready for review? (Basically we just want to replace all the JSON stuff with pure v2 and get similar configs). |
|
@srikailash Please merge master. That has a fix for the failing ipv6 tests. |
|
@ccaraman done, thanks. |
configs/generate_v2_config.py
Outdated
| @@ -0,0 +1,65 @@ | |||
| #Import necessary functions from Jinja2 module | |||
There was a problem hiding this comment.
will remove this file before final merging.
|
@mattklein123 , this is ready for review. |
There was a problem hiding this comment.
Nice!!!
@junr03 can you review as this is related to what you have done internally?
@srikailash we should remove the v1 config generators as part of this change. Can you also make sure the new files are being config tested as part of tests? Thanks!!!
|
@mattklein123 , please elaborate on this 'Can you also make sure the new files are being config tested as part of tests?'. Does that mean starting envoy with generated yaml and manually testing? Thanks! |
|
@srikailash take a look at https://github.com/envoyproxy/envoy/blob/master/configs/BUILD. We test all the configs that are output by configgen. So the result of this change should be to delete all the JSON output and instead test all of the YAML output v2 configs. |
There was a problem hiding this comment.
@srikailash thanks for doing this! I got you started with all my comments for one of the templates. However, what I noticed is that you did a 1:1 translation of the templates from JSON to yaml. Unfortunately the transition from v1 to v2 is also structural and a lot of the configuration is different.
I gave you all the pointers I found in one of the templates. Please take that, and the docs for the v2 format https://www.envoyproxy.io/docs/envoy/latest/api-v2/api to rewrite the remaining templates. I can take a look again after you go through them.
| @@ -0,0 +1,110 @@ | |||
| --- | |||
| {% macro listener(address,ssl,proxy_proto) -%} | |||
| - address: "{{ address }}" | |||
There was a problem hiding this comment.
this address needs to follow https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/core/address.proto#envoy-api-msg-core-address
There was a problem hiding this comment.
Needs to look akin to:
- address:
socket_address:
address: {{ address }}
protocol: {{ protocol }}
port_value: {{ port }}
| {% macro listener(address,ssl,proxy_proto) -%} | ||
| - address: "{{ address }}" | ||
| {% if ssl -%} | ||
| ssl_context: |
There was a problem hiding this comment.
this field has changed a lot. It now hangs of off the filter_chains field and its organization has changed quite a bit: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/auth/cert.proto#envoy-api-msg-auth-downstreamtlscontext
| private_key_file: certs/serverkey.pem | ||
| {% endif -%} | ||
| {% if proxy_proto -%} | ||
| use_proxy_proto: true |
There was a problem hiding this comment.
this also hangs off of filter_chains
| {% if proxy_proto -%} | ||
| use_proxy_proto: true | ||
| {% endif -%} | ||
| filters: |
There was a problem hiding this comment.
This also hangs off of filter_chains. Take a looks at the listener fields: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/lds.proto#envoy-api-msg-listener and also the filter_chains field: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/listener/listener.proto#envoy-api-msg-listener-filterchain
| filters: | ||
| - name: http_connection_manager | ||
| config: | ||
| codec_type: auto |
There was a problem hiding this comment.
values that are enums should be all upper case. ex: AUTO
| cluster_manager: | ||
| clusters: | ||
| - name: statsd | ||
| connect_timeout_ms: 250 |
There was a problem hiding this comment.
same as comment above about duration fields
| - name: statsd | ||
| connect_timeout_ms: 250 | ||
| type: static | ||
| lb_type: round_robin |
There was a problem hiding this comment.
the field name is now lb_policy. enums need to be all upper case
| type: static | ||
| lb_type: round_robin | ||
| hosts: | ||
| - url: tcp://127.0.0.1:8125 |
There was a problem hiding this comment.
the host list needs to follow the address msg type: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/core/address.proto#envoy-api-msg-core-address
| lb_type: round_robin | ||
| features: http2 | ||
| max_requests_per_connection: 25000 | ||
| ssl_context: |
There was a problem hiding this comment.
this field is quite different now follow: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/auth/cert.proto#envoy-api-msg-auth-upstreamtlscontext
| - front-proxy.yourcompany.net | ||
| hosts: | ||
| - url: tcp://front-proxy.yourcompany.net:9400 | ||
| - name: lightstep_saas |
There was a problem hiding this comment.
both this, and the above cluster have the same comments as the statsd cluster
|
@junr03 ,thanks for detailed comments. Will let you know after updating. |
|
@junr03 mind having a quick look at above commit changes. |
There was a problem hiding this comment.
@srikailash looking better. There are still some problems, but heading in the right direction. Let me know when you have updated
| protocol: | ||
| address: tcp://127.0.0.1 | ||
| port_value: 9901 | ||
| named_port: |
There was a problem hiding this comment.
this field and the two below are not needed
| access_log_path: "var/log/envoy/admin_access.log" | ||
| address: | ||
| socket_address: | ||
| protocol: |
| address: | ||
| socket_address: | ||
| protocol: | ||
| address: tcp://127.0.0.1 |
There was a problem hiding this comment.
the address should just be 127.0.0.1
| runtime: | ||
| symlink_root: /srv/runtime_data/current | ||
| subdirectory: envoy | ||
| override_subdirectory: envoy_override |
There was a problem hiding this comment.
nit: not sure why, but there are many fields like this that have two spaces, can you fix?
| type: STATIC | ||
| connect_timeout: 0.25s | ||
| lb_policy: ROUND_ROBIN | ||
| hosts: |
| lb_policy: ROUND_ROBIN | ||
| hosts: | ||
| socket_address: | ||
| address: tcp://127.0.0.1 |
There was a problem hiding this comment.
same comment as above on the address. The protocol should be in its own field, and the address part should only be the 127.0.0.1 part
| socket_address: | ||
| address: tcp://127.0.0.1 | ||
| port: 8125 | ||
| - name: backhaul |
There was a problem hiding this comment.
same comments as the ones above for this cluster and the one below
|
@junr03, fixed problems mentioned by you. please take a look. |
|
@srikailash do you mind solving the CI issues first, then I'll take a look at the whole change. Thanks! |
configs/BUILD
Outdated
| ]), | ||
| external_deps = ["jinja2"], | ||
| external_deps = ["jinja2", "yaml"], |
There was a problem hiding this comment.
@junr03 should this line be enough to import yaml in configgen.py? I see tests are failing because of "import yaml" in configgen.py. Thanks!
There was a problem hiding this comment.
@srikailash you need to add it to the python deps like jinja2. Take a look at how that is done
Line 125 in a328bc5
envoy/bazel/external/jinja.BUILD
Line 1 in a328bc5
envoy/bazel/repository_locations.bzl
Line 65 in a328bc5
There was a problem hiding this comment.
@junr03, friendly ping. :)
There was a problem hiding this comment.
Sorry, didn't see this comment before. I'm away at a conference. Do you mind going to the #bazel channel on the envoy slack to ask for advice on this case?
There was a problem hiding this comment.
@junr03, tried asking in #bazel channel and had some discussion with Kuat. Couldn't solve the issue. Any thoughts on how to fix this? Thanks!
bazel/repositories.bzl
Outdated
| name = "pyyaml", | ||
| actual = "@com_github_yaml_pyyaml//:pyyaml", | ||
| name = "yaml", | ||
| actual = "@com_github_yaml_pyyaml//:lib/yaml", |
There was a problem hiding this comment.
I think the actual name should be @com_github_yaml_pyyaml//:yaml. The external workspace binds only top-level names.
There was a problem hiding this comment.
@kyessenov, thanks for checking. I guess that should be the directory after cloning from com_github_yaml_pyyaml defined in repository_locations.bzl. I made that change and couldn't fix build. Could be please take a look again? Thanks in advance!
There was a problem hiding this comment.
Hm, This name resolves to the py_library in pyyaml.BUILD. You can rename that library to lib/yaml or use :yaml here, either way is fine, I think.
There was a problem hiding this comment.
@kyessenov , changed with 5602eed
There was a problem hiding this comment.
@kyessenov, friendly ping. :)
There was a problem hiding this comment.
Sorry, missed notifications :( Bazel stuff looks fine to me.
|
@srikailash coming back to this. What do you need from us to move this forward? Should I do another review? Did we resolve all the bazel/build issues? |
|
@junr03, thanks for checking this again. I need help with two things
|
|
@jmillikin-stripe do you have a cycle to help @srikailash with his dependency issue? |
|
@srikailash for 2. DM me on the envoy slack and we can iterate on that if you have specific questions |
|
Let me pull your fork and try out bazel build locally. |
|
Can you try this: |
|
@kyessenov, thanks for the fix. importing error is fixed, now it is for me to fix other tests. |
| @@ -15,7 +15,7 @@ TEST(ExampleConfigsTest, All) { | |||
| RELEASE_ASSERT(::getcwd(cwd, PATH_MAX) != nullptr); | |||
| RELEASE_ASSERT(::chdir(directory.c_str()) == 0); | |||
|
|
|||
| EXPECT_EQ(26UL, ConfigTest::run(directory)); | |||
| EXPECT_EQ(24UL, ConfigTest::run(directory)); | |||
There was a problem hiding this comment.
will update this once other configs are up-to-date.
|
@srikailash is this ready for review again or are more changes forthcoming? |
|
@zuercher i asked Jose for a review just to make sure it is in the right path, but yeah there are some updates that needs to be done in service_to_service template. |
|
@srikailash Thanks for the update. Please give him a mention here when you're ready for another look. |
|
@junr03, this is ready for review now.
|
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
Will update and reopen the PR. Thanks :) |
|
Hello, building upstream envoy fails with
couldn't get a response in slack. please let me know how to resolve this blocker. Thanks |
|
Hi @srikailash, what version of bazel do you use? Are you on osx or linux? |
|
@dio , thanks and this is the version : 0.11.0-homebrew and i am on osx. |
|
@srikailash I think it's worth to do |
|
@dio, build still fails with |
|
bazel run //tools:v1_to_bootstrap envoy_double_proxy.json fails to build on upstream with this error |
|
@srikailash can you please blow away your entire dev environment and setup from scratch along with having the most recent versions of all the packages? If it still doesn't work I would get in Slack and ask there either in the OSX room if you are using OSX or in the normal dev room if you are using Linux. |
|
@mattklein123, tried with new dev environment. Still have the same issue. |
Signed-off-by: sri kailash <[email protected]>
stale
bot
removed
the
stale
bazel/repositories.bzl
Outdated
| @@ -173,14 +173,21 @@ def _python_deps(): | |||
| actual = "@com_github_pallets_jinja//:jinja2", | |||
| ) | |||
| _repository_impl( | |||
| name = "com_github_yaml_pyyaml", | |||
| build_file = "@envoy//bazel/external:pyyaml.BUILD", | |||
| name = "com_github_apache_thrift", | |||
There was a problem hiding this comment.
I think this must be a merge problem, right?
| runtime_key: access_log.access_error.duration | ||
| {% endif %} | ||
| config: | ||
| path: /var/log/envoy/access_error.log |
| - key: database | ||
| value: "{{ key }}" | ||
| {% endif %} | ||
| {% if not loop.last %}{% endif -%} |
| cds_config: | ||
| api_config_source: | ||
| cluster_names: | ||
| - sds |
Signed-off-by: sri kailash <[email protected]>
|
@junr03 , this is ready for another pass. |
|
@junr03 @danielhochman friendly ping PTAL. I would love to land this next week. |
There was a problem hiding this comment.
Sorry for the delay, was coming back from vacation.
Alright, I can't see anything else missing here at plain sight, and we have gone through the due diligence of comparing with the conversion tool. I think we should check in. And if there are needed changes by the community deal with them in small PRs.
Thanks for your hard work here @srikailash
|
Thanks for merging and review process, this helped to learn about envoy more. |
* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <[email protected]>
The format (and filenames) changed in envoyproxy#2957. Signed-off-by: Will Hayworth <[email protected]>
The format (and filenames) changed in #2957. Signed-off-by: Will Hayworth <[email protected]>
…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <[email protected]>
This is work in progress PR for converting envoy configs to v2. (#2139)
Signed-off-by: sri kailash [email protected]
title: Converting envoy configs to v2.
Description:
added yaml files for envoy_front_proxy.template, envoy_double_proxy.template, envoy_router.template, routing_helper.template and added a temporary generate_v2_config file which will be removed later on.
Used json2yaml.com to transform json files to yaml.
Risk Level: Low
Testing:
Need to test with generated configs. this is under todo list.
Docs Changes:
TODO
TODO: