Simplify handling of cluster URIs from kubeconfig #8499

vasiliy-ul · 2022-09-21T11:41:14Z

What this PR does / why we need it:

Handling of cluster URIs with additional path elements added in the commit fba65ed can be simplified with the use of Request.AbsPath

kubevirt/vendor/k8s.io/client-go/rest/request.go

Lines 305 to 317 in 0f2c671

    
           // AbsPath overwrites an existing path with the segments provided. Trailing slashes are preserved 
        
           // when a single segment is passed. 
        
           func (r *Request) AbsPath(segments ...string) *Request { 
        
           	if r.err != nil { 
        
           		return r 
        
           	} 
        
           	r.pathPrefix = path.Join(r.c.base.Path, path.Join(segments...)) 
        
           	if len(segments) == 1 && (len(r.c.base.Path) > 1 || len(segments[0]) > 1) && strings.HasSuffix(segments[0], "/") { 
        
           		// preserve any trailing slashes for legacy behavior 
        
           		r.pathPrefix += "/" 
        
           	} 
        
           	return r 
        
           }

which literally does the same as the introduced adaptUriForHostPath function:

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

Lines 177 to 183 in 0f2c671

    
           func (v *vmis) adaptUriForHostPath(uri string) string { 
        
           	u, err := url.Parse(v.config.Host) 
        
           	if err != nil { 
        
           		return uri 
        
           	} 
        
           	return path.Join(u.Path, uri) 
        
           }

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

This is a follow-up of the discussion #8129 (comment)

Release note:

NONE

xpivarc · 2022-09-21T11:45:50Z

Thanks for following up @vasiliy-ul !
/cc

vasiliy-ul · 2022-09-21T11:50:44Z

Thanks for following up @vasiliy-ul ! /cc

👌

... Still, let's see if the CI tests pass. I also found a couple more places where complex cluster URIs were not handled. Fixed those as well.

vasiliy-ul · 2022-09-21T14:02:02Z

@mlhnono68, FYI. Maybe you could also take a look here? Unfortunately, I do not have a Rancher cluster to test e2e. Though I checked the resulting request URIs with different kubeconfig's and the path is preserved.

vasiliy-ul · 2022-09-21T14:51:37Z

/retest-required

xpivarc · 2022-09-21T14:59:56Z

/approve
I think we could simulate this with functional test. It might be tricky as we would need to spawn proxy(the easier part) and modify kubeconfig for the one test.

xpivarc · 2022-09-21T15:09:12Z

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

@@ -65,7 +65,7 @@ var _ = Describe("Kubevirt Version Client", func() {
 				ghttp.RespondWithJSONEncoded(http.StatusOK, groupInfo),
 			),
 			ghttp.CombineHandlers(
-				ghttp.VerifyRequest("GET", "/apis/"+groupInfo.PreferredVersion.GroupVersion+"/version"),
+				ghttp.VerifyRequest("GET", "/apis"+groupInfo.PreferredVersion.GroupVersion+"/version"),


How come we don't need this "/"?

Yeah, looks a bit weird. It appears that we use /apis//apis/subresources.kubevirt.io/v1alpha3/version uri (with //):

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

Lines 79 to 83 in 0f2c671

// Now, query the preferred version

uri = fmt.Sprintf("%s/apis/%s/version", u.Path, group.PreferredVersion.GroupVersion)

var serverInfo version.Info

result = v.restClient.Get().RequestURI(uri).Do(context.Background())

Here group.PreferredVersion.GroupVersion already contains a leading / (potentially we can fix also the format if it makes sense).

Previously, it was passed to the RequestURI method, which under the hood calls url.Parse(uri). This one preserves the path as-is with //. Hence, we needed this extra / in the test.

With this patch, the uri is now passed to AbsPath, which does path.Join(..., uri). It looks like this join sanitizes the resulting path and the output has only a single /.

Simple reproducer: https://go.dev/play/p/IU80SS_Gj5v

https://pkg.go.dev/path#Join:

Join joins any number of path elements into a single path, separating them with slashes. Empty elements are ignored. The result is Cleaned. However, if the argument list is empty or all its elements are empty, Join returns an empty string.

vasiliy-ul · 2022-09-22T05:46:09Z

I think we could simulate this with functional test. It might be tricky as we would need to spawn proxy(the easier part) and modify kubeconfig for the one test.

I was thinking more in the direction of unit-testing. Even if we manage to setup a proxy in CI and to use a custom config, still calling all the APIs e2e will require a significant amount of effort and resources.

xpivarc · 2022-09-23T13:01:18Z

I think we could simulate this with functional test. It might be tricky as we would need to spawn proxy(the easier part) and modify kubeconfig for the one test.

I was thinking more in the direction of unit-testing. Even if we manage to setup a proxy in CI and to use a custom config, still calling all the APIs e2e will require a significant amount of effort and resources.

Agree. I was thinking about discovery & dry-run but I think unit-tests are enough too.

vasiliy-ul · 2022-09-26T10:47:09Z

Added unit tests in a separate commit. Basically, I used the existing tests for regular URIs and transformed them to tables parametrized with an additional path element.

vasiliy-ul · 2022-09-26T14:39:30Z

/test pull-kubevirt-e2e-k8s-1.23-sig-compute-nonroot

vasiliy-ul · 2022-09-27T07:31:59Z

Added more unit-tests. No functional changes.

Handling of cluster URIs with additional path elements added in the commit fba65ed can be simplified with the use of Request.AbsPath which literally does the same as the introduced adaptUriForHostPath function. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

E.g. https://rancher-url/k8s/clusters/c-m-someid Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

xpivarc

/approve
@rmohr Can you take a look, please?

xpivarc · 2022-09-27T08:14:26Z

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

-
-	result := v.restClient.Get().RequestURI(uri).Do(context.Background())
+	uri := ApiGroupName
+	result := v.restClient.Get().AbsPath(uri).Do(context.Background())


nit: You can inline the uri

kubevirt-bot · 2022-09-27T08:19:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

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

xpivarc · 2022-10-07T08:37:06Z

/cc @fossedihelm Would you be able to have a look?

fossedihelm · 2022-10-07T08:38:54Z

@xpivarc sure

fossedihelm · 2022-10-07T09:28:30Z

Thanks!
/lgtm
/retest

vasiliy-ul · 2022-10-07T14:20:58Z

/retest-required

vasiliy-ul · 2022-10-07T16:53:01Z

/retest-required

kubevirt-commenter-bot · 2022-10-07T21:46:52Z

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

kubevirt-bot · 2022-10-07T23:19:17Z

@vasiliy-ul: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot	`e9c7c65`	link	true	`/test pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot`

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. I understand the commands that are listed here.

kubevirt-commenter-bot · 2022-10-08T02:46:52Z

/retest-required
This bot automatically retries required 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-none dco-signoff: yes size/L labels Sep 21, 2022

kubevirt-bot requested review from acardace and rmohr September 21, 2022 11:41

kubevirt-bot requested a review from xpivarc September 21, 2022 11:45

kubevirt-bot added the approved label Sep 21, 2022

xpivarc reviewed Sep 21, 2022

View reviewed changes

kubevirt-bot added size/XL and removed size/L labels Sep 26, 2022

vasiliy-ul force-pushed the simplify-uri-handling branch from f007747 to 88a42cb Compare September 27, 2022 07:31

vasiliy-ul added 2 commits September 27, 2022 10:03

Add unit tests to cover proxied server URLs

e9c7c65

E.g. https://rancher-url/k8s/clusters/c-m-someid Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

vasiliy-ul force-pushed the simplify-uri-handling branch from 88a42cb to e9c7c65 Compare September 27, 2022 08:05

xpivarc reviewed Sep 27, 2022

View reviewed changes

kubevirt-bot assigned fossedihelm Oct 7, 2022

kubevirt-bot added the lgtm label Oct 7, 2022

kubevirt-bot merged commit 85575d0 into kubevirt:main Oct 8, 2022

vasiliy-ul deleted the simplify-uri-handling branch October 9, 2022 12:34

	// AbsPath overwrites an existing path with the segments provided. Trailing slashes are preserved
	// when a single segment is passed.
	func (r Request) AbsPath(segments ...string) Request {
	if r.err != nil {
	return r
	}
	r.pathPrefix = path.Join(r.c.base.Path, path.Join(segments...))
	if len(segments) == 1 && (len(r.c.base.Path) > 1 \|\| len(segments[0]) > 1) && strings.HasSuffix(segments[0], "/") {
	// preserve any trailing slashes for legacy behavior
	r.pathPrefix += "/"
	}
	return r
	}

	func (v *vmis) adaptUriForHostPath(uri string) string {
	u, err := url.Parse(v.config.Host)
	if err != nil {
	return uri
	}
	return path.Join(u.Path, uri)
	}

	// Now, query the preferred version
	uri = fmt.Sprintf("%s/apis/%s/version", u.Path, group.PreferredVersion.GroupVersion)
	var serverInfo version.Info

	result = v.restClient.Get().RequestURI(uri).Do(context.Background())

Simplify handling of cluster URIs from kubeconfig #8499

Simplify handling of cluster URIs from kubeconfig #8499

Uh oh!

Conversation

vasiliy-ul commented Sep 21, 2022

Uh oh!

xpivarc commented Sep 21, 2022

Uh oh!

vasiliy-ul commented Sep 21, 2022

Uh oh!

vasiliy-ul commented Sep 21, 2022

Uh oh!

vasiliy-ul commented Sep 21, 2022

Uh oh!

xpivarc commented Sep 21, 2022

Uh oh!

xpivarc Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

vasiliy-ul Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasiliy-ul commented Sep 22, 2022

Uh oh!

xpivarc commented Sep 23, 2022

Uh oh!

vasiliy-ul commented Sep 26, 2022

Uh oh!

vasiliy-ul commented Sep 26, 2022

Uh oh!

vasiliy-ul commented Sep 27, 2022

Uh oh!

xpivarc left a comment

Choose a reason for hiding this comment

Uh oh!

xpivarc Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

kubevirt-bot commented Sep 27, 2022

Uh oh!

xpivarc commented Oct 7, 2022

Uh oh!

fossedihelm commented Oct 7, 2022

Uh oh!

fossedihelm commented Oct 7, 2022

Uh oh!

vasiliy-ul commented Oct 7, 2022

Uh oh!

vasiliy-ul commented Oct 7, 2022

Uh oh!

kubevirt-commenter-bot commented Oct 7, 2022

Uh oh!

kubevirt-bot commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubevirt-commenter-bot commented Oct 8, 2022

Uh oh!

Uh oh!

vasiliy-ul Sep 21, 2022 •

edited

Loading

kubevirt-bot commented Oct 7, 2022 •

edited

Loading