support/kind: create kubeconfig with stdout

[heylongdacoder]

trafficstars

What type of PR is this?

/kind bug

What this PR does / why we need it:

The current kubeconfig is created with both stdout and stderr. We should only use stdout to create the kubeconfig.

Which issue(s) this PR fixes:

Fixes #347

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., Usage docs, etc.:

NONE

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: heylongdacoder / name: Wen Long (02eca98bf5fc6c2e9f2f55a2b0b34830c5b9b8c2)

[k8s-ci-robot]

Welcome @heylongdacoder!

It looks like this is your first PR to kubernetes-sigs/e2e-framework 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/e2e-framework has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @heylongdacoder. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[tingsl409]

Definitely, nice to have!

[harshanarayana]

/ok-to-test

[harshanarayana]

/retest

[heylongdacoder]

/retest

[heylongdacoder]

seems like the TestKwokCluster is completely broken, I can try to have a look. But should I open another Issue and PR for that? Or I can do it with this PR? 😄

[heylongdacoder]

/retest

[heylongdacoder]

The test passed. 😄

[harshanarayana]

@heylongdacoder You are right. That test has been flaking a bit lately. I will take a look at that over the weekend to see if we can fix that once and for all.

/lgtm

/cc @vladimirvivien

[heylongdacoder]

Thanks for the review, everyone!

@vladimirvivien, I agree with your suggestion. Just want to confirm before I implement. Is that your idea is to add one more field to kind.Cluster(https://github.com/kubernetes-sigs/e2e-framework/blob/main/support/kind/kind.go#L43) like getKubeConfigOpts []string? 😄

[vladimirvivien]

@heylongdacoder yes, you will need a field to capture that configuration

*Cluster.WithQuiet()

Or something like that.

[heylongdacoder]

@vladimirvivien, please correct me if I misunderstood, are you suggesting that WithQuiet() will append -q to all the kind commands, such as kind get kubeconfig, kind create cluster, and so on? However, in my case, I only want the framework to produce a correct kubeconfig without silencing other commands. Now, I have another idea to fix this.

var stdout, stderr bytes.Buffer
p := gexe.NewProc(fmt.Sprintf(`%s get kubeconfig --name %s`, k.path, k.name))
p.SetStdout(&stdout)
p.SetStderr(&stderr)

What if we maintain the current get kubeconfig command and separate stdout and stderr? Stdout will be used to create the kubeconfig, and stderr will be used for logging, like log.V(4).Info(stderr.String()).

WDYT? 😄

[vladimirvivien]

@heylongdacoder originally, yes that was the intention. When/if someone uses Cluster.WithQuiet, it would append the -q to the command and therefore ignores errors. But, as you pointed out, it would

With respect to your new suggestion, how would separating the output help ? Let me know.

Another approach is to add a new method for getting the kubeconfig with options.

func (k *Cluster) GetKubeconfigWithOption(opts ...support.ClusterOpts) (string, error) {
...
}

Let me know if that makes sense.

[heylongdacoder]

With respect to your new suggestion, how would separating the output help ? Let me know.

^This is the combined stderr and stdout output.

^This is stdout output

^This is stderr output

@vladimirvivien This is the result I obtained with kind v0.17.0. Therefore, using only stdout to create the kubeconfig can also resolve this issue. 😄

[vladimirvivien]

@heylongdacoder I get a different result running on MacOS

 kind get kubeconfig --name kind
apiVersion: v1
clusters:
...

Notice that there is no extraneous text in the combined about, just the config. And my version

kind version
kind v0.17.0 go1.19 darwin/arm64

Is this behavior tied to your environment ?

I am asking these questions to make sure this will be a one-off and that it is a general fix/enhancement.

[heylongdacoder]

@vladimirvivien I guess this only happen when you are using podman as kind provider. I also found another merged PR(https://github.com/kubernetes-sigs/e2e-framework/pull/84) that fixed this issue. 😄

[vladimirvivien]

@heylongdacoder Ok. So can this PR be closed ? Or does #84 fixes your issue?

[heylongdacoder]

@vladimirvivien I think there was a regression. In #84, the kubeconfig was being retrieved from stdout (https://github.com/kubernetes-sigs/e2e-framework/pull/84/files#diff-c66ce9879f2c64ba698cc9f0688530c2068a838797e98b94080045eb1122d235R58). However, in the latest code (https://github.com/kubernetes-sigs/e2e-framework/blob/main/support/kind/kind.go#L119), it seems to be using both stderr and stdout again. 😄

I think the fixes in #84 make sense, I would like to do a similar fix. 🙏

[vladimirvivien]

Ok @heylongdacoder , can this PR put back what got regressed in #84 and we'll (re)start from there.

[heylongdacoder]

/retest

[Cajga]

We run into the same. Thanks @heylongdacoder for the PR. Will you have time to finish the PR?

[heylongdacoder]

hey @Cajga, recently got very busy with my new job. Probably I will have time to continue this weekend. Thanks!

[vladimirvivien]

@heylongdacoder Any progress on this front ?

[heylongdacoder]

yes, tested with podman and kind on my M1 chip Macbook. 😄

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshanarayana, heylongdacoder

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

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [harshanarayana]

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