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

envoy: Use embedded proxylib from cilium-proxy image #26101

Merged
merged 4 commits into from Jun 26, 2023
Merged

envoy: Use embedded proxylib from cilium-proxy image #26101

merged 4 commits into from Jun 26, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Jun 12, 2023 •
edited

Description

The goal is to address one known limitation of the dedicated envoy proxy mode. Please refer to individual commit for more details.

Testing

Testing was done with the existing conformance-ginkgo job, the kafka-related policy tests are passed as per below:

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 12, 2023
@sayboras
Copy link
Member Author

/test

@sayboras sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 18, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 18, 2023
@sayboras
Copy link
Member Author

/test

@sayboras sayboras force-pushed the tam/proxylib branch 4 times, most recently from e5603f6 to d789db3 Compare June 18, 2023 12:02
@sayboras
Copy link
Member Author

/test

@sayboras sayboras added the dont-merge/preview-only Only for preview or testing, don't merge it. label Jun 18, 2023
@sayboras sayboras mentioned this pull request Jun 18, 2023
Merged
@sayboras
Copy link
Member Author

/test

@mhofstetter
Copy link
Member

@sayboras as mentioned, it's possible to remove the currently documented limitation in the docs https://github.com/cilium/cilium/blob/main/Documentation/security/network/proxy/envoy.rst?plain=1#L61 🎉

@jrajahalme
Copy link
Member

Now that cilium/proxy#232 has merged we can update this to newest main references :-)

@sayboras
Copy link
Member Author

Now that cilium/proxy#232 has merged we can update this to newest main references :-)

Thanks, let me make an update shortly 👍

@sayboras sayboras added area/servicemesh GH issues or PRs regarding servicemesh kind/bug This is a bug in the Cilium logic. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. and removed dont-merge/preview-only Only for preview or testing, don't merge it. labels Jun 23, 2023
@sayboras sayboras force-pushed the tam/proxylib branch 3 times, most recently from 6ba3676 to 2de923d Compare June 23, 2023 10:16
@sayboras
Copy link
Member Author

/test

@sayboras sayboras changed the title envoy: Use embedded proxylib envoy: Use embedded proxylib from cilium-proxy image Jun 23, 2023
@sayboras sayboras marked this pull request as ready for review June 23, 2023 10:57
@sayboras sayboras requested review from a team as code owners June 23, 2023 10:57
learnitall
learnitall approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

LGTM on docs!

joestringer
joestringer approved these changes Jun 25, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Minor nits, seems like a straightforward migration to me.

CODEOWNERS Outdated Show resolved Hide resolved
proxylib/Makefile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we still have proxylib documentation that needs to be updated for this change? If so, please fix that up as well. Doesn't need to block this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

noted and thanks, I will send the follow-up PR on docs 👍

@sayboras
envoy: Use embedded proxylib from cilium-proxy container
e9a72fb 
The same proxylib library is now available in cilium/proxy container.
The image is built from https://github.com/cilium/proxy/actions/runs/5354917649/jobs/9712555724.

Relates: cilium/proxy#232
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
Copy link
Member Author

/test

@sayboras
proxylib: Remove proxylib module
e1ad277 
This module is moved to cilium/proxy as part of the below PR, the main
reason is to make sure that cilium/proxy container image is fully
self-contained, and has no dependency with cilium/cilium.

cilium/proxy#232
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
pkg: Remove pkg/policy/api/kafka module
736fd4a 
This module can be imported directly from cilium/proxy now

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
docs: Remove go extension limitation in dedicated mode
f324251 
The mentioned limitation is no longer true, as the dedicated proxy image
is shipped with proxylib for Go Extension support now.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
Copy link
Member Author

/test

gandro
gandro approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@gandro gandro 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 from my code owners

mhofstetter
mhofstetter approved these changes Jun 26, 2023
View reviewed changes
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 - thanks Tam (also for removing the limitation)!

@youngnick youngnick removed the request for review from jrajahalme June 26, 2023 12:38
nathanjsweet
nathanjsweet approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

LGTM.

@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 26, 2023
@nathanjsweet nathanjsweet merged commit 9a14440 into cilium:main Jun 26, 2023
65 checks passed
@sayboras sayboras deleted the tam/proxylib branch June 26, 2023 23:26
@sayboras sayboras mentioned this pull request Jun 27, 2023
Merged
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
sayboras added a commit to sayboras/cilium that referenced this pull request Jun 30, 2023
@sayboras
docs: Update Go Extension docs
5f72fb5 
This is to make sure that related docs is pointing to cilium/proxy repo.

Relates: cilium#26101
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@mhofstetter mhofstetter mentioned this pull request Jul 10, 2023
Closed
julianwiedmann pushed a commit that referenced this pull request Jul 10, 2023
@sayboras @julianwiedmann
docs: Update Go Extension docs
3b979b7 
This is to make sure that related docs is pointing to cilium/proxy repo.

Relates: #26101
Signed-off-by: Tam Mach <tam.mach@cilium.io>
jibi pushed a commit that referenced this pull request Jul 13, 2023
@sayboras @jibi
docs: Update Go Extension docs
051ad04 
[ upstream commit 3b979b7 ]

This is to make sure that related docs is pointing to cilium/proxy repo.

Relates: #26101
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
aanm pushed a commit that referenced this pull request Jul 14, 2023
@sayboras @aanm
docs: Update Go Extension docs
8becfb5 
[ upstream commit 3b979b7 ]

This is to make sure that related docs is pointing to cilium/proxy repo.

Relates: #26101
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@sayboras sayboras mentioned this pull request Mar 19, 2024
Merged
4 tasks
@sayboras sayboras added the backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@joestringer joestringer joestringer approved these changes

@rolinh rolinh rolinh approved these changes

@mhofstetter mhofstetter mhofstetter approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@youngnick youngnick youngnick approved these changes

@learnitall learnitall learnitall approved these changes

Assignees
No one assigned
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@sayboras @mhofstetter @jrajahalme @gandro @nathanjsweet @joestringer @rolinh @bimmlerd @youngnick @learnitall