-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add ambient mode support to profile-controller #127
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
base: main
Are you sure you want to change the base?
feat: Add ambient mode support to profile-controller #127
Conversation
Signed-off-by: madmecodes <[email protected]>
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Thank you for the PR @madmecodes . The changes look promising. There are some things missing though.
For visibility, it 'd be useful to include in the PR, somewhere before the summary of changes, the features (from below) plus the goal of the PR, explaining why we need it (make profile-controller compatible with Istio ambient's mode that requires Gateway API compatibility). Let's reiterate what the profile-controller needs to do in order to achieve this.
In Istio ambient, L7 AuthoizationPolicies are attached to waypoint, thus a waypoint is needed to secure the namespace. So when profile-controller works in ambient mode, it needs to:
- Accept arguments about
waypoint,waypoint-namespaceandcreate-waypoint(instead of the gateway ones) - Create the (existing) L7 AuthorizationPolicy as it already does, but attach it to the waypoint provided.
- Create a second L4 AuthorizationPolicy to allow traffic from the waypoint's principal to all services in the profile namespace.
- label profile namespace with appropriate labels.
istio.io/use-waypoint=<waypoint-name> istio.io/use-waypoint-namespace=<waypoint-namespace> istio.io/ingress-use-waypoint=true - if the create-waypoint is set to true, create a waypoint in the profile namespace.
| - "-service-mesh-mode" | ||
| - $(SERVICE_MESH_MODE) | ||
| - "-gateway-name" | ||
| - $(GATEWAY_NAME) | ||
| - "-gateway-namespace" | ||
| - $(GATEWAY_NAMESPACE) |
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.
In the code, the gateway and gateway-namespace are not used at all. The HTTPRoute is applied during deployment (instead of a VirtualService) and not by the profile-controller binary AFAIK.
The AuthorizationPolicy that the profile-controller already creates is what needs to be changed to secure profile namespaces in Istio ambient mode. To achieve this, it needs to be attached to a waypoint, since this is an L7 AuthorizationPolicy (see this example).
This is done using a targetRef instead of a selector. The targetRef in that case is indeed of kind: Gateway but the gateway has a gateway-class: waypoint. Thus, using the waypoint naming in the code is more clear, since it actually targets a different resource than the HTTPRoute.
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.
I 'm not too familiar with overlays but I think we should be able split the kubeflow to two flavours in order to avoid manifests duplication and only change what's needed in sidecar and ambient.
components/profile-controller/config/overlays/kubeflow-ambient/patches/kfam.yaml
Show resolved
Hide resolved
| env: | ||
| - name: SERVICE_MESH_MODE | ||
| value: ambient | ||
| - name: GATEWAY_NAME | ||
| value: kubeflow-gateway | ||
| - name: GATEWAY_NAMESPACE |
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.
Other environment variables are being fetched from the generated configmap which is generated here https://github.com/madmecodes/dashboard/blob/2ecf65c9c42aaf4ccdbbc5913c18ac78899982fe/components/profile-controller/config/manager/kustomization.yaml#L5-L14. To keep a single source of truth and ensure those manifests are configurable from a higher level, it 'd be nice to follow what's already done.
components/profile-controller/controllers/profile_controller.go
Outdated
Show resolved
Hide resolved
| if r.ServiceMeshMode == "ambient" { | ||
| // In ambient mode, disable sidecar injection but enable ambient mesh |
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.
Instead of including this if statement twice in the code, I think we could leverage the existing setNamespaceLabels() calls, by probably creating a new object including istio labels + defaultKubeflowNamespaceLabels and passing this to those calls.
Signed-off-by: madmecodes <[email protected]>
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.
EDIT: review in progress, submitted those comments by accident
| delete(ns.Labels, "istio.io/dataplane-mode") | ||
| delete(ns.Labels, "istio.io/use-waypoint") | ||
| delete(ns.Labels, "istio.io/use-waypoint-namespace") | ||
| delete(ns.Labels, "istio.io/ingress-use-waypoint") |
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.
good catch, kudos!
| // In ambient mode, we still use selector but target the waypoint workload | ||
| // TODO: Once Istio supports targetRef in AuthorizationPolicy, update this | ||
| if r.ServiceMeshMode == "istio-ambient" { | ||
| // For now, keep the selector-based approach for ambient mode | ||
| // The waypoint will be created separately and policies will apply to namespace workloads |
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.
Istio in ambient mode already supports the use of targetRef. That's how it can be bound to all traffic passing through the whole namespace https://github.com/orfeas-k/kubeflow-istio-ambient-demo/blob/main/use-case-c-programmatic-access-using-k8s-token/ns-owner-access-istio-ap-l7-waypoint.yaml#L38-L41.
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.
Good job @madmecodes !! We 're close, I 've left some comments. As a side note, I would appreciate if there was a response to comments to understand how each comment is addressed in the pushed changes, cause I needed to scan the changes to figure out the rationale.
| } | ||
|
|
||
| // Set service mesh labels based on mode | ||
| r.setServiceMeshLabels(ns, instance) |
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.
As a follow up to #127 (comment), can't this and setNamespaceLabels be one call? I don't see the benefit in doing two calls in the code to set namespace labels.
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.
Created setNamespaceLabelsAndServiceMesh() that handles both default labels and service mesh labels in one call
| - name: WAYPOINT_NAMESPACE | ||
| value: "" | ||
| - name: CREATE_WAYPOINT | ||
| value: "true" |
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.
Here it defaults to true while here to false. LEt's keep the default to false everywhere.
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.
Changed ambient patch to consistently use "false"
| } | ||
|
|
||
| // updateL4AuthorizationPolicy creates L4 AuthorizationPolicy to allow traffic from waypoint to services | ||
| func (r *ProfileReconciler) updateL4AuthorizationPolicy(profileIns *profilev1.Profile) error { |
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.
good job, this looks good!
| "cluster.local/ns/kubeflow/sa/ml-pipeline-ui") | ||
|
|
||
| return istioSecurity.AuthorizationPolicy{ | ||
| policy := istioSecurity.AuthorizationPolicy{ |
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.
Is this change necessary?
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.
Reverted to direct return since no policy modifications were actually needed
components/profile-controller/config/overlays/kubeflow-ambient/patches/kfam.yaml
Show resolved
Hide resolved
components/profile-controller/controllers/profile_controller.go
Outdated
Show resolved
Hide resolved
| - name: WAYPOINT_NAMESPACE | ||
| value: "" | ||
| - name: CREATE_WAYPOINT | ||
| value: "true" No newline at end of file |
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.
I don't think we need those here since we have the envFrom in the deployment and the same envvars in the configmap, right?
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.
Kept only SERVICE_MESH_MODE=istio-ambient override, removed redundant WAYPOINT_NAME, WAYPOINT_NAMESPACE, and CREATE_WAYPOINT
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.
When I tried to apply this I got
The HTTPRoute "profiles-kfam" is invalid:
* spec.hostnames[0]: Invalid value: "*": spec.hostnames[0] in body should match '^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'
* spec.parentRefs[0].namespace: Invalid value: "$(GATEWAY_NAMESPACE)": spec.parentRefs[0].namespace in body should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?$'
* spec.rules[0].backendRefs[0].namespace: Invalid value: "$(PROFILES_NAMESPACE)": spec.rules[0].backendRefs[0].namespace in body should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?$'
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.
When I tried to apply this I got
The HTTPRoute "profiles-kfam" is invalid: * spec.hostnames[0]: Invalid value: "*": spec.hostnames[0] in body should match '^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' * spec.parentRefs[0].namespace: Invalid value: "$(GATEWAY_NAMESPACE)": spec.parentRefs[0].namespace in body should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?$' * spec.rules[0].backendRefs[0].namespace: Invalid value: "$(PROFILES_NAMESPACE)": spec.rules[0].backendRefs[0].namespace in body should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?$'
i tried, some issue in injecting variable values, if i harcode them like this they work and are installed,
- $(GATEWAY_NAMESPACE) to istio-system
- $(PROFILES_NAMESPACE) to kubeflow
this is how i am installing:
- kind create cluster --name profile-controller-test
- kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.2.0/standard-install.yaml
- kustomize build /Users/themadme/gsoc/manifests/common/istio/istio-crds/base | kubectl apply -f -
Finally:
kustomize build config/overlays/kubeflow-ambient | kubectl apply --dry-run=client -f -
Result: All Resources Validated Successfully
Am i missing anything?
Signed-off-by: madmecodes <[email protected]>
Signed-off-by: madmecodes <[email protected]>
Signed-off-by: madmecodes <[email protected]>
Signed-off-by: madmecodes <[email protected]>
Signed-off-by: madmecodes <[email protected]>
- Fix HTTPRoute hostname validation (*.kubeflow.local to kubeflow.local) - Add ambient mode CLI arguments to manager container - Disable sidecar injection for ambient deployments - Separate KFAM and manager container arguments - Add SERVICE_MESH_MODE environment variable support Signed-off-by: madmecodes <[email protected]>
04904a1 to
29274e3
Compare
|
@Orfeas Kourkakis I have tested the Dashboard PR in a GKE cluster with ambient support. |
|
This pull request has been automatically marked as stale because it has not had recent activity. Members may comment |
Motivation
This PR makes the Kubeflow profile-controller compatible with Istio ambient mode, enabling Kubeflow to leverage service mesh architecture for improved performance and simplified operations.
Why this change is needed
Istio ambient mode represents a fundamental shift in service mesh architecture that provides simplified operations and improved performance. However, it requires different configuration and resources compared to traditional Istio sidecar mode:
istio-injection: enablednamespace labels and VirtualService resources for L7 routing (in CNI envoy sidecar is present but no istio-init privilege container )istio.io/dataplane-mode: ambient) and Gateway API resources for L7 routingThe profile-controller is responsible for configuring namespaces and network policies for Kubeflow user profiles. To support ambient mode, it needs to:
What this PR accomplishes
istio.io/dataplane-mode: ambient,istio.io/use-waypoint, etc.)SERVICE_MESH_MODEcontrols the mesh architecture, waypoint settings control proxy configurationConfiguration
SERVICE_MESH_MODE=istio-sidecarSERVICE_MESH_MODE=istio-ambientwith optional waypoint configuration:WAYPOINT_NAME: Name of waypoint proxy (default:waypoint)WAYPOINT_NAMESPACE: Waypoint namespace (optional, defaults to profile namespace)CREATE_WAYPOINT: Whether to create waypoint if it doesn't exist (default:false)This change is a critical component for enabling Kubeflow to work seamlessly in ambient mesh deployments, complementing the Gateway API support added to other Kubeflow controllers in PR #7736.
Configuration Options
New environment variables and CLI flags:
SERVICE_MESH_MODE:istio-sidecar(default) oristio-ambientWAYPOINT_NAME: Name of waypoint proxy (default:waypoint)WAYPOINT_NAMESPACE: Waypoint namespace (optional, defaults to profile namespace)CREATE_WAYPOINT: Whether to create waypoint if it doesn't exist (default:false)Deployment