Preserve path for cluster server endpoints (fixes virtctl with Rancher) #7361

SeanKnight · 2022-03-12T03:24:41Z

What this PR does / why we need it:

This PR fixes virtctl so that it can be used with clusters accessed via Rancher's authentication proxy.

Here is an example kubeconfig to illustrate the problem:

apiVersion: v1
kind: Config
clusters:
- name: "rancher-managed-cluster"
  cluster:
    server: "https://rancher.example.com/k8s/clusters/c-137"  # this is a cluster managed by Rancher
- name: "rancher-server"
  cluster:
    server: "https://rancher.example.com"  # the cluster running Rancher Server, similar URL but no path
...

As you can see, the URL for the rancher-managed-cluster contains a path component, which is valid, but uncommon so one might assume that they are never present if they have not encountered it before.

I ran virtctl console myvmi with a debugger until I found where the path component was being discarded:

kubevirt/staging/src/kubevirt.io/client-go/kubecli/vmi.go

Line 150 in a424098

    
           u.Path = fmt.Sprintf("/apis/subresources.kubevirt.io/%s/namespaces/%s/%s/%s/%s", v1.ApiStorageVersion, namespace, resource, name, subresource)

Before the above line is executed, the value of u.Path was:

/k8s/clusters/c-137

After the line was executed the observed value was:

/apis/subresources.kubevirt.io/v1alpha3/namespaces/default/virtualmachinesinstances/myvmi/console

The correct value that should be expected is:

/k8s/clusters/c-137/apis/subresources.kubevirt.io/v1alpha3/namespaces/default/virtualmachinesinstances/myvmi/console

The result is that the API request ends up going to the rancher-server cluster instead of rancher-managed-cluster, which results in very confusing error messages since it was not obvious that the wrong cluster was responding.

The proposed code change appends to the u.Path value instead of replacing it.

Which issue(s) this PR fixes:

Fixes #3760, Fixes #2608

Special notes for your reviewer:

Tested with my cluster accessed through Rancher, and tests in vmi_test.go passed, but no other testing was performed.

Release note:

Fixed a bug that prevents virtctl from working with clusters accessed via Rancher authentication proxy, or any other cluster where the server URL contains a path component. (#3760)

kubevirt-bot · 2022-03-12T03:24:51Z

Hi @SeanKnight. Thanks for your PR.

I'm waiting for a kubevirt 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.

Fuzzy-Math · 2022-03-12T05:56:50Z

staging/src/kubevirt.io/client-go/kubecli/vmi.go

+	u.Path = path.Join(
+		u.Path,
+		fmt.Sprintf("/apis/subresources.kubevirt.io/%s/namespaces/%s/%s/%s/%s", v1.ApiStorageVersion, namespace, resource, name, subresource),
+	)
 	u.Path = fmt.Sprintf("/apis/subresources.kubevirt.io/%s/namespaces/%s/%s/%s/%s", v1.ApiStorageVersion, namespace, resource, name, subresource)


This line would need to be removed otherwise it will overwite u.Path

Thank you very much, it slipped back in there when I was typing up the PR and wanted to get the variable value. I have fixed it.

vasiliy-ul · 2022-03-14T07:01:11Z

/ok-to-test

vasiliy-ul · 2022-03-14T09:37:44Z

/retest

SeanKnight · 2022-03-17T07:15:37Z

/retest

SeanKnight · 2022-03-17T07:20:25Z

/retest

SeanKnight · 2022-03-17T09:16:21Z

I undid my changes (as a new commit) and force pushed, and the same test failed (and now an additional one), so I do not believe my commit is what is causing these tests to fail.

SeanKnight · 2022-03-17T09:22:02Z

@dankenigsberg is it possible that the pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2 failure could be caused by ca2686a#diff-005747350099de38fcd22401693fe634b2c4607b4deec581b298efe440b93d2fR154 ? That is the most recent change I see on the test that is failing.

Signed-off-by: Sean Knight <git@seanknight.com>

SeanKnight · 2022-03-17T09:25:13Z

Rebased from origin/main in case recent commits fixed something.

dankenigsberg · 2022-03-17T10:16:02Z

dankenigsberg is it possible that the pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2 failure could be caused by ca2686a#diff-005747350099de38fcd22401693fe634b2c4607b4deec581b298efe440b93d2fR154 ? That is the most recent change I see on the test that is failing.

The failure in https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/7361/pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2/1503304370333159424#1:build-log.txt%3A3038 seems about the kubevirtci cluster not responding fast enough; it seem unrelated to my recent changes.

Unfortunately, it is not uncommon for a single test of ours to fail due to a fluke timeout. My PR #7251 is a small step towards simplifying and normalizing our tests. I hope more people join this cleanup effort, which should eventually make our tests more reliable and easier to debug.

vasiliy-ul · 2022-03-17T13:48:52Z

I undid my changes (as a new commit) and force pushed, and the same test failed (and now an additional one), so I do not believe my commit is what is causing these tests to fail.

The CI failure does not seem related to the changes here. Probably just some flaky tests. From my perspective the PR looks fine. @rmohr , maybe you could take a look as well?

/lgtm
/retest

rmohr · 2022-03-17T14:46:23Z

I undid my changes (as a new commit) and force pushed, and the same test failed (and now an additional one), so I do not believe my commit is what is causing these tests to fail.

The CI failure does not seem related to the changes here. Probably just some flaky tests. From my perspective the PR looks fine. @rmohr , maybe you could take a look as well?

/lgtm /retest

Yes everything looks great. We had temporary issues due to a github issue and, and a public golang proxy going down. The job backlog is causing a little bit more pressure on CI than it should.

@SeanKnight I love that PR! The fix is so simple. This issue was bugging us for a really long time.

/approve

kubevirt-bot · 2022-03-17T14:46:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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~~ [rmohr]

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

SeanKnight · 2022-03-17T20:18:44Z

@dankenigsberg thank you for taking a look, I could not for the life of me figure out what was going on.

@vasiliy-ul @rmohr thank you too for taking a look at this.

/retest

🤞

kubevirt-commenter-bot · 2022-03-17T23:15:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2022-03-18T05:16:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

SeanKnight · 2022-03-18T07:57:06Z

/retest

rmohr · 2022-03-18T08:03:01Z

@SeanKnight I know our merge experience for new contributors is not optimal, but you can just leave it as is. Our bot will take care of the retests (but you are welcome to write retest yourself if you like).

SeanKnight · 2022-03-18T08:27:05Z

@rmohr thank you I will let the bot take care of it.

kubevirt-commenter-bot · 2022-03-18T13:17:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 12, 2022

kubevirt-bot requested review from AlonaKaplan and marceloamaral March 12, 2022 03:24

kubevirt-bot added the size/XS label Mar 12, 2022

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 12, 2022

Fuzzy-Math reviewed Mar 12, 2022

View reviewed changes

SeanKnight force-pushed the virtctl-rancher branch from d6dba21 to 8fc8f3a Compare March 12, 2022 06:11

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 14, 2022

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 17, 2022

SeanKnight force-pushed the virtctl-rancher branch from 47034d1 to a0513b3 Compare March 17, 2022 07:19

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Mar 17, 2022

SeanKnight force-pushed the virtctl-rancher branch from a0513b3 to 8fc8f3a Compare March 17, 2022 09:16

Preserve path for cluster server endpoints (fixes virtctl with Rancher)

4cd6269

Signed-off-by: Sean Knight <git@seanknight.com>

SeanKnight force-pushed the virtctl-rancher branch from 8fc8f3a to 4cd6269 Compare March 17, 2022 09:23

kubevirt-bot assigned vasiliy-ul Mar 17, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2022

kubevirt-bot merged commit fd67d84 into kubevirt:main Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve path for cluster server endpoints (fixes virtctl with Rancher) #7361

Preserve path for cluster server endpoints (fixes virtctl with Rancher) #7361

SeanKnight commented Mar 12, 2022

kubevirt-bot commented Mar 12, 2022

Fuzzy-Math Mar 12, 2022

SeanKnight Mar 12, 2022

vasiliy-ul commented Mar 14, 2022

vasiliy-ul commented Mar 14, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

dankenigsberg commented Mar 17, 2022

vasiliy-ul commented Mar 17, 2022

rmohr commented Mar 17, 2022

kubevirt-bot commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

kubevirt-commenter-bot commented Mar 17, 2022

kubevirt-commenter-bot commented Mar 18, 2022

SeanKnight commented Mar 18, 2022

rmohr commented Mar 18, 2022

SeanKnight commented Mar 18, 2022

kubevirt-commenter-bot commented Mar 18, 2022

Preserve path for cluster server endpoints (fixes virtctl with Rancher) #7361

Preserve path for cluster server endpoints (fixes virtctl with Rancher) #7361

Conversation

SeanKnight commented Mar 12, 2022

kubevirt-bot commented Mar 12, 2022

Fuzzy-Math Mar 12, 2022

Choose a reason for hiding this comment

SeanKnight Mar 12, 2022

Choose a reason for hiding this comment

vasiliy-ul commented Mar 14, 2022

vasiliy-ul commented Mar 14, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

dankenigsberg commented Mar 17, 2022

vasiliy-ul commented Mar 17, 2022

rmohr commented Mar 17, 2022

kubevirt-bot commented Mar 17, 2022

SeanKnight commented Mar 17, 2022

kubevirt-commenter-bot commented Mar 17, 2022

kubevirt-commenter-bot commented Mar 18, 2022

SeanKnight commented Mar 18, 2022

rmohr commented Mar 18, 2022

SeanKnight commented Mar 18, 2022

kubevirt-commenter-bot commented Mar 18, 2022