ambient: support explicit EndpointSlice endpoints #51591

howardjohn · 2024-06-14T22:48:37Z

This adds support for reading EndpointSlice addresses in ambient. This works by getting all non-pod addresses and sending them over.

The #1 use case is the kubernetes.default.svc service. This has a test case, and it was intentionally broken in istio/ztunnel@636c69c. That is why this PR does not have any new e2e tests; they already exist and are broken with ztunnel from HEAD (ztunnel has not update as a result).

Fixes #51539

istio-testing · 2024-06-14T22:48:39Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

This is for direct endpoint-slice objects. Note this is required after istio/ztunnel#1137 otherwise kubernetes.default.svc is broken. However, this is useful beyond that for correctness.

istio-policy-bot · 2024-06-17T19:27:13Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex.go

costinm

My main concern is the security impact - left few cosmetic comments as well.

With pods - K8S has some restrictions on the CIDR range and manages the allocation (there are some issues there as well probably).

What is the impact of a malicious user creating a ServiceEntry with an IP of an external service or non-local K8S Service ( from other clsuters ) or even a cluster local service ?

Would be great if the code had more comments - in particular around sensitive areas like possible abuse and why it's safe against that.

Do we want to have some extra protections - like allowing user to turn off watching arbitrary SE, or restricting the IP range ?

pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex.go

costinm · 2024-06-17T20:28:32Z

pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

+	WorkloadServices krt.Collection[model.ServiceInfo],
+) krt.TransformationMulti[*discovery.EndpointSlice, model.WorkloadInfo] {
+	return func(ctx krt.HandlerContext, es *discovery.EndpointSlice) []model.WorkloadInfo {
+		// We only care about EndpointSlices that are for a Service.


Why ? I think ES can be used without Service to represent endpoints that may not have a Service associated ( but something else ?). Don't we want to apply the same rules as we do for pods ?

You can represent them, but there is no way for Istio to know what to do with them. They are just data?

So from the perspective of 'workload to workload' communication - we treat them as plain text ?
Instead of for example checking if they have a HBONE port, generating some rule to check spiffe://.../ns/namespace/sa/*, etc ?

That's related to my security question on how we control malicious IPs - but on the opposite side, assuming ES is strictly locked with RBAC/OPA - and user is using them to represent non-Pod workloads that may or may not use Service but are clearly getting associated with a namespace and have port info ( which in turn may allow inferring if they use HBONE).

Also: not sure I fully understand all ambient code, but will this only add endpoints to Service - or will it have any impact on the workload-to-workload communication ?

What does it even mean to have an EndpointSlice read by a proxy that does not have a service associated? There is no other proxies doing this (kube-proxy, istio sidecars, etc).

it doesn't have defined semantics.

Well - kube-proxy implement Service so I assume watches for ES associated with Service.

I don't know exactly if istio-sidecars don't 'accidentally' do some label selections or have other means to take EndpointSlices not associated with a Service but with a ServiceEntry or WorkloadGroup or something similar - after all we did in the past use selector-less Services and Endpoints to represent VMs.

And for 'others' - and semantics - I don't know. It looks like K8S went pretty far in making ES not require a Service - they could have easily added a field and make it required/validated. The semantics are 'a bag of IPs with labels and port info' - just like Gateway API can attach to many things except Service, no clue of ES is not associated with something else, like a ServiceImport or some VM-like resource.

Not saying I know a specific use - just that we should at least comment the code and make it clear that ES for other things may exist and we choose to ignore them - and perhaps warn if we find them.

Similar comment for the other things we silently ignore - IPs that are not valid ( hostnames - I have not seen the deprecation announce and what will happen with them), etc.

Compared with WorkloadEntry - SE provides about the same capabilities but with a more standard interface and some interoperability. Istio is pushing the boundaries with ServiceEntry and WE - we can't expect nobody else does.

That doesn't mean I'm against this change - just trying to understand the risks.

selector-less Services and Endpoints to represent VMs.

Key word there is Service... these are ones associated with a service.

In theory we could do something with the arbitrary ones, but I have no clue what. They do not have useful information to us -- no service account, etc.

costinm · 2024-06-17T20:31:16Z

pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

+				health = workloadapi.WorkloadStatus_HEALTHY
+			}
+			addresses, err := slices.MapErr(ep.Addresses, func(e string) ([]byte, error) {
+				n, err := netip.ParseAddr(e)


Does ES allows anything else ( UDS, etc ) ? The err seems to drop all endpoints even if just one fails, not sure that's what we want ?

No, it does not. They must be IP addresses.

AFAIK FQDN is also allowed.

Ah good point. It is deprecated now I think though. Ill make sure to more explicitly filter

The issue I commented on is the handling - if I read the code correctly, one bad entry will cause the entire set to be ignored. Maybe I missread.

Error or some varz may be better than silently ignoring.

costinm · 2024-06-18T00:29:47Z

They do have useful info - port name/number ( so hbone can be inferred) and namespace - which is pretty much the main thing you can rely on. In any case - more worried about what arbitrary IPs one can set, which is a broad problem but in ambient it may be more sensitive since it does workload auth

…

On Mon, Jun 17, 2024, 17:23 John Howard ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go <#51591 (comment)>: > @@ -335,6 +349,113 @@ func (a *index) serviceEntryWorkloadBuilder( } } +func (a *index) endpointSlicesBuilder( + MeshConfig krt.Singleton[MeshConfig], + WorkloadServices krt.Collection[model.ServiceInfo], +) krt.TransformationMulti[*discovery.EndpointSlice, model.WorkloadInfo] { + return func(ctx krt.HandlerContext, es *discovery.EndpointSlice) []model.WorkloadInfo { + // We only care about EndpointSlices that are for a Service. selector-less *Services* and Endpoints to represent VMs. Key word there is Service... these are ones associated with a service. In theory we could do something with the arbitrary ones, but I have no clue what. They do not have useful information to us -- no service account, etc. — Reply to this email directly, view it on GitHub <#51591 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2QVBQM4Q26YFYDFBELZH54X5AVCNFSM6AAAAABJLDPKDOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRUGEYTGOJRGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

howardjohn · 2024-06-18T14:38:22Z

added more comments

howardjohn · 2024-06-19T00:02:04Z

blocking #51592

stevenctl · 2024-06-24T18:19:52Z

pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

+			// no service found
+			return nil
+		}
+		// There SHOULD only be one. This is only Service which has unique hostnames.


nit: debug level warn when there are more than 1

stevenctl · 2024-06-24T18:27:26Z

Could be nice to have a really basic unit test for the kubernetes.default.svc case

istio-testing · 2024-06-24T19:06:11Z

In response to a cherrypick label: #51591 failed to apply on top of branch "release-1.22":

Applying: ambient: support reading workloads from EndpointSlice
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 ambient: support reading workloads from EndpointSlice
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2024-06-24T19:06:12Z

In response to a cherrypick label: new issue created for failed cherrypick: #51690

* ambient: support reading workloads from EndpointSlice This is for direct endpoint-slice objects. Note this is required after istio/ztunnel#1137 otherwise kubernetes.default.svc is broken. However, this is useful beyond that for correctness. * add comments (cherry picked from commit 99d3d4c)

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Jun 14, 2024

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 14, 2024

ambient: support reading workloads from EndpointSlice

06e60c7

This is for direct endpoint-slice objects. Note this is required after istio/ztunnel#1137 otherwise kubernetes.default.svc is broken. However, this is useful beyond that for correctness.

howardjohn changed the title ~~ambient/endpoint slices~~ ambient: support explicit EndpointSlice endpoints Jun 17, 2024

howardjohn marked this pull request as ready for review June 17, 2024 19:26

howardjohn requested a review from a team as a code owner June 17, 2024 19:26

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 17, 2024

costinm reviewed Jun 17, 2024

View reviewed changes

pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex.go Show resolved Hide resolved

costinm reviewed Jun 17, 2024

View reviewed changes

add comments

57f86af

howardjohn force-pushed the ambient/endpoint-slices branch from 6daafe0 to 57f86af Compare June 18, 2024 14:38

howardjohn requested a review from costinm June 18, 2024 16:13

howardjohn added the cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch label Jun 19, 2024

howardjohn assigned stevenctl Jun 24, 2024

stevenctl reviewed Jun 24, 2024

View reviewed changes

stevenctl approved these changes Jun 24, 2024

View reviewed changes

istio-testing merged commit 99d3d4c into istio:master Jun 24, 2024
28 checks passed

istio-testing mentioned this pull request Jun 24, 2024

[release-1.22] ambient: support explicit EndpointSlice endpoints #51690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ambient: support explicit EndpointSlice endpoints #51591

ambient: support explicit EndpointSlice endpoints #51591

howardjohn commented Jun 14, 2024 •

edited

Loading

istio-testing commented Jun 14, 2024

istio-policy-bot commented Jun 17, 2024

costinm left a comment

costinm Jun 17, 2024

howardjohn Jun 17, 2024

costinm Jun 17, 2024

costinm Jun 17, 2024

howardjohn Jun 17, 2024

costinm Jun 17, 2024

howardjohn Jun 18, 2024

costinm Jun 17, 2024

howardjohn Jun 17, 2024

costinm Jun 17, 2024

howardjohn Jun 17, 2024

costinm Jun 17, 2024

costinm commented Jun 18, 2024 via email

howardjohn commented Jun 18, 2024

howardjohn commented Jun 19, 2024

stevenctl Jun 24, 2024

stevenctl commented Jun 24, 2024

istio-testing commented Jun 24, 2024

istio-testing commented Jun 24, 2024

ambient: support explicit EndpointSlice endpoints #51591

ambient: support explicit EndpointSlice endpoints #51591

Conversation

howardjohn commented Jun 14, 2024 • edited Loading

istio-testing commented Jun 14, 2024

istio-policy-bot commented Jun 17, 2024

costinm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costinm commented Jun 18, 2024 via email

howardjohn commented Jun 18, 2024

howardjohn commented Jun 19, 2024

Choose a reason for hiding this comment

stevenctl commented Jun 24, 2024

istio-testing commented Jun 24, 2024

istio-testing commented Jun 24, 2024

howardjohn commented Jun 14, 2024 •

edited

Loading