feat: Add ambient mode support to profile-controller (clone)

[kimwnasptd]

This is a clone of the existing PR #127 plus the fixes from #127 (comment), so that the AuthorizationPolicy gets attached to the waypoint (L7 traffic).

The PR also bumps the golang version, as it's needed to use the latest Istio module version (for using targetRefs).

@madmecodes I'm happy to drop this PR, if we resolve the last comment on the other one. Otherwise let's move on with this PR to ensure we close the loop.

/cc @juliusvonkohout @madmecodes

[kimwnasptd]

I'm also getting the License Compliance error as the initial PR, but I can't understand from the logs of the PR which package has the issue.

@juliusvonkohout do you know how to debug this?

[juliusvonkohout]

/retest

[kimwnasptd]

/retest

[kimwnasptd]

@juliusvonkohout the PR is already based on top of latest main, so at this point I can't do much more. Any other ideas?

EDIT: I closed and re-opened the PR and this seemed to do the trick...

[juliusvonkohout]

Thank you for the PR. The GO updates are really important. Regarding the manifests I am worried a bit about duplication and files not being in the right place. Nevertheless i only had a short amount of time to review and might have missed something.

[kimwnasptd]

@juliusvonkohout the summary is that @madmecodes wanted to keep the existing manifests intact, and replicated the overlays/kubeflow one to the new ambient overlay.

Do you suggest that we work on factoring out the common manifests in this PR? I'd personally be a bit cautious of whether we want to include this one as well in this PR. But WDYT?

[juliusvonkohout]

@juliusvonkohout the summary is that @madmecodes wanted to keep the existing manifests intact, and replicated the overlays/kubeflow one to the new ambient overlay.

Do you suggest that we work on factoring out the common manifests in this PR? I'd personally be a bit cautious of whether we want to include this one as well in this PR. But WDYT?

Yes a minimal kustomize component or if that not possible a minimal kustomize overlay. Otherwise the maintainability degrades a lot.

[kimwnasptd]

/retest

[kimwnasptd]

The unit tests are now suddenly failing with

Failure [0.002 seconds]
[BeforeSuite] BeforeSuite 
/home/runner/work/dashboard/dashboard/components/profile-controller/controllers/suite_test.go:52
  Unexpected error:
      <*fmt.wrapError | 0xc000658aa0>: {
          msg: "unable to start control plane itself: failed to start the controlplane. retried 5 times: exec: \"etcd\": executable file not found in $PATH",
          err: <*fmt.wrapError | 0xc000658a80>{
              msg: "failed to start the controlplane. retried 5 times: exec: \"etcd\": executable file not found in $PATH",
              err: <*exec.Error | 0xc000658a40>{
                  Name: "etcd",
                  Err: <*errors.errorString | 0x2ca2c50>{
                      s: "executable file not found in $PATH",
                  },
              },
          },
      }
      unable to start control plane itself: failed to start the controlplane. retried 5 times: exec: "etcd": executable file not found in $PATH
  occurred
  /home/runner/work/dashboard/dashboard/components/profile-controller/controllers/suite_test.go:64

I see this in some other repos kubernetes-sigs/kueue#7786 but would need to look deeper

---

@juliusvonkohout the requested manifests changes should now be ready for review

[juliusvonkohout]

Please rebase to master to fix the conflicts.

[thesuperzapper]

Also, while we hope to get this in for the next release I am going to put a hold on it so I can cut a release without it in case we don't make it in time for the 1-2 weeks we have before Kubeflow 26.03

But please keep working on it.

/hold

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from juliusvonkohout. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• components/profile-controller/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

This PR adds Istio ambient mode support to the profile-controller, enabling it to configure namespaces for ambient mesh with appropriate labels and create waypoint proxy resources. It is a clone of PR #127 with an additional fix to attach the AuthorizationPolicy to the waypoint (L7 traffic) via targetRefs. The PR also upgrades several dependencies including the Go version, istio.io/api, and various golang.org/x/* packages.

Changes:

• Adds ambient mode controller logic: new ServiceMeshMode, WaypointName, WaypointNamespace, CreateWaypoint configuration; createWaypoint() and updateL4AuthorizationPolicy() functions; updated setNamespaceLabelsAndServiceMesh() to apply correct Istio labels per mode
• Splits the istio Kustomize component into istio-common (AuthorizationPolicy), istio-sidecar (VirtualService), and istio-ambient (HTTPRoute), and adds a new kubeflow-ambient overlay
• Bumps Go to 1.23, istio.io/api to v1.28.1, adds sigs.k8s.io/gateway-api v0.5.1, and upgrades various transitive dependencies

Reviewed changes

Copilot reviewed 15 out of 18 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
controllers/profile_controller.go	Core ambient mode logic: waypoint creation, L4 policy, namespace labeling, targetRefs for AuthorizationPolicy, katib-controller principal addition
controllers/profile_controller_test.go	Updated tests to use setNamespaceLabelsAndServiceMesh instead of old setNamespaceLabels
main.go	New CLI flags for ambient mode config; Gateway API scheme registration
go.mod	Go version bump to 1.23, adds gateway-api dependency, bumps istio.io/api and other packages
go.sum	Updated checksums for new/bumped dependencies
Dockerfile	Go base image bump to 1.23; removes hashicorp copy step
Makefile	Contains an unresolved merge conflict marker
manifests/.../istio-common/	Removes VirtualService (moved to istio-sidecar); now contains only AuthorizationPolicy
manifests/.../istio-sidecar/	New component containing VirtualService for sidecar mode
manifests/.../istio-ambient/	New component containing HTTPRoute for ambient mode
manifests/.../kubeflow-ambient/	New overlay for deploying in ambient mode with appropriate params and patches
manifests/.../kubeflow/kustomization.yaml	Updated to use new istio-common + istio-sidecar split components

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kimwnasptd]

@juliusvonkohout @thesuperzapper I've rebased on top of the latest main. But, I've sqashed the commits as it was being really painful to keep rebasing. I've kept one commit with the code changes and one with the manifests changes.

On the manifests note, @thesuperzapper @alokdangre really great job refactoring the manifests!

The changes I did in this PR are the following regarding the manifests:

1. Update the istio component to istio-common, to include only the AuthorizationPolicy
2. Created an istio-sidecar component, that includes only the VirtualService
3. Created an istio-ambient component, that includes only the HTTPRoute
4. Updated the kubeflow component to use both the now istio-common and the new istio-sidecar components
5. Created a new kubeflow-ambient component
   1. It uses the replacements to properly set the KFAM host in the HTTPRoute, and also handle the ServiceAccount
   2. Adds a patch for the Profile Controller Deployment, to get the new args for ambient
   3. Uses the istio-common and istio-ambient components

This should now be ready for review!

[kimwnasptd]

I'm also removing the hold as we don't yet have the freeze, so that this can't be included. Let's add it back once/if we know the PR should not be merged

/hold cancel

[Copilot]

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

[christian-heusel]

Closing this in favor of the rebased version in #267 picked up by @dariofaccin
/close

[google-oss-prow]

@christian-heusel: Closed this PR.

Details

In response to this:

Closing this in favor of the rebased version in #267 picked up by @dariofaccin
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.