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.

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

service addressed waypoint #832

Merged
merged 6 commits into from
Mar 12, 2024

Conversation

ilrudie
Copy link
Contributor

@ilrudie ilrudie commented Feb 29, 2024

implementation for sending to waypoint for service-addressed traffic

Signed-off-by: ilrudie <ian.rudie@solo.io>
Signed-off-by: ilrudie <ian.rudie@solo.io>
Signed-off-by: ilrudie <ian.rudie@solo.io>
@ilrudie ilrudie requested a review from a team as a code owner February 29, 2024 23:16
@istio-testing 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 Feb 29, 2024
@ilrudie ilrudie changed the title WIP: svc addressed waypoint WIP: service addressed waypoint Feb 29, 2024
src/proxy/outbound.rs Outdated Show resolved Hide resolved
src/proxy/outbound.rs Outdated Show resolved Hide resolved
SocketAddr::new(waypoint_ip, waypoint_us.port);
let destination_service = ServiceDescription::try_from(&*s).ok();

return Ok(Request {
Copy link
Member

Choose a reason for hiding this comment

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

can we abstract a fine grained function to reuse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean some sort of "request builder" instead of returning literals all the time?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not address in this PR if it's all the same. I opened #846 to track work on this idea.

Signed-off-by: ilrudie <ian.rudie@solo.io>
@ilrudie
Copy link
Contributor Author

ilrudie commented Mar 1, 2024

This seems far enough along to begin work on CP. If folks feel strongly we should finish this out now I'm happy to do so but otherwise I think it may make more sense to put a pin in this for a moment and begin work on istiod so both PRs can merge closer together.

Signed-off-by: ilrudie <ian.rudie@solo.io>

return Ok(Request {
protocol: Protocol::HBONE,
direction: Direction::Inbound,
Copy link
Member

Choose a reason for hiding this comment

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

isn't it outbound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably should be. I did a copy/paste to start building the request and it seems to be a hangover from a hangover.

https://github.com/istio/ztunnel/pull/486/files#r1187432943

@stevenctl
Copy link
Contributor

stevenctl commented Mar 6, 2024

How are we preventing bypass on the inbound side? Currently only Workload's waypoints are guarded on the inbound side.

This can be handled with the control plane:

  • Option 1: CP sets Workload.waypoint to the service's Waypoint for all the endpoints that don't have their own per-workload waypoint.
  • Option 2: CP creates authorization policies that make sure the Workload is only accessible from the Waypoint. We do away with any special inbound logic and let RBAC handle it.
  • Option 3: Expanded inbound check check to look at the Workload's services when the workload doesn't have it's own waypoint. We could check all it's services OR the result of guess_inbound_service.

@stevenctl
Copy link
Contributor

https://docs.google.com/document/d/1DAM_6SajCyC2hD8wBUraaZWZJqUVYPyemzw0-xpHmdY/edit?disco=AAABIl-P1-k

This thread affects the decision for the bypass stuff... which waypoint must we traverse.

@ilrudie
Copy link
Contributor Author

ilrudie commented Mar 7, 2024

Option 4 (or maybe 2.1) expose some API for the control plane to optionally create policy at the svc owner's discretion

Signed-off-by: Ian Rudie <ian.rudie@solo.io>
@ilrudie ilrudie changed the title WIP: service addressed waypoint service addressed waypoint Mar 8, 2024
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 8, 2024
@hzxuzhonghu
Copy link
Member

How can we skip sending to waypoint if a pod belongs to svc disabled waypoint according to https://docs.google.com/document/d/1DAM_6SajCyC2hD8wBUraaZWZJqUVYPyemzw0-xpHmdY/edit?disco=AAABIl-P1-k

@ilrudie
Copy link
Contributor Author

ilrudie commented Mar 11, 2024

How can we skip sending to waypoint if a pod belongs to svc disabled waypoint according to https://docs.google.com/document/d/1DAM_6SajCyC2hD8wBUraaZWZJqUVYPyemzw0-xpHmdY/edit?disco=AAABIl-P1-k

I think your question is what if a user declares they want a waypoint on a pod and declares they want #none for svc. It's an interesting case and I'm not sure the answer is clear here. Should #none on a service mean bypass all waypoints for to service traffic or just do not configure a namespace-defined waypoint?

cc @louiscryan and @stevenctl

@louiscryan
Copy link

How can we skip sending to waypoint if a pod belongs to svc disabled waypoint according to https://docs.google.com/document/d/1DAM_6SajCyC2hD8wBUraaZWZJqUVYPyemzw0-xpHmdY/edit?disco=AAABIl-P1-k

I think your question is what if a user declares they want a waypoint on a pod and declares they want #none for svc. It's an interesting case and I'm not sure the answer is clear here. Should #none on a service mean bypass all waypoints for to service traffic or just do not configure a namespace-defined waypoint?

cc @louiscryan and @stevenctl

Use-waypoint is a directive around how to route 'addressed' traffic so we should not infer anything from "service X capture #none" other than that the user wants traffic addressing that service (via VIP/host) to not transit a waypoint. The user can simultaneously say that they want traffic addressing a pod which is a member of that service to go through a waypoint. There is no conflict here, its just important to document for users the semantics of capture on the addressing mechanism.

If users want to remove ambiguity they should just capture namespaces for the most part.

@stevenctl stevenctl mentioned this pull request Mar 11, 2024
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

That is clear, a service label indicates whther to tansit traffic targeted at its clusterip. rather than targeted for its pods

@istio-testing istio-testing merged commit 8a0046f into istio:master Mar 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants