Skip to content

Documentation: Update documentation and examples to remove warnings due to Struct deprecation for Any#6025

Merged
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
moderation:master
Feb 27, 2019
Merged

Documentation: Update documentation and examples to remove warnings due to Struct deprecation for Any#6025
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
moderation:master

Conversation

@moderation
Copy link
Contributor

Description: Update documentation and examples to remove warnings due to Struct deprecation for Any. I went to update the Google proxy example and fell into the rabbit hole of Struct deprecation in favor of Any. TODO is update templates in /configs which are currently generating deprecated config. Similarly the v1 to v2 scripts output deprecated config.
Risk Level: Low. Documentation or examples only. There are some YAML updates that are processed in testing but this should be OK.
Testing: bazel test //test/...
Docs Changes: included in PR
Release Notes: none required

Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
@mattklein123
Copy link
Member

Thanks a ton @moderation. Assigning over to @lizan. I do wonder if we should actually deprecate the struct version at all, and might consider undeprecating it and supporting both options for ease of configuration. cc @htuch

@moderation
Copy link
Contributor Author

I think it is OK to deprecate Struct with the understanding it is going to be a lot of work to update all the references to the old way. I haven't even looked at the instances in C test code which I expect will be numerous.

The increase in complexity is primarily in defining hosts which used to be fairly concise. The new load_assignment method is verbose.

Lastly, there is a bunch of stuff like MySQL that haven't been changed over. The following config will still generate a deprecation warning but there isn't a way to use typed_config with MySQL. I haven't looked at Redis, Mongo etc.

    filter_chains:
      - filters:
          - name: envoy.filters.network.mysql_proxy
            config:
              stat_prefix: mysql_stats
          - name: envoy.tcp_proxy
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy
              stat_prefix: tcp_stats
              cluster: cluster_0

@moderation
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6025 (comment) was created by @moderation.

see: more, trace.

@lizan
Copy link
Member

lizan commented Feb 21, 2019

Thanks for taking this, my bad that didn't have a chance to make this happen earlier.

@moderation the following for MySQL should work, no?

    filter_chains:
      - filters:
          - name: envoy.filters.network.mysql_proxy
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.network.mysql_proxy.v1alpha1.MySQLProxy
              stat_prefix: mysql_stats
          - name: envoy.tcp_proxy
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy
              stat_prefix: tcp_stats
              cluster: cluster_0

@htuch
Copy link
Member

htuch commented Feb 21, 2019

I think the migration should be pretty easy for most folks who want to keep Struct, they just put it in the Any field and it works. That said, I think the plan was to make this an extended deprecation cycles, e.g. a year or something.

@htuch
Copy link
Member

htuch commented Feb 21, 2019

FWIW, the main cost of keeping Struct that I see is API inconsistency. Going forward, we want to just use Any for opaque fields, so, turning down direct Struct fields seems the way to go (eventually).

@moderation
Copy link
Contributor Author

@lizan I was able to work out your suggestion from the proto but when you try to run it you get:

[2019-02-21 11:02:38.169][2384][critical][main] [source/server/server.cc:86] error initializing configuration '/home/moderation/Library/envoyproxy/moderation/envoy/test/extensions/filters/network/mysql_proxy/mysql_test_config.yaml': Unable to parse JSON as proto (INVALID_ARGUMENT:(static_resources.listeners.filter_chains[0].filters[0].typed_config): invalid value Invalid type URL, unknown type: envoy.config.filter.network.mysql_proxy.v1alpha1.MySQLProxy for type Any): {"static_resources":{"listeners":{"filter_chains":[{"filters":[{"typed_config":{"stat_prefix":"mysql_stats","@type":"type.googleapis.com/envoy.config.filter.network.mysql_proxy.v1alpha1.MySQLProxy"},"name":"envoy.filters.network.mysql_proxy"},{"typed_config":{"@type":"type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy","cluster":"cluster_0","stat_prefix":"tcp_stats"},"name":"envoy.tcp_proxy"}]}],"name":"listener_0","address":{"socket_address":{"port_value":3307,"address":"0.0.0.0"}}},"clusters":{"load_assignment":{"endpoints":[{"lb_endpoints":[{"endpoint":{"address":{"socket_address":{"port_value":0,"address":"127.0.0.1"}}}}]}],"cluster_name":"cluster_0"},"name":"cluster_0","connect_timeout":"2s"}},"admin":{"address":{"socket_address":{"port_value":0,"address":"127.0.0.1"}},"access_log_path":"/dev/null"}}

Config used is:

admin:
  access_log_path: /dev/null
  address:
    socket_address:
      address: 127.0.0.1
      port_value: 0
static_resources:
  clusters:
    name: cluster_0
    connect_timeout: 2s
    load_assignment:
      cluster_name: cluster_0
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: 127.0.0.1
                port_value: 0
  listeners:
    name: listener_0
    address:
      socket_address:
        address: 0.0.0.0
        port_value: 3307
    filter_chains:
      - filters:
          - name: envoy.filters.network.mysql_proxy
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.network.mysql_proxy.v1alpha1.MySQLProxy
              stat_prefix: mysql_stats
          - name: envoy.tcp_proxy
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy
              stat_prefix: tcp_stats
              cluster: cluster_0

I suspect the proto at https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/mysql_proxy/v1alpha1/mysql_proxy.proto doesn't have the required information to make this work.

@lizan
Copy link
Member

lizan commented Feb 21, 2019

@moderation it worked for me, which version of envoy are you using? 6c95ed9 worked for me

@moderation
Copy link
Contributor Author

@lizan works for me too. I had the mysql extension compiled out. Doh. Once this PR is merged I'll take a stab at the outstanding items like the templates and tests.

Signed-off-by: Michael Payne <michael@sooper.org>
@moderation
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6025 (comment) was created by @moderation.

see: more, trace.

@moderation
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6025 (comment) was created by @moderation.

see: more, trace.

@moderation
Copy link
Contributor Author

moderation commented Feb 26, 2019

PTAL @lizan @mattklein123

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM modulo one nit.

listener_filters:
- name: envoy.listener.original_dst
typed_config: {}
name: original_dst
Copy link
Member

Choose a reason for hiding this comment

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

Is this the listener name, put it at top of the listener fields? While it is technically valid though I find it hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the name field as it isn't mandatory. I added it during testing as this file needed a large refactor to get to v2 config.

Signed-off-by: Michael Payne <michael@sooper.org>
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.

Amazing! Thank you!

@mattklein123 mattklein123 merged commit deae64b into envoyproxy:master Feb 27, 2019
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
…ue to Struct deprecation for Any (envoyproxy#6025)

Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this pull request Mar 27, 2019
…cation for Any and hosts deprecation for load_assignment (#6368)

Update examples for Struct deprecation for Any

Risk Level: Low - generated configs only, no changes to code
Testing: bazel build //configs:example_configs, bazel test //test/...
Docs Changes: None required
Release Notes: None required

Fixes #6025
Replaces #6356
Related #6346

Signed-off-by: Michael Payne <michael@sooper.org>
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