feat: add ambient mode support - #267
Conversation
554c1f8 to
9580ff9
Compare
|
Please do a proper rebase to master, I see old merged commits from other people. |
Signed-off-by: madmecodes <ayushguptadev1@gmail.com> Signed-off-by: Kimonas Sotirchos <kimonas.sotirchos@canonical.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@gmail.com>
9580ff9 to
5a878ec
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @juliusvonkohout, the PR is rebased on current |
| <<<<<<<< HEAD:components/profile-controller/manifests/kustomize/overlays/kubeflow/patches/remove-namespace.yaml | ||
| name: profiles-system | ||
| ======== | ||
| name: system | ||
| >>>>>>>> 9024371 (feat: Add ambient mode support to profile-controller):components/profile-controller/config/components/remove-system-namespace/patches/remove-namespace.yaml |
| // Create waypoint using Gateway API with waypoint gateway class | ||
| // This creates an Istio waypoint proxy that handles L7 policies in ambient mode | ||
| gatewayClassName := "istio-waypoint" |
There was a problem hiding this comment.
is this not some sort of a loop? what component implements the gateway api? is also istio?
|
Looking at the implementation, I have several architectural concerns because this couples the profile controller with the underlying infrastructure. It's not portable to assume Istio will always exist, or is that a good assumption? On another point, reusing existing infrastructure is one thing, but actively configuring its internals from the profile controller is a different story, since it breaks the separation of concerns. You are making the person responsible for deploying notebooks also accountable for configuring and modifying how the underlying mesh components work. I would avoid depending on beta APIs (gatewayv1beta1), that is going to cause upgrade problems and version skew support issues later on. IMHO the mesh configuration should be part of the infrastructure layer |
|
Please rebase to master |
There was a problem hiding this comment.
Pull request overview
Adds Istio ambient-mode support to Profile Controller while retaining sidecar mode.
Changes:
- Adds waypoint, Gateway API routing, authorization, and namespace labeling.
- Splits shared, sidecar, and ambient Istio manifests.
- Updates Go dependencies, build tooling, and tests.
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
overlays/kubeflow/kustomization.yaml |
Uses split Istio components. |
kubeflow-ambient/patches/remove-namespace.yaml |
Removes the base namespace. |
kubeflow-ambient/patches/deployment.yaml |
Adds ambient controller flags. |
kubeflow-ambient/params.env |
Defines ambient configuration. |
kubeflow-ambient/namespace-labels.yaml |
Defines Profile namespace labels. |
kubeflow-ambient/kustomization.yaml |
Assembles the ambient overlay. |
istio-sidecar/virtual-service.yaml |
Defines sidecar-mode routing. |
istio-sidecar/network-policy.yaml |
Restricts KFAM ingress. |
istio-sidecar/kustomization.yaml |
Assembles sidecar resources. |
istio-common/kustomization.yaml |
Assembles shared Istio resources. |
istio-common/authorizationpolicy.yaml |
Authorizes dashboard access. |
istio-ambient/kustomization.yaml |
Assembles ambient routing. |
istio-ambient/httproute.yaml |
Defines Gateway API routing. |
main.go |
Adds ambient configuration flags. |
go.sum |
Updates dependency checksums. |
go.mod |
Updates Go and mesh dependencies. |
Dockerfile |
Updates the builder configuration. |
profile_controller.go |
Implements ambient reconciliation. |
profile_controller_test.go |
Adds ambient-related tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Waypoint already exists, check if update is needed | ||
| if !reflect.DeepEqual(waypoint.Spec, foundWaypoint.Spec) { | ||
| logger.Info("Updating waypoint", "waypoint", waypoint.Name, "namespace", waypoint.Namespace) | ||
| foundWaypoint.Spec = waypoint.Spec | ||
| err = r.Update(context.TODO(), foundWaypoint) |
| // Create waypoint and L4 AuthorizationPolicy in ambient mode | ||
| if r.ServiceMeshMode == "istio-ambient" { |
| // Verify waypoint namespace defaults to profile namespace when empty | ||
| // This is just testing the field value, not the actual creation logic | ||
| // which would require mocking the Kubernetes client | ||
| _ = profile // Use profile to avoid unused variable error |
This PR is the clone of #185.
The PR by Kimonas was rebased (and conflicts fixed) on top of
v2.0-branch.cc @deusebio