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

envoy: Support internal listeners in CiliumEnvoyConfig CRDs #29026

Conversation

jrajahalme
Copy link
Member

Envoy Internal listeners do not have a real listening socket, nor is the downstream connection backed by a socket, so socket options can not be applied on them. Allow use of Envoy internal listeners by refraining from injecting Cilium filters on internal listeners, so that socket options are not applied. This also means that Cilium policy enforcement is not performed on internal listeners, so they must be used with caution.

This should be addressed in a future commit to propagate Cilium metadata to the internal listener from the previous, real listener.

Note that as of now Envoy Internal listeners is not a stable feature, so Envoy emits the following warning during bootstrap:

[misc] [external/envoy/source/common/protobuf/message_validator_impl.cc:35]
message 'envoy.extensions.bootstrap.internal_listener.v3.InternalListener'
is contained in proto file
'envoy/extensions/bootstrap/internal_listener/v3/internal_listener.proto'
marked as work-in-progress. API features marked as work-in-progress are not
considered stable, are not covered by the threat model, are not supported
by the security team, and are subject to breaking changes. Do not use this
feature without understanding each of the previous points.

@jrajahalme jrajahalme added area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/misc This PR makes changes that have no direct user impact. labels Nov 7, 2023
@jrajahalme jrajahalme requested review from a team as code owners November 7, 2023 10:30
@jrajahalme
Copy link
Member Author

/test

Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

LGTM. only a non-blocking nit for better readability

pkg/envoy/ciliumenvoyconfig.go Outdated Show resolved Hide resolved
pkg/envoy/ciliumenvoyconfig.go Outdated Show resolved Hide resolved
@jrajahalme jrajahalme force-pushed the envoy-initial-support-for-internal-listeners branch from f0afacc to ff0ef24 Compare November 9, 2023 02:00
@jrajahalme
Copy link
Member Author

rebased for CI fixes

@jrajahalme
Copy link
Member Author

/test

Envoy Internal listeners do not have a real listening socket, nor is the
downstream connection backed by a socket, so socket options can not be
applied on them. Allow use of Envoy internal listeners by refraining from
injecting Cilium filters on internal listeners, so that socket options
are not applied. This also means that Cilium policy enforcement is not
performed on internal listeners, so they must be used with caution.

This should be addressed in a future commit to propagate Cilium metadata
to the internal listener from the previous, real listener.

Note that as of now Envoy Internal listeners is not a stable feature, so
Envoy emits the following warning during bootstrap:

    [misc] [external/envoy/source/common/protobuf/message_validator_impl.cc:35]
    message 'envoy.extensions.bootstrap.internal_listener.v3.InternalListener'
    is contained in proto file
    'envoy/extensions/bootstrap/internal_listener/v3/internal_listener.proto'
    marked as work-in-progress. API features marked as work-in-progress are not
    considered stable, are not covered by the threat model, are not supported
    by the security team, and are subject to breaking changes. Do not use this
    feature without understanding each of the previous points.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the envoy-initial-support-for-internal-listeners branch from ff0ef24 to bd8d274 Compare November 10, 2023 05:51
@jrajahalme
Copy link
Member Author

/test

Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

Changes look good, just some non blocking questions/suggestions.

pkg/envoy/ciliumenvoyconfig.go Show resolved Hide resolved
pkg/envoy/ciliumenvoyconfig.go Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 13, 2023
@julianwiedmann julianwiedmann merged commit 71908e7 into cilium:main Nov 13, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants