Move pkg/clone/auth to API lib and introduce single dv.Authorize() #2636

akalenyu · 2023-03-13T14:15:35Z

What this PR does / why we need it:
This way other projects don't have to vendor in CDI, the API lib will be enough.
While it's not a huge deal at this point, each existing dependency is an entry door for pulling in more stuff in the future by accident.
Also, these are annoying

containerized-data-importer/go.mod

Lines 26 to 27 in 20dc7d4

    
           github.com/openshift/api v0.0.0 
        
           github.com/openshift/client-go v0.0.0

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 #2552

Special notes for your reviewer:

Release note:

Move clone auth helpers to CDI API library

akalenyu · 2023-03-13T14:36:08Z

/cc @dhiller @mhenriks

mhenriks · 2023-03-13T19:08:04Z

pkg/apiserver/webhooks/datavolume-mutate.go

@@ -33,8 +33,8 @@ import (
 	"k8s.io/klog/v2"

 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
+	cdiv1cloneauth "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/clone"


I think kubevirt.io/containerized-data-importer-api/pkg/clone would be better. Let's keep pkg/apis for kubernetes apis

So we already have

containerized-data-importer/pkg/controller/common/util.go

Line 45 in be1add0

cdiv1utils "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/utils"

That function takes a DataVolume as a parameter though so having multiple versions of the api makes sense. These funcs however do not take any api resources as parameters

makes sense, was about to do this, but then realized we have #2552 which will accept a DV

okay then maybe it makes to refactor to do something like:

ok, err := dv.Authorize()

so Authorize() will go in ${version}/utils, and we will be able to have different authorization logic for different version?
I am just making sure, I have no issue with moving to kubevirt.io/containerized-data-importer-api/pkg/clone

I guess what I'm saying is do #2552 in this pr and have Authorize be a DataVolume member function. All code will exist under pkg/apis... no pkg/clone

Posted a first draft just to make sure we are fine with the direction

Still missing authorization via SA

Feels like the logic is clunky, any pointers are greatly appreciated

Would aggregating the proxies into one interface make sense?

akalenyu · 2023-03-16T13:50:18Z

/test all

akalenyu · 2023-03-16T19:16:00Z

/test all

mhenriks

Great start! I'm not that down on this

mhenriks · 2023-03-21T00:48:22Z

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/authorize.go

+
+// Authorize indicates if the creating context is authorized to create the data volume
+// For sources other than clone (import/upload/etc), this is a no-op
+func (dv *DataVolume) Authorize(requestNamespace, requestName string, sarClient SubjectAccessReviewsProxy, nsClient NamespaceProxy, dsClient DataSourceProxy, userInfo authentication.UserInfo) (bool, string, cloneSourceHandler, error) {


Maybe use the client native interfaces instead of our own proxies, so SubjectAccessReviewsGetter, NamespacesGetter, DataSourcesGetter?

Is that an improvement?

Scratch that, can't have DataSourcesGetter in here (circular dep). But maybe the others? Or will that make the deps situation worse?

Introduced a single type AuthorizationHelperProxy interface, hope that is fine

I'm not that down on this

I am not sure if you meant you're not down with the approach (don't like) or don't hate it too much (like~)

akalenyu · 2023-03-21T14:12:04Z

/test all

akalenyu · 2023-03-21T22:21:55Z

/test all

mhenriks · 2023-03-22T15:25:06Z

staging/src/kubevirt.io/containerized-data-importer-api/go.mod

@@ -3,9 +3,11 @@ module kubevirt.io/containerized-data-importer-api
 go 1.19

 require (
+	github.com/kubernetes-csi/external-snapshotter/client/v6 v6.0.1


Can we get rid of this somehow?

Let's keep this go.mod lean and mean. Copying string consts is fine

akalenyu · 2023-04-13T13:42:11Z

pkg/clone/auth.go

@mhenriks do you think it's fine to delete this? should we keep for backwards compatibility?

gentle ping

mhenriks · 2023-04-17T20:45:39Z

...ing/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/authorize_utils.go

+type dsGetFunc func(string, string) (*DataSource, error)
+
+// AuthorizationHelperProxy proxies calls to APIs used for DV authorization
+type AuthorizationHelperProxy interface {


Can't believe I just thought of this and I'd be remiss to not bring it up, what if our api just takes a dynamic.Interface instead of the proxy?

PRO:
Less code for clients

CON:
more code in api (translating to/from unstructured)
adding client-go to go.mod

Don't care too much either way just want to get your opinion

Would have to look up some references but the immediate concern is:
Would we still be able to restrict access to the passed client similarly to how we do with proxy?
I mean, with the proxy, external entities know that CDI doesn't have access to the client itself other than
the implemented Get/Create methods

With the proxy, callers know exactly what calls we are making, and that may be nice, but ultimately there is RBAC in place to keep the SA from doing anything bad. And obviously the code is available to audit and make sure we're not reading secrets or whatever.

Yes my concern is that we get passed a general SA that is probably not very limited,
unless you are suggesting to somehow reduce the SA

No I don't think it is practical to create a special client with reduced permissions just to pass to this api. I also don't think passing the dynamic client is a security concern.

I also don't think passing the dynamic client is a security concern.

I thought the reason we went with proxy structs in the first place was to avoid passing a full-blown client

I also don't think passing the dynamic client is a security concern.

I thought the reason we went with proxy structs in the first place was to avoid passing a full-blown client

The issue is with the typed client here: https://github.com/kubevirt/containerized-data-importer/blob/main/pkg/client/clientset/versioned/typed/core/v1beta1/datavolume.go#L40

That would cause a circular dependency

Ahh right.

I thought the reason we went with proxy structs in the first place was to avoid passing a full-blown client

We used the sarProxy prior to this PR though (and thus prior to the circular dep potential), was this^ not the main concern?

Anyway my (not 100% trustworthy) gut is telling me that whipping out a dynamic client
when we have a defined set of actions on objects we know about is not right, so I lean towards current impl

All past decisions are not made with perfect information. If I could go back in time I would probably use the dynamic client. But if you like using this interface, I don't care enough to keep belaboring the discussion.

mhenriks · 2023-04-21T14:11:01Z

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/authorize.go

+
+// AuthorizeUser indicates if the creating user is authorized to create the data volume
+// For sources other than clone (import/upload/etc), this is a no-op
+func (dv *DataVolume) AuthorizeUser(requestNamespace, requestName string, proxy AuthorizationHelperProxy, userInfo authentication.UserInfo) (bool, string, CloneSourceHandler, error) {


Rather than returning four vales, how about a new struct that combines the first three?

Yep makes sense, added
My biggest worry is still this though:
#2636 (comment)

I think it's fine to delete auth.go. I seriously doubt any project other than kubevirt is importing it

This way other projects don't have to vendor in CDI, the API lib will be enough. While it's not a huge deal at this point, each existing dependency is an entry door for pulling in more stuff in the future by accident. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

mhenriks · 2023-04-28T00:30:40Z

/approve

kubevirt-bot · 2023-04-28T00:30:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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

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

arnongilboa · 2023-05-08T07:24:06Z

tests/webhook_test.go

 	"kubevirt.io/containerized-data-importer/tests/framework"
 	"kubevirt.io/containerized-data-importer/tests/utils"
 )

-type sarProxy struct {
-	client kubernetes.Interface
+type authProxy struct {


why not using authProxy from datavolume-mutate.go?

I think it's nice that webhook_test.go will showcase all building blocks toward authorizing a data volume
plus, it feels a little "off" to export that struct from datavolume-mutate since it's not really of use to anyone but the tests

arnongilboa · 2023-05-09T10:12:57Z

/lgtm

This is a partial backport of kubevirt#2636 This tends to flake often, especially in OpenShift testing. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

This is a partial backport of #2636 This tends to flake often, especially in OpenShift testing. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

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 13, 2023

kubevirt-bot requested review from maya-r and mhenriks March 13, 2023 14:15

kubevirt-bot added the size/S label Mar 13, 2023

kubevirt-bot requested a review from dhiller March 13, 2023 14:36

mhenriks reviewed Mar 13, 2023

View reviewed changes

mhenriks mentioned this pull request Mar 13, 2023

Remove cdi dependency kubevirt/kubevirt#9348

Closed

akalenyu force-pushed the move-clone-auth-api branch from be1add0 to 6bcfbde Compare March 16, 2023 13:46

kubevirt-bot added size/L and removed size/S labels Mar 16, 2023

akalenyu changed the title ~~Move pkg/clone/auth to API lib~~ Move pkg/clone/auth to API lib and introduce single dv.Authorize() Mar 16, 2023

akalenyu marked this pull request as draft March 16, 2023 13:47

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2023

akalenyu force-pushed the move-clone-auth-api branch from 6bcfbde to 3e97126 Compare March 16, 2023 17:12

mhenriks reviewed Mar 21, 2023

View reviewed changes

akalenyu force-pushed the move-clone-auth-api branch from 3e97126 to a3deee6 Compare March 21, 2023 14:04

kubevirt-bot added size/XL and removed size/L labels Mar 21, 2023

akalenyu force-pushed the move-clone-auth-api branch from a3deee6 to dc9e31f Compare March 21, 2023 16:02

mhenriks reviewed Mar 22, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2023

akalenyu force-pushed the move-clone-auth-api branch from dc9e31f to c8af204 Compare April 13, 2023 13:40

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2023

akalenyu marked this pull request as ready for review April 13, 2023 13:41

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2023

kubevirt-bot requested review from aglitke and mhenriks April 13, 2023 13:41

akalenyu commented Apr 13, 2023

View reviewed changes

akalenyu force-pushed the move-clone-auth-api branch from c8af204 to e8d5bfd Compare April 16, 2023 14:03

kubevirt-bot added size/XXL and removed size/XL labels Apr 16, 2023

mhenriks reviewed Apr 17, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2023

akalenyu force-pushed the move-clone-auth-api branch from e8d5bfd to 7bf544e Compare April 19, 2023 11:32

kubevirt-bot added size/XL and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL labels Apr 19, 2023

mhenriks reviewed Apr 21, 2023

View reviewed changes

akalenyu force-pushed the move-clone-auth-api branch from 7bf544e to fabdbd1 Compare April 27, 2023 15:59

kubevirt-bot added size/XXL and removed size/XL labels Apr 27, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2023

arnongilboa reviewed May 8, 2023

View reviewed changes

kubevirt-bot assigned arnongilboa May 9, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2023

kubevirt-bot merged commit 4b2d633 into kubevirt:main May 9, 2023

mhenriks mentioned this pull request Jun 21, 2023

akalenyu nomination to approver #2766

Merged

akalenyu mentioned this pull request Sep 18, 2023

[release-v1.56] Fix flakes when using gigabytes intead of gibibytes #2901

Merged

kubevirt-bot pushed a commit that referenced this pull request Sep 18, 2023

Fix flakes when using gigabytes intead of gibibytes (#2901)

16ff45d

This is a partial backport of #2636 This tends to flake often, especially in OpenShift testing. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

	github.com/openshift/api v0.0.0
	github.com/openshift/client-go v0.0.0

Move pkg/clone/auth to API lib and introduce single dv.Authorize() #2636

Move pkg/clone/auth to API lib and introduce single dv.Authorize() #2636

Conversation

akalenyu commented Mar 13, 2023 • edited Loading

akalenyu commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu commented Mar 16, 2023

akalenyu commented Mar 16, 2023

mhenriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

akalenyu commented Mar 21, 2023

akalenyu commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Apr 28, 2023

kubevirt-bot commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa commented May 9, 2023

akalenyu commented Mar 13, 2023 •

edited

Loading

mhenriks Mar 14, 2023 •

edited

Loading

akalenyu Mar 21, 2023 •

edited

Loading

akalenyu Apr 18, 2023 •

edited

Loading

akalenyu Apr 20, 2023 •

edited

Loading