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

Experimental Support for ambient on Windows #1461

Draft
wants to merge 24 commits into
base: experimental-windows-ambient
Choose a base branch
from

Conversation

keithmattix
Copy link
Contributor

@keithmattix keithmattix commented Feb 21, 2025

Part of istio/istio#27893. Istio CNI piece is at istio/istio#55216

More information to follow in an upcoming IstioDay talk as well as a blog post, but in summary: we've made substantial progress towards support Istio on Windows, and we decided the time is right to share where we are with the community! After discussing with the rest of Istio TOC, we have agreed that a long-lived experimental branch is the best place for this code to live for now as we work towards productionizing it and getting CI set up.

What works

  • in pod traffic redirection
  • HBONE upgrade
  • waypoint forwarding
  • ZDS communication with ztunnel

What doesn't work

  • DNS proxying (haven't tested, redirection in CNI isn't implemented)
  • Communicating with istiod via DNS (windows host process pods can't resolve via kube-dns)
  • Tests that rely on creating a netns programmatically (pre-WS2025, I don't think this is possible because of how compartments and namespaces work)
  • Probably more stuff I'm forgetting

I'm opening this PR as a draft for folks to take a look and comment on the diff/approach before merging it into the experimental branch. Feel free to reach out with any questions or concerns. Welcome to Istio, Windows!

@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 21, 2025
@istio-policy-bot
Copy link

😊 Welcome @keithmattix! This is either your first contribution to the Istio ztunnel repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 21, 2025
proto/zds.proto Outdated
// Add a workload to the ztunnel. this will be accompanied by ancillary data contianing
// the workload's netns file descriptor.
message AddWorkload {
string uid = 1;
WorkloadInfo workload_info = 2;
// The namespace for the workload. Windows only.
WindowsNamespace windows_namespace = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make this an optional in WorkloadInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I think it was in an earlier draft of this, but I can't remember why the shift. Agreed that an optional is much cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I remember now, it was because I wanted to model it like the auxiliary data (i.e. the FD) from the unix domain socket communication. I don't really have any preference, and went ahead and just stuck it on workload_info

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

What happened to a "small 60 line change" 😛

Good stuff. Will take a look soon

Signed-off-by: Keith Mattix II <[email protected]>

cfg_if::cfg_if! {
if #[cfg(target_os = "windows")] {
output = Command::new("powershell.exe")

Choose a reason for hiding this comment

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

Suggested change
output = Command::new("powershell.exe")
let output = Command::new("powershell.exe")

}
}

match output {
Ok(output) => {

Choose a reason for hiding this comment

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

Minor issue here where it doesn't print anything when powershell fails to execute the script (eg. because of permission policies).

       Ok(output) => {
            if output.status.success() {
                for line in String::from_utf8(output.stdout).unwrap().lines() {
                    // Each line looks like `istio.io/pkg/version.buildGitRevision=abc`
                    if let Some((key, value)) = line.split_once('=') {
                        let key = key.split('.').last().unwrap();
                        println!("cargo:rustc-env=ZTUNNEL_BUILD_{key}={value}");
                    } else {
                        println!("cargo:warning=invalid build output {line}");
                    }
                }
            } else {
                println!(
                    "cargo:warning=build info script failed (stderr): {}",
                    String::from_utf8(output.stderr)
                        .unwrap()
                        .replace("\r\n", "")
                );
            }
        }

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 23, 2025
@istio-testing
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants