-
Notifications
You must be signed in to change notification settings - Fork 477
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.
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
ServiceEntry support #10870
ServiceEntry support #10870
Conversation
the issue was I didn't put We must document somewhere that the status.addresses functionality must be enabled OR users must manually write addresses in their ServiceEntry spec. https://istio.io/latest/news/releases/1.23.x/announcing-1.23/#dns-auto-allocation-improvements |
internal/kgateway/extensions2/plugins/serviceentry/backendref.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/waypoint/waypoint_translator_test.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/waypoint/waypointquery/service_model.go
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/waypoint/waypointquery/service_model.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly LGTM, just a few comments/questions
Settings: settings, | ||
Namespaces: namespaces, | ||
Services: services, | ||
ServiceEntries: serviceEntries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not suggesting to change this but more for my own understanding, do we actually need ServiceEntries here as part of the CommonCollections? i.e. will anything outside of the SE plugin need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The waypoint is already reading them. The alternative is to create a "logical service" that merges over Service and ServiceEntry for waypoint to utilize.
If we don't put this here, we still end up sharing informers but run the collection logic twice for waypoint (attachment to SE) and service entry (CDS/EDS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do i need to add authorizationpolicies in collection here. My proto code works without it, but i might be missing something? Please let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authz can live in the waypointqueries
initialization until we make it apply to non-waypoints. No time to get that in for 2.0.0 release though.
internal/kgateway/extensions2/plugins/waypoint/waypoint_translator.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/waypoint/waypointquery/service_model.go
Show resolved
Hide resolved
Approved, but will defer to @lgadban /others for final approval |
spoke with @lgadban offline, can address anything else in follow ups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
API changes
networking.istio.io/Hostname
using the ServiceEntry hostname to point at a ServiceEntry in any namespace as the backend.networking.istio.io/Hostname
to attach policy a Service or ServiceEntryCode changes
ServiceEntry
to common collections as it's used across serviceentry CDS/EDS code and Waypoint translation code for attachment purposes.serviceentry
plugin that provides Backends, Endpoints and a backendRef resolverContext
#10841
Testing steps
Unit/in-memory tests:
E2E tests:
Notes for reviewers
Checklist: