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

Allow custom local address resolvers. #27705

Merged
merged 84 commits into from
Sep 8, 2023
Merged

Conversation

pradeepcrao
Copy link
Contributor

@pradeepcrao pradeepcrao commented May 30, 2023

Commit Message:
Additional Description:

#27881 introduces the concept of EDS clusters with hosts that have multiple (potentially > 2) IP addresses.

The current implementation of UpstreamLocalAddressSelector limits the number of source addresses in BindConfig artificially to 2, and further requires that the addresses be of different address families.

The workaround for this (if we need to specify more than 2 source addresses or have multiple addresses from the same family) is to use a custom address resolver that resolves the bind config address to nullptr (and therefore ignore it) and call bind in a customised SocketInterfaceImpl to a local source address determined by the SocketInterfaceImpl specialisation.

This PR makes it possible to define a custom local address selector, that makes it easy to work with a custom address resolver to pick the right source address based on the upstream address selected by HappyEyeballsConnectionImpl

Risk Level: Low
Testing: Added test.
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: N/A

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #27705 was opened by pradeepcrao.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27705 was opened by pradeepcrao.

see: more, trace.

Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
Signed-off-by: pcrao <[email protected]>
@pradeepcrao pradeepcrao marked this pull request as ready for review June 14, 2023 15:11
@pradeepcrao
Copy link
Contributor Author

/retest

@pradeepcrao
Copy link
Contributor Author

/assign @adisuissa

@soulxu
Copy link
Member

soulxu commented Jun 20, 2023

This PR makes it possible to define a custom local address selector, that makes it easy to work with a custom address resolver to pick the right source address based on the upstream address selected by HappyEyeballsConnectionImpl

If we have multiple addresses for the same family, then what criteria are used to select source address from those same family addresses?

@pradeepcrao
Copy link
Contributor Author

This PR makes it possible to define a custom local address selector, that makes it easy to work with a custom address resolver to pick the right source address based on the upstream address selected by HappyEyeballsConnectionImpl

If we have multiple addresses for the same family, then what criteria are used to select source address from those same family addresses?

We have use cases where-in the source address selection is to be done based on the subnet of the selected IP address of the upstream. This PR leaves the selection criteria of the source address up to the extension implementation.

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.

/lgtm api

@pradeepcrao
Copy link
Contributor Author

/retest

yanavlasov
yanavlasov previously approved these changes Sep 6, 2023
@yanavlasov yanavlasov enabled auto-merge (squash) September 6, 2023 13:51
auto-merge was automatically disabled September 6, 2023 18:39

Head branch was pushed to by a user without write access

@soulxu soulxu enabled auto-merge (squash) September 7, 2023 12:59
Copy link
Contributor

@adisuissa adisuissa 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 api

@repokitteh-read-only repokitteh-read-only bot removed the api label Sep 7, 2023
@yanavlasov
Copy link
Contributor

/wait

@adisuissa
Copy link
Contributor

/retest

@soulxu soulxu merged commit da47a54 into envoyproxy:main Sep 8, 2023
@moderation
Copy link
Contributor

moderation commented Sep 8, 2023

After this PR, I'm getting an error launching my standard instances. Any ideas on what is going on here? A config change required? /cc @pradeepcrao

[2023-09-08 09:27:09.517][12285440][critical][main] [source/server/server.cc:134] error initializing config '  sync-service-envoy-v3.json': Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'
[2023-09-08 09:27:09.517][12285440][info][main] [source/server/server.cc:988] exiting
Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'

@pradeepcrao
Copy link
Contributor Author

After this PR, I'm getting an error launching my standard instances. Any ideas on what is going on here? A config change required? /cc @pradeepcrao

[2023-09-08 09:27:09.517][12285440][critical][main] [source/server/server.cc:134] error initializing config '  sync-service-envoy-v3.json': Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'
[2023-09-08 09:27:09.517][12285440][info][main] [source/server/server.cc:988] exiting
Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'

You should not need a config change. What is sync-service-envoy-v3.json?

@pradeepcrao
Copy link
Contributor Author

After this PR, I'm getting an error launching my standard instances. Any ideas on what is going on here? A config change required? /cc @pradeepcrao

[2023-09-08 09:27:09.517][12285440][critical][main] [source/server/server.cc:134] error initializing config '  sync-service-envoy-v3.json': Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'
[2023-09-08 09:27:09.517][12285440][info][main] [source/server/server.cc:988] exiting
Didn't find a registered implementation for '' with type URL: 'envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector'

You should not need a config change. What is sync-service-envoy-v3.json?

What do you use to build Envoy? Can you provide repro steps?

The only place envoy.config.upstream.local_address_selector.v3.DefaultLocalAddressSelector is used is here, https://github.com/envoyproxy/envoy/blob/main/source/common/upstream/upstream_impl.cc#L349-L354 and the factory registration is a dependency for upstream_impl.cc : https://github.com/envoyproxy/envoy/blob/main/source/common/upstream/BUILD#L396

@moderation
Copy link
Contributor

moderation commented Sep 8, 2023

I'm building on Google RBE for Linux. I'm now getting the same error on my Linux and MacOS hosts and MacOS is built locally. All of my Envoy instances generate this error. Even running against https://github.com/envoyproxy/envoy/blob/main/configs/envoyproxy_io_proxy.yaml generates this issue.

I'm suspecting that I'm compiling out an extension that is now required to make this work. Checking now.

There are no new extensions for this PR for https://github.com/envoyproxy/envoy/commits/main/source/extensions/extensions_build_config.bzl

@pradeepcrao
Copy link
Contributor Author

@pradeepcrao
Copy link
Contributor Author

This PR adds a core "extension" : https://github.com/envoyproxy/envoy/blob/main/source/common/upstream/default_local_address_selector_factory.cc#L72

Should this new extension be included in https://github.com/envoyproxy/envoy/commits/main/source/extensions/extensions_build_config.bzl ?

It should not need to be there as it's not in source/extensions and is already a build dependency for a core target.
Can you confirm if adding it there fixes the issue for you?

@moderation
Copy link
Contributor

I just built in a different work environment and same thing. bazel-bin/source/exe/envoy-static.stripped --config-path configs/envoyproxy_io_proxy.yaml generates the error. There must be an extension that I'm compiling out that it now required.

# See bazel/README.md for details on how this system works.
EXTENSIONS = {
    # Access loggers
    "envoy.access_loggers.file": "//source/extensions/access_loggers/file:config",
    "envoy.access_loggers.open_telemetry": "//source/extensions/access_loggers/open_telemetry:config",
    "envoy.access_loggers.stdout": "//source/extensions/access_loggers/stream:config",
    "envoy.access_loggers.stderr": "//source/extensions/access_loggers/stream:config",

    # Clusters
    "envoy.clusters.dynamic_forward_proxy": "//source/extensions/clusters/dynamic_forward_proxy:cluster",
    "envoy.clusters.logical_dns": "//source/extensions/clusters/logical_dns:logical_dns_cluster_lib",
    "envoy.clusters.static": "//source/extensions/clusters/static:static_cluster_lib",
    "envoy.clusters.strict_dns": "//source/extensions/clusters/strict_dns:strict_dns_cluster_lib",

    # Compression
    "envoy.compression.brotli.compressor": "//source/extensions/compression/brotli/compressor:config",
    "envoy.compression.brotli.decompressor": "//source/extensions/compression/brotli/decompressor:config",
    "envoy.compression.gzip.compressor": "//source/extensions/compression/gzip/compressor:config",
    "envoy.compression.gzip.decompressor": "//source/extensions/compression/gzip/decompressor:config",

    # Health checkers
    "envoy.health_checkers.tcp": "//source/extensions/health_checkers/tcp:health_checker_lib",
    "envoy.health_checkers.http": "//source/extensions/health_checkers/http:health_checker_lib",
    "envoy.health_checkers.grpc": "//source/extensions/health_checkers/grpc:health_checker_lib",

    # HTTP filters
    "envoy.filters.http.alternate_protocols_cache": "//source/extensions/filters/http/alternate_protocols_cache:config",
    "envoy.filters.http.buffer": "//source/extensions/filters/http/buffer:config",
    "envoy.filters.http.cache": "//source/extensions/filters/http/cache:config",
    "envoy.filters.http.compressor": "//source/extensions/filters/http/compressor:config",
    "envoy.filters.http.cors": "//source/extensions/filters/http/cors:config",
    "envoy.filters.http.csrf": "//source/extensions/filters/http/csrf:config",
    "envoy.filters.http.decompressor": "//source/extensions/filters/http/decompressor:config",
    "envoy.filters.http.dynamic_forward_proxy": "//source/extensions/filters/http/dynamic_forward_proxy:config",
    "envoy.filters.http.ext_authz": "//source/extensions/filters/http/ext_authz:config",
    "envoy.filters.http.health_check": "//source/extensions/filters/http/health_check:config",
    "envoy.filters.http.jwt_authn": "//source/extensions/filters/http/jwt_authn:config",
    "envoy.filters.http.local_ratelimit": "//source/extensions/filters/http/local_ratelimit:config",
    "envoy.filters.http.oauth2": "//source/extensions/filters/http/oauth2:config",
    "envoy.filters.http.original_src": "//source/extensions/filters/http/original_src:config",
    "envoy.filters.http.ratelimit": "//source/extensions/filters/http/ratelimit:config",
    "envoy.filters.http.rbac": "//source/extensions/filters/http/rbac:config",
    "envoy.filters.http.router": "//source/extensions/filters/http/router:config",

    # Listener filters
    "envoy.filters.listener.http_inspector": "//source/extensions/filters/listener/http_inspector:config",
    # NOTE: The original_dst filter is implicitly loaded if original_dst functionality is
    #       configured on the listener. Do not remove it in that case or configs will fail to load.
    "envoy.filters.listener.original_dst": "//source/extensions/filters/listener/original_dst:config",
    "envoy.filters.listener.original_src": "//source/extensions/filters/listener/original_src:config",
    # NOTE: The proxy_protocol filter is implicitly loaded if proxy_protocol functionality is
    #       configured on the listener. Do not remove it in that case or configs will fail to load.
    "envoy.filters.listener.proxy_protocol": "//source/extensions/filters/listener/proxy_protocol:config",
    "envoy.filters.listener.tls_inspector": "//source/extensions/filters/listener/tls_inspector:config",

    # Network filters
    "envoy.filters.network.direct_response": "//source/extensions/filters/network/direct_response:config",
    "envoy.filters.network.ext_authz": "//source/extensions/filters/network/ext_authz:config",
    "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config",
    "envoy.filters.network.local_ratelimit": "//source/extensions/filters/network/local_ratelimit:config",
    "envoy.filters.network.ratelimit": "//source/extensions/filters/network/ratelimit:config",
    "envoy.filters.network.rbac": "//source/extensions/filters/network/rbac:config",
    "envoy.filters.network.sni_dynamic_forward_proxy": "//source/extensions/filters/network/sni_dynamic_forward_proxy:config",
    "envoy.filters.network.tcp_proxy": "//source/extensions/filters/network/tcp_proxy:config",

    # UDP filters
    "envoy.filters.udp_listener.udp_proxy": "//source/extensions/filters/udp/udp_proxy:config",

    # Stat sinks
    "envoy.stat_sinks.open_telemetry": "//source/extensions/stat_sinks/open_telemetry:config",
    "envoy.stat_sinks.statsd": "//source/extensions/stat_sinks/statsd:config",

    # Tracers
    "envoy.tracers.opentelemetry": "//source/extensions/tracers/opentelemetry:config",

    # Transport sockets
    "envoy.transport_sockets.raw_buffer": "//source/extensions/transport_sockets/raw_buffer:config",
    "envoy.transport_sockets.tcp_stats": "//source/extensions/transport_sockets/tcp_stats:config",
    "envoy.transport_sockets.internal_upstream": "//source/extensions/transport_sockets/internal_upstream:config",
    "envoy.transport_sockets.upstream_proxy_protocol": "//source/extensions/transport_sockets/proxy_protocol:upstream_config",

    # CacheFilter plugins
    "envoy.extensions.http.cache.file_system_http_cache": "//source/extensions/http/cache/file_system_http_cache:config",
    "envoy.extensions.http.cache.simple": "//source/extensions/http/cache/simple_http_cache:config",

    # Http Upstreams (excepting envoy.upstreams.http.generic which is hard-coded into the build so not registered here)
    "envoy.upstreams.http.http": "//source/extensions/upstreams/http/http:config",
    "envoy.upstreams.http.tcp": "//source/extensions/upstreams/http/tcp:config",
    "envoy.upstreams.http.udp": "//source/extensions/upstreams/http/udp:config",
    # "envoy.upstream.local_address_selector": "//source/extensions/upstream:default_local_address_selector_factory",

    # IO socket
    "envoy.io_socket.user_space": "//source/extensions/io_socket/user_space:config",
    "envoy.bootstrap.internal_listener": "//source/extensions/bootstrap/internal_listener:config",

    # QUIC extensions
    "envoy.quic.deterministic_connection_id_generator": "//source/extensions/quic/connection_id_generator:envoy_deterministic_connection_id_generator_config",
    "envoy.quic.crypto_stream.server.quiche": "//source/extensions/quic/crypto_stream:envoy_quic_default_crypto_server_stream",
    "envoy.quic.proof_source.filter_chain": "//source/extensions/quic/proof_source:envoy_quic_default_proof_source",
    "envoy.quic.server_preferred_address.fixed": "//source/extensions/quic/server_preferred_address:fixed_server_preferred_address_config_factory_config",

    # UDP packet writers
    "envoy.udp_packet_writer.default": "//source/extensions/udp_packet_writer/default:config",
    "envoy.udp_packet_writer.gso": "//source/extensions/udp_packet_writer/gso:config",

    # Key value store
    "envoy.key_value.file_based": "//source/extensions/key_value/file_based:config_lib",

    # RBAC matchers
    "envoy.rbac.matchers.upstream_ip_port": "//source/extensions/filters/common/rbac/matchers:upstream_ip_port_lib",

    # DNS Resolver
    # c-ares DNS resolver extension is recommended to be enabled to maintain the legacy DNS resolving behavior.
    "envoy.network.dns_resolver.cares": "//source/extensions/network/dns_resolver/cares:config",
    # apple DNS resolver extension is only needed in MacOS build plus one want to use apple library for DNS resolving.
    "envoy.network.dns_resolver.apple": "//source/extensions/network/dns_resolver/apple:config",

    # Header Validators
    "envoy.http.header_validators.envoy_default": "//source/extensions/http/header_validators/envoy_default:config",

    # Early Data option
    "envoy.route.early_data_policy.default": "//source/extensions/early_data:default_early_data_policy_lib",
}

# These can be changed to ["//visibility:public"], for  downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config", "//:contrib_library", "//:examples_library", "//:mobile_library"]
EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library", "//:contrib_library", "//:examples_library", "//:mobile_library"]
CONTRIB_EXTENSION_PACKAGE_VISIBILITY = ["//:contrib_library"]
MOBILE_PACKAGE_VISIBILITY = ["//:mobile_library"]

# Set this variable to true to disable alwayslink for envoy_cc_library.
# TODO(alyssawilk) audit uses of this in source/ and migrate all libraries to extensions.
LEGACY_ALWAYSLINK = 1

@moderation
Copy link
Contributor

I'm unblocked for now going back to the commit before this one with git checkout 2cb9401202cc1fb6ac7c33317c7197754462da46 in case anyone else is affected

@moderation
Copy link
Contributor

@pradeepcrao I've worked it out. If you have build --define library_autolink=disabled defined in a custom .bazelrc the resulting binary won't run any config with the error above being produced

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.

9 participants