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

Assume Ingress identity also for cluster internal traffic via Ingress #24826

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Assume Ingress identity also for cluster internal traffic via Ingress #24826

merged 3 commits into from
Apr 25, 2023

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Apr 11, 2023 •
edited by aanm

Add 'useOriginalSourceAddr' parameter to ParseResources() to inform if
the listener should use original source address or not. Pass this as
'true' from C/CEC watchers if OwnerReferences does not contain
Kind=Ingress. That is, do not use original source addressing for Ingress
listeners. This makes upstream connections from Ingress listeners to use
node's allocated Ingress addresses in all cases, so that the source if
Ingress traffic within the cluster is seen as 'ReservedIdentityIngress'
(8).

Fixes: #24536

Assume Ingress identity for cluster internal traffic through Cilium Ingress for policy enforcement.

@jrajahalme jrajahalme requested review from a team as code owners April 11, 2023 23:54
@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 Apr 11, 2023
@jrajahalme jrajahalme marked this pull request as draft April 11, 2023 23:54
@jrajahalme jrajahalme changed the title Ingress id east west Assume Ingress identity also for cluster internal traffic via Ingress Apr 12, 2023
@jrajahalme jrajahalme added the area/servicemesh GH issues or PRs regarding servicemesh label Apr 12, 2023
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme added the release-note/misc This PR makes changes that have no direct user impact. label Apr 12, 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 Apr 12, 2023
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme self-assigned this Apr 14, 2023
@jrajahalme
Copy link
Member Author

Updated to new cilium/proxy build.

@jrajahalme
Copy link
Member Author

IntegrationTest failed to start due to:

gpg: Can't check signature: No public key
ERROR: Failed to verify clang+llvm-10.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz

restarted.

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

Updated to pick ownerReference change in CEC for shared Ingress.

@jrajahalme jrajahalme marked this pull request as ready for review April 18, 2023 16:38
@jrajahalme jrajahalme requested review from a team as code owners April 18, 2023 16:38
@jrajahalme jrajahalme added the dont-merge/preview-only Only for preview or testing, don't merge it. label Apr 18, 2023
@dylandreimerink
Copy link
Member

FYI. CI is failing on a test added yesterday #24835, so this branch might need to be rebased to get it to go away. (hitting the same on one of my PRs)

tklauser
tklauser approved these changes Apr 21, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser 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 for @cilium/vendor changes.

@sayboras
Copy link
Member

FYI. CI is failing on a test added yesterday #24835, so this branch might need to be rebased to get it to go away. (hitting the same on one of my PRs)

Thanks a lot, I was wondering why egress tests failed suddenly 🎖️

sayboras added a commit to sayboras/cilium-cli that referenced this pull request Apr 24, 2023
@sayboras
connectivity: Add Ingress related tests
e72270b 
This commit is to improve the Ingress coverage with below cases:

- no policy, requests should pass.
- default denied for all endpoints, request should fail.
- default denied for all endpoints, but allow Ingress identity, requests
  should pass.

Relates: cilium/cilium#24826

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras mentioned this pull request Apr 24, 2023
Merged
@jrajahalme
debug
815ad65 
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
envoy: Update to support Ingress identity for local sources
ee566c9 
Some Envoy API fields were renamed for clarity.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
envoy: pass 'useOriginalSourceAddr' parameter through ParseResources()
189c142 
Add 'useOriginalSourceAddr' parameter to ParseResources() to inform if
the listener should use original source address or not. Pass this as
'true' from C/CEC watchers if OwnerReferences does not contain
Kind=Ingress. That is, do not use original source addressing for Ingress
listeners. This makes upstream connections from Ingress listeners to use
node's allocated Ingress addresses in all cases, so that the source if
Ingress traffic within the cluster is seen as 'ReservedIdentityIngress'
(8).

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

Rebased to pick up an egress gw fix, this should be good to review/merge given that @sayboras added an integration test for this in cilium/cilium-cli#1533.

@jrajahalme
Copy link
Member Author

/test

@aanm aanm added release-note/major This PR introduces major new functionality to Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Apr 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Apr 24, 2023
sayboras
sayboras approved these changes Apr 24, 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, one small comment as per below.

pkg/k8s/watchers/cilium_envoy_config.go
Comment on lines +90 to +93
if owner.Kind == "Ingress" {
return true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

the same might need to be done for GatewayAPI resources (e.g. Gateway)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that on a separate PR as that would not need to be backported to 1.13, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrajahalme pinging you directly for exposure in #28254. it seems this hasn't been backported for gateway api?

@jrajahalme jrajahalme merged commit 01a0367 into cilium:main Apr 25, 2023
56 checks passed
@sayboras sayboras mentioned this pull request Apr 25, 2023
Closed
@michi-covalent
Copy link
Contributor

why doesn't this pull request have backport-pending/1.13 label? there is #25019 open for backporting this to v1.13 branch 👀

@jrajahalme jrajahalme 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 Apr 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 Apr 25, 2023
@jrajahalme
Copy link
Member Author

why doesn't this pull request have backport-pending/1.13 label? there is #25019 open for backporting this to v1.13 branch 👀

Fixed that, sorry. This was an author-backport and so far for me that has been a manual process. Forgot to flip the label, that's all.

@michi-covalent michi-covalent 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 Apr 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.3 Apr 25, 2023
michi-covalent pushed a commit to cilium/cilium-cli that referenced this pull request Apr 26, 2023
@sayboras @michi-covalent
connectivity: Add Ingress related tests
c11d062 
This commit is to improve the Ingress coverage with below cases:

- no policy, requests should pass.
- default denied for all endpoints, request should fail.
- default denied for all endpoints, but allow Ingress identity, requests
  should pass.

Relates: cilium/cilium#24826

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@thorn3r thorn3r mentioned this pull request May 17, 2023
Merged
michi-covalent pushed a commit to michi-covalent/cilium that referenced this pull request May 30, 2023
@sayboras @michi-covalent
connectivity: Add Ingress related tests
4c457e9 
This commit is to improve the Ingress coverage with below cases:

- no policy, requests should pass.
- default denied for all endpoints, request should fail.
- default denied for all endpoints, but allow Ingress identity, requests
  should pass.

Relates: cilium#24826

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@rauanmayemir rauanmayemir mentioned this pull request Sep 23, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rauanmayemir rauanmayemir rauanmayemir left review comments

@tklauser tklauser tklauser approved these changes

@christarazi christarazi christarazi approved these changes

@sayboras sayboras sayboras approved these changes

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles tommyp1ckles is a code owner automatically assigned from cilium/sig-k8s

Assignees

@jrajahalme jrajahalme

Labels
area/servicemesh GH issues or PRs regarding servicemesh backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/major This PR introduces major new functionality to Cilium.
Projects
No open projects
1.13.3
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Fix Ingress traffic interactions with other Policy enforcement
8 participants
@jrajahalme @sayboras @dylandreimerink @michi-covalent @rauanmayemir @tklauser @christarazi @aanm