proxy: introduce envoy cell #26657
proxy: introduce envoy cell #26657
Conversation
mhofstetter
added
kind/cleanup
mhofstetter
changed the title
|
/test |
mhofstetter
force-pushed
the
pr/mhofstetter/proxy-envoy-cell
branch
from
bfbe648
to
6eb03f5
Compare
|
|
||
| if !option.Config.EnableL7Proxy { | ||
| log.Debug("L7 proxies are disabled - don't start Envoy xDS server") | ||
| return xdsServer, nil |
There was a problem hiding this comment.
What's the behavior of the public API methods of XDSServer when it hasn't been started?
Also could we hide EnableL7Proxy option inside this package (or pkg/proxy) and refactor the users of this API to not care whether or not it's been enabled? E.g. by either always calling a no-op method or adding IsEnabled or similar if there's expensive work needed prior to a call. I think it'd be preferably to reduce reliance on checking "option.Config.EnableFoo" and instead make APIs that are safe to use regardless of whether they're enabled or not.
There was a problem hiding this comment.
What's the behavior of the public API methods of XDSServer when it hasn't been started?
it's just updating it's internal state without actually exposing the config via gRPC. (and for the time being it's access is "secured" by the feature flag anyway)
Also could we hide EnableL7Proxy option inside this package (or pkg/proxy) and refactor the users of this API to not care whether or not it's been enabled? E.g. by either always calling a no-op method or adding IsEnabled or similar if there's expensive work needed prior to a call. I think it'd be preferably to reduce reliance on checking "option.Config.EnableFoo" and instead make APIs that are safe to use regardless of whether they're enabled or not.
I'd like to refactor this in a follow up PR too. I already pointed this out here #26627 (comment). But this needs to be handled in the proxy cell - which should return a noop-proxy in case of L7 proxy functionality is disabled.
|
|
||
| s.stopFunc = startXDSGRPCServer(socketListener, resourceConfig, 5*time.Second) |
There was a problem hiding this comment.
I know it's the old code, but if it's not too much trouble, lift the 5*time.Second into a constant and document what it's about. Also wondering how resilient this is.. what happens if the timeout is hit? How do we recover from it?
There was a problem hiding this comment.
good hint - which should lead to deletion of some code - because eventually this duration is used to create a context with timeout... which isn't used at all :)
will open a separate PR if this is ok for you
There was a problem hiding this comment.
filed the PR: #26747
mhofstetter
force-pushed
the
pr/mhofstetter/proxy-envoy-cell
branch
from
6eb03f5
to
6ad3b0b
Compare
|
@joamaki thanks for the feedback and questions. i answered them inline. most will be handled in planned follow ups. |
This commit renames the file `pkg/envoy/server.go` to `pkg/envoy/xds_server.go`. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit moves the Envoy components xDS- and accesslog-server from the common proxy cell to a new dedicated Envoy proxy cell. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit extracts a dedicated XdsServer interface as a preparation for separating the xDS server from the Proxy struct. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit extracts functions within the xds- and accesslog server to create the socketlisteners (xds & accesslog) and the xds config (xds). Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter
force-pushed
the
pr/mhofstetter/proxy-envoy-cell
branch
from
6ad3b0b
to
315a514
Compare
|
rebased to |
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
This PR introduces a dedicated hive cell for the Envoy proxy and its control plane.
xDS- and accesslog server are moved from the proxy cell to the new Envoy proxy cell.
In addition, an interface has been extracted for the xds server. This will be used in upcoming refactorings to decouple the proxy from the xds server.
Follow up of #25779