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.

Sign up for GitHub

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

Jump to bottom

proxy: introduce initial proxy cell #25779

Merged
merged 7 commits into from Jun 15, 2023
Merged

proxy: introduce initial proxy cell #25779

merged 7 commits into from Jun 15, 2023

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented May 31, 2023 •
edited

This PR introduces an initial version of a proxy hive cell which provides the (L7-)Proxy.

The daemon itself still depends on the Proxy and keeps it as member for the time being (which is necessary to pass it to non-modularized components.

The main changes are the following ones

  • Remove explicit initialization of proxy in the daemon
  • Starting the envoy xDS- and accesslog server became part of a lifecycle hook
  • Refactor / Move endpoint related interfaces to prevent cyclic dependencies
  • Remove global variables IdentityAllocator & EndpointManager
  • Proper teardown of xDS & accesslog server

Further changes are meant to be refactored in separate upcoming PRs to keep the scope of this PR as small as possible.

  • introduce smaller inner cells (e.g. for envoy specific parts (xds server))
  • extract interface (at best there are multiple ones) - and provide this instead of the struct
  • dedicated config struct
  • ...

@mhofstetter mhofstetter added kind/enhancement This would improve or streamline existing functionality. kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. area/modularization labels May 31, 2023
@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

mhofstetter commented May 31, 2023 •
edited

/test -> 🟢

@mhofstetter mhofstetter added the area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. label May 31, 2023
@mhofstetter mhofstetter marked this pull request as ready for review May 31, 2023 10:10
@mhofstetter mhofstetter requested review from a team as code owners May 31, 2023 10:10
bimmlerd
bimmlerd approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

reviewed just contributing changes, lgtm

(btw I almost broke the benchmarks in proxy/logger_test.go when I last did something to code in that area, they don't get built by default 😅)

@mhofstetter mhofstetter requested a review from joamaki June 5, 2023 06:10
@mhofstetter
Copy link
Member Author

rebased to main to resolve conflicts

joamaki
joamaki approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for starting to tackle this!

daemon/cmd/cells.go Outdated
@@ -112,6 +113,9 @@ var (
// daemonCell wraps the legacy daemon initialization and provides Promise[*Daemon].
daemonCell,

// Proxy provides the L7 proxy functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I wonder if there's a more exact way of explaining what pkg/proxy does? From this comment you'll get the impression that pkg/proxy does the L7 proxying when in fact it just talks to Envoy. E.g. say here it synchronizes redirected L7 backends to envoy or something along those lines so it's more obvious where this piece fits in and what it ingests.

The way I'm thinking about the one-liner comments in this file is how someone new to this codebase pieces together how everything fits together, so I'd try to find a succinct way of describing what each thing does and how it relates to others. Here for example it'd be quite useful to mention that this cell processes the CiliumEnvoyConfig CRD.

Copy link
Member

Choose a reason for hiding this comment

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

How about this:

Proxy provides the proxy port allocation and related datapath coordination and makes different L7 proxies (Envoy, DNS proxy) usable to CIlium endpoints through a common Proxy 'redirect' abstraction.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrajahalme thanks for the proposal. it provides way more context and more or less covers all the aspects of the package.

sayboras
sayboras approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

@mhofstetter mhofstetter removed the request for review from ti-mo June 6, 2023 07:57
@mhofstetter
Copy link
Member Author

removed review-request for @ti-mo - sig-foundation (modularization aspect) already covered by @joamaki

jrajahalme
jrajahalme requested changes Jun 12, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Original file errors should be passed on rather than simply dropped.

daemon/cmd/cells.go Outdated
@@ -112,6 +113,9 @@ var (
// daemonCell wraps the legacy daemon initialization and provides Promise[*Daemon].
daemonCell,

// Proxy provides the L7 proxy functionality
Copy link
Member

Choose a reason for hiding this comment

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

How about this:

Proxy provides the proxy port allocation and related datapath coordination and makes different L7 proxies (Envoy, DNS proxy) usable to CIlium endpoints through a common Proxy 'redirect' abstraction.

pkg/envoy/accesslog_server.go Outdated Show resolved Hide resolved
pkg/envoy/accesslog_server.go Outdated Show resolved Hide resolved
pkg/envoy/server.go Outdated Show resolved Hide resolved
pkg/envoy/server.go Outdated Show resolved Hide resolved
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/proxy-cell branch 2 times, most recently from 2fab210 to 4047b55 Compare June 12, 2023 15:28
@mhofstetter
Copy link
Member Author

Original file errors should be passed on rather than simply dropped.

@jrajahalme thanks for your review.

fixed the error propagation and used your proxy cell description!

@mhofstetter
Copy link
Member Author

rebased to main and resolved conflicts with #25969

no other changes

@mhofstetter
Copy link
Member Author

/test

jrajahalme
jrajahalme approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@jrajahalme jrajahalme added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 14, 2023
@mhofstetter
proxy: introduce initial cell
a07d21c 
This commit introduces the proxy hive cell which provides the Proxy.

Starting the envoy xDS- and accesslog server became part of a
lifecycle hook.

The daemon depends on the Proxy and keeps it as member for the time
being.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
proxy: move endpoint interfaces to proxy/endpoint
7dd7443 
This commit moves the endpoint interfaces `EndpointInfoSource`
& `EndpointUpdater` from `pkg/proxy/logger` to `pkg/proxy/endpoint`
because they aren't related with logging. This is necessary to break
some cyclic dependencies.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
proxy: remove global variables IdentityAllocator & EndpointManager
14d1c46 
This commit removes the global variables IdentityAllocator &
EndpointManger from `pkg/proxy/epinfo.go`.

Now, they are properly kept as fields.

To break cyclic dependencies, the endpointinfo has been moved to
`pkg/proxy/logger/endpoint/epinfo.go` and is provided via private
constructor.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
proxy: introducing MonitorAgentLogRecordNotifier
3547125 
This commit introduces the `MonitorAgentLogRecordNotifier` as replacement
for letting the Daemon directly implement `LogRecordNotifier`.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
proxy: stopping xds- & accesslog server in proxy cell LC hook
aaee23a 
This commit introduces proper shutdown of the envoy xDS- and accesslog
servers via lifecycle hooks in the proxy cell.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
proxy: errorhandling when starting xds- and accesslog server
7226410 
This commit introduces proper errorhandling (with propagation) when
starting the envoy xDS & accesslog servers.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
codeowners: remove deleted daemon/cmd/proxy.go
8b6c83b 
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Copy link
Member Author

rebased to main to fetch some fixes for ci-ginkgo

@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

mhofstetter commented Jun 14, 2023 •
edited

/test-runtime

previous run flaked with #25939

@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 Jun 14, 2023
@tklauser tklauser merged commit d1de743 into cilium:main Jun 15, 2023
65 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/proxy-cell branch June 15, 2023 07:05
This was referenced Jul 4, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/modularization area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. kind/cleanup This includes no functional changes. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mhofstetter @joamaki @jrajahalme @bimmlerd @sayboras @tklauser