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

Add policy listener support #21600

Merged
merged 11 commits into from
Dec 13, 2022
Merged

Add policy listener support #21600

merged 11 commits into from
Dec 13, 2022

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Oct 6, 2022 •
edited

Add support for listener keyword in CNP to redirect matching traffic to an Envoy listener.

Please review commit-by-commit.

Traffic can now we redirected to Envoy listeners via Cilium Network Policy `listener` option.

@jrajahalme jrajahalme added area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/servicemesh GH issues or PRs regarding servicemesh labels Oct 6, 2022
@jrajahalme jrajahalme requested review from a team as code owners October 6, 2022 07:25
@jrajahalme jrajahalme requested a review from rolinh October 6, 2022 07:25
@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 Oct 6, 2022
@jrajahalme jrajahalme marked this pull request as draft October 6, 2022 07:27
@jrajahalme
proxy: Do not create Envoy redirects for listeners defined in CEC CRDs
1e47031 
Add a new no-op CRDRedirect type to be used with Envoy listeners defined
in CEC CRDs. In this case the listeners already exist and new Envoy
Listener resources do not need to be created for them.

This is needed for the forthcoming policy feature where policy can refer
to a Listener defined in CEC CRD.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
proxy: Find CRD proxy ports by name instead of by type
391b544 
Find CRD proxy port by name instead of type. This is needed for enabling
CEC CRD defined listeners to be used in CNPs. Prior to this CRD proxy
ports did not use this code path, which is only called from endpoint
policy updates, so there was no need to find CRD proxy ports by name.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
envoy: Move resourceQualifiedName() to policy/api
f8c10a0 
Move resourceQualifiedName() to policy/api and export it so that it can
be used in policy as well as in envoy package.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
policy: Add CRD Listener support for egress policies
55d477d 
Add `listener` field to CNP and CCNP, that causes traffic to the
specified port(s) to be redirected to the named Envoy listener. If the
listener does not exist the traffic is not allowed at all.

When `listener.envoyConfig.kind` is left out it defaults to namespaced
`CiliumEnvoyConfig` for rules in namespaced policies (CNP) or to
cluster-scoped `CiliumClusterwideEnvoyConfig` for rules in cluster-scoped
policies (CCNP). Namespaced policies can also refer to cluster-scoped
listeners with an explicit `listener.envoyConfig.kind:
CiliumClusterwideEnvoyConfig`. Cluster-scoped policies can not refer to
namespaced listeners.

Endpoint policies are regenerated whenever Envoy listeners change to
update potential listener redirections in the bpf policy maps.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme requested a review from a team as a code owner December 13, 2022 10:43
@jrajahalme
Copy link
Member Author

@christarazi Addressed your comments and rebased :-)

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

CLI review is not needed as it is affected only via cilium policy import, marking this as ready-to-merge.

@jrajahalme jrajahalme added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 13, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.0-rc4 Dec 13, 2022
@pchaigno pchaigno merged commit 9159609 into cilium:master Dec 13, 2022
Agent Project Tracking automation moved this from High priority features to Done Dec 13, 2022
@joestringer joestringer mentioned this pull request Dec 21, 2022
Merged
@joestringer joestringer added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Dec 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.13 in 1.13.0-rc4 Dec 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.13 in 1.13.0-rc4 Dec 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Dec 21, 2022
@joestringer joestringer added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Dec 22, 2022
@joestringer joestringer moved this from Backport pending to v1.13 to Backport done to v1.10 in 1.13.0-rc4 Dec 22, 2022
@joestringer joestringer mentioned this pull request Dec 22, 2022
Merged
nbusseneau pushed a commit to nbusseneau/cilium that referenced this pull request Jun 22, 2023
@jrajahalme @nbusseneau
proxy: Do not panic on local error
d5c7d29 
[ upstream commit 525007f ]

[ backporter's notes: conflicts in proxy_test.go around the new test
  case, I decided not to include the test case because it depends on
  changes introduced in cilium#21600, which was not backported to v1.12. If
  the test case is important enough, we should remove this from the
  current round of backport and do another round with the dependent PR
  included. ]

CreateOrUpdateRedirect called nil revertFunc when any local error was
returned. This was done using the pattern `return 0, err, nil, nil` which
sets the revertFunc return variable as nil, but this was called on a
deferred function to revert any changes on a local error.

Fix this by calling ReverStack.Revert() directly on the deferred
function, and setting the return variable if there was no local error.

This was hit any time a CiliumNetworkPolicy referred to a non-existing
listener.

Add a test case that reproduced the panic and works after the fix.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
borkmann pushed a commit that referenced this pull request Jun 29, 2023
@jrajahalme @borkmann
proxy: Do not panic on local error
6c0f631 
[ upstream commit 525007f ]

[ backporter's notes: conflicts in proxy_test.go around the new test
  case, I decided not to include the test case because it depends on
  changes introduced in #21600, which was not backported to v1.12. If
  the test case is important enough, we should remove this from the
  current round of backport and do another round with the dependent PR
  included. ]

CreateOrUpdateRedirect called nil revertFunc when any local error was
returned. This was done using the pattern `return 0, err, nil, nil` which
sets the revertFunc return variable as nil, but this was called on a
deferred function to revert any changes on a local error.

Fix this by calling ReverStack.Revert() directly on the deferred
function, and setting the return variable if there was no local error.

This was hit any time a CiliumNetworkPolicy referred to a non-existing
listener.

Add a test case that reproduced the panic and works after the fix.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chancez chancez chancez approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

Assignees

@jrajahalme jrajahalme

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. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
1.13.0-rc4
Backport done to v1.13
Agent Project Tracking
Done
Milestone
1.13
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jrajahalme @chancez @christarazi @aanm @joestringer @pchaigno