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

Alpha support for provision volumes from cross-namespace data sources #805

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

ttakahashi21
Copy link
Contributor

@ttakahashi21 ttakahashi21 commented Nov 3, 2022

What type of PR is this?
/kind feature

What this PR does / why we need it:
Please see CrossNamespaceVolumeDataSource

Which issue(s) this PR fixes:

Part of ##804

Special notes for your reviewer:
Discuss the API definition kubernetes/kubernetes#113186.
The code may change due to API specification changes.

Does this PR introduce a user-facing change?:

Add support for cross-namespace data sources alpha feature

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @ttakahashi21!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. 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-csi/external-provisioner 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. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 3, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @ttakahashi21. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 3, 2022
@ttakahashi21 ttakahashi21 changed the title Update codes for controller and rbac Alpha support for provision volumes from cross-namespace snapshots Nov 3, 2022
@@ -57,6 +59,7 @@ import (
"github.com/kubernetes-csi/csi-lib-utils/rpc"
snapapi "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
snapclientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
referenceGrantv1alpha2 "sigs.k8s.io/gateway-api/pkg/client/listers/apis/v1alpha2"
Copy link
Contributor

@humblec humblec Nov 4, 2022

Choose a reason for hiding this comment

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

just wondering , considering we have v1beta1 available cant we use v1beta1 of RefGrants insted of alphav2? Isnt it better to use it ? @ttakahashi21

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ReferenceGrant is not yet in beta. Therefore, I'm using Alpha2.
See the URL below.
https://github.com/kubernetes-sigs/gateway-api

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to use it by adding the following to my go.mod file:

require sigs.k8s.io/gateway-api v0.5.1-0.20221110191515-2d586cc49ab4

HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha},
Topology: {Default: false, PreRelease: featuregate.GA},
HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha},
CrossNamespaceSourceProvisioning: {Default: false, PreRelease: featuregate.Alpha},
Copy link
Contributor

Choose a reason for hiding this comment

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

we also need to add this feature gate to the readme doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you tell me where should i write it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added CrossNamespaceVolumeDataSource in README.md.

@humblec
Copy link
Contributor

humblec commented Nov 4, 2022

@ttakahashi21 thanks. I have few review comments . Apart from that, can we add some tests to this PR ?

@humblec
Copy link
Contributor

humblec commented Nov 4, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot 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 Nov 4, 2022
@@ -57,6 +59,7 @@ import (
"github.com/kubernetes-csi/csi-lib-utils/rpc"
snapapi "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
snapclientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
referenceGrantv1alpha2 "sigs.k8s.io/gateway-api/pkg/client/listers/apis/v1alpha2"
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

return p.getSnapshotSourceInternal(ctx, claim, sc, claim.Namespace, claim.Spec.DataSourceRef2.Name)
}

if ok, err := p.IsGranted(ctx, claim.Spec.DataSourceRef2.Namespace, claim.Spec.DataSourceRef2.Name, claim); err != nil || !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we check somewhere if the PVC namespace is the same as the VolumeSnapshot namespace? If they are the same, then referenceGrant is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we check somewhere if the PVC namespace is the same as the VolumeSnapshot namespace?

Changed to check.

@robscott
Do you have any comments from the gateway api point of view?

Copy link

Choose a reason for hiding this comment

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

I'm not sure I get the context here. Agree that ReferenceGrant is only useful/necessary to enable cross-namespace references. Same namespace references should not require a ReferenceGrant.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 11, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2022
@ttakahashi21
Copy link
Contributor Author

/assign msau42

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2022
@@ -57,6 +57,9 @@ rules:
- apiGroups: ["storage.k8s.io"]
resources: ["volumeattachments"]
verbs: ["get", "list", "watch"]
- apiGroups: ["gateway.networking.k8s.io"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you comment these out for now with a note that it's alpha?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. But if I comment it out, CI fails. Is it possible to test without commenting out in the CI side test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the test failure you see?

go.mod Outdated
sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
sigs.k8s.io/yaml v1.3.0 // indirect
)

replace k8s.io/api => k8s.io/api v0.25.2
replace k8s.io/api => k8s.io/api v0.0.0-20221110160654-5cb32024090c
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this support human-friendly tags like v1.26.0-beta.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it temporarily because I need to use the API added for k8s 1.26 for successful CI testing. Therefore, I will change it when the release tag is attached.

}
rc.snapshot = true
default:
return nil, controller.ProvisioningFinished, fmt.Errorf("the PVC source specified via DataSourceRef with non-empty namespace does not belong to the right Kind. Expected %s, Got %s", snapshotKind, claim.Spec.DataSourceRef.Kind)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we allowing pvc source and data populators too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added. 1f9df93

}

for _, to := range grant.Spec.To {
if to.Group != snapshotAPIGroup || to.Kind != snapshotKind {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make the function more generic to handle non-snapshot data sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Is possible. Fixed code 1f9df93

@@ -582,6 +589,21 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
}
}
}

if utilfeature.DefaultFeatureGate.Enabled(features.CrossNamespaceVolumeDataSource) {
if claim.Spec.DataSourceRef != nil && claim.Spec.DataSourceRef.Namespace != nil && len(*claim.Spec.DataSourceRef.Namespace) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move the check for feature gate and return error earlier at this place. There's a number of csi calls we make after this that may be nice to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added feature gate check code early in the prepare Provision function.

if claim.Spec.DataSource != nil && (rc.clone || rc.snapshot) {
if claim.Spec.DataSourceRef != nil && claim.Spec.DataSourceRef.Namespace != nil && len(*claim.Spec.DataSourceRef.Namespace) > 0 {
if utilfeature.DefaultFeatureGate.Enabled(features.CrossNamespaceVolumeDataSource) && rc.snapshot {
volumeContentSource, err := p.getVolumeContentSourceFromXnsDataSource(ctx, claim, sc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a new function, or can we modify getVolumeContentSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Is possible. Fixed code 1f9df93
Could you check code?

@ttakahashi21 ttakahashi21 force-pushed the KEP-3294 branch 2 times, most recently from 2b8ea8c to bbd6d77 Compare November 18, 2022 08:08
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2022
@@ -57,6 +57,9 @@ rules:
- apiGroups: ["storage.k8s.io"]
resources: ["volumeattachments"]
verbs: ["get", "list", "watch"]
- apiGroups: ["gateway.networking.k8s.io"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the test failure you see?

go.mod Outdated
@@ -29,6 +29,8 @@ require (
sigs.k8s.io/sig-storage-lib-external-provisioner/v8 v8.0.0
)

require sigs.k8s.io/gateway-api v0.5.1-0.20221110191515-2d586cc49ab4
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of creating a new require block, can you add it to the block above?

Also is there a release tag of gateway-api we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also is there a release tag of gateway-api we can use?
There is none.

@robscott
Could you please create a tag that includes the ReferenceGrant API Beta?

@@ -537,6 +544,11 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
return nil, controller.ProvisioningFinished, errors.New("storage class was nil")
}

if !utilfeature.DefaultFeatureGate.Enabled(features.CrossNamespaceVolumeDataSource) &&
claim.Spec.DataSourceRef != nil && claim.Spec.DataSourceRef.Namespace != nil && len(*claim.Spec.DataSourceRef.Namespace) > 0 {
return nil, controller.ProvisioningFinished, fmt.Errorf("error handling for DataSourceRef Type %s with non-empty namespace by Name %s: CrossNamespaceVolumeDataSource feature disabled", claim.Spec.DataSourceRef.Kind, claim.Spec.DataSourceRef.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can simplify the error message to say "dataSourceRef namespace specified but the CrossNamespaceVolumeDataSource feature is disabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -582,6 +594,28 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
}
}
}
if utilfeature.DefaultFeatureGate.Enabled(features.CrossNamespaceVolumeDataSource) {
if claim.Spec.DataSourceRef != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic looks the same as the DataSource logic. Can we create a common function that takes in Name and Kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// For now we shouldn't pass other things to this function, but treat it as a noop and extend as needed
return nil, nil
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the if/else? The logic looks exactly the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

return false, nil
}

// Get all ReferenceGrants in snapshot's namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

update comment to say "data source" instead of "snapshot"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -1081,47 +1175,117 @@ func (p *csiProvisioner) getPVCSource(ctx context.Context, claim *v1.PersistentV
return volumeContentSource, nil
}

func (p *csiProvisioner) IsGranted(ctx context.Context, claim *v1.PersistentVolumeClaim) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a todo that this function should eventually be replaced by a common kubernetes library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw does the gateway controller have a similar method?

}

for _, to := range grant.Spec.To {
if claim.Spec.DataSourceRef.APIGroup != nil && (string(to.Group) != *claim.Spec.DataSourceRef.APIGroup || string(to.Kind) != claim.Spec.DataSourceRef.Kind) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if dataSourceRef.APIGroup == nil and to.Group is set?

Also it should be groupMatches and kindMatches right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

if !allowed {
return false, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could just be return allowed, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

var claim *v1.PersistentVolumeClaim
var err error

if pvc.Spec.DataSourceRef != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How much of this logic can be simplified across all the data sources if we had a common function that can normalize DataSource and DataSourceRef? Something like:

func dataSource(claim *v1.PersistentVolumeClaim) (ObjectReference, error)

I see that this pattern repeats many times for each of the data source types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is still room for improvement, but I was able to normalize and remove the duplicated code. Is the fix as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to check the featuregate in the dataSource function. Also, since it is confirmed at the beginning of the prepareProvision function, the confirmation of featuregate removed in the "IsGrant ", "getSnapshotSource ", and "getPVCSource " function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks much better now thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2022
@ttakahashi21 ttakahashi21 changed the title Alpha support for provision volumes from cross-namespace snapshots Alpha support for provision volumes from cross-namespace data sources Nov 28, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2022
@ttakahashi21 ttakahashi21 force-pushed the KEP-3294 branch 2 times, most recently from 59b292e to ef748fe Compare November 30, 2022 05:42
@ttakahashi21
Copy link
Contributor Author

#805 (comment)
I'm sorry. I was mistaken. No error occurs.

var claim *v1.PersistentVolumeClaim
var err error

if len(dataSource.Namespace) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can dataSource also set the namespace field so we don't need this check?

Copy link
Contributor Author

@ttakahashi21 ttakahashi21 Nov 30, 2022

Choose a reason for hiding this comment

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

It is possible. Fixed code.

var err error
if len(dataSource.Namespace) > 0 {
if claim.Namespace != dataSource.Namespace {
if ok, err := p.IsGranted(ctx, claim); err != nil || !ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can IsGranted also be moved to dataSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible. Fixed code.

@@ -1081,47 +1175,117 @@ func (p *csiProvisioner) getPVCSource(ctx context.Context, claim *v1.PersistentV
return volumeContentSource, nil
}

func (p *csiProvisioner) IsGranted(ctx context.Context, claim *v1.PersistentVolumeClaim) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw does the gateway controller have a similar method?


var provisionRequest controller.ProvisionOptions
if setnamespace {
provisionRequest = controller.ProvisionOptions{
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like almost everything is the same and the only difference is the namespace field. I think you can move this out of this if block and only change the namespace inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

for k, tc := range testcases {
tc := tc
t.Run(k, func(t *testing.T) {
t.Parallel()
if !tc.xnsEnabled {
t.Parallel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does the xns feature require serial tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the test did not work with feature gate enabled when it was executed in parallel test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm that seems to imply there is some global shared state somewhere. I think it would be good to investigate more into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultFeatureGate defined in utilfeature seems a global value, therefore setting value during tests causes changes to a global value in each test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for investigting, can you add a comment and open an issue so we can track improving this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the link of the issue I created as a comment

},
}
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be collapsed into 2 conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -1081,47 +1118,108 @@ func (p *csiProvisioner) getPVCSource(ctx context.Context, claim *v1.PersistentV
return volumeContentSource, nil
}

// TODO: This function should eventually be replaced by a common kubernetes library.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For now this is a pretty important function from a security perspective. Can we move this out into a util file and have extensive unit tests for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new file of util.go and util_test.go on the controller package.

Copy link
Collaborator

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

Thanks for all the great work! I just have a few unit test suggestions.

}

for k, tc := range testcases {
tc := tc
t.Run(k, func(t *testing.T) {
t.Parallel()
if !tc.xnsEnabled {
t.Parallel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for investigting, can you add a comment and open an issue so we can track improving this in the future?

func (p *csiProvisioner) setCloneFinalizer(ctx context.Context, pvc *v1.PersistentVolumeClaim) error {
claim, err := p.claimLister.PersistentVolumeClaims(pvc.Namespace).Get(pvc.Spec.DataSource.Name)
func (p *csiProvisioner) setCloneFinalizer(ctx context.Context, pvc *v1.PersistentVolumeClaim, dataSource *v1.ObjectReference) error {
var claim *v1.PersistentVolumeClaim
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you restore the original code style, where the variables were defined inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

volOpts controller.ProvisionOptions
refGrantList []*gatewayv1beta1.ReferenceGrant
}
testcases := map[string]testcase{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case for nil datasource apigroup?

Copy link
Contributor Author

@ttakahashi21 ttakahashi21 Dec 11, 2022

Choose a reason for hiding this comment

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

testcases := map[string]testcase{
"Allowed to access dataSource for xns PVC with refGrant": {
volOpts: generatePVCForProvisionFromXnsdataSource(toNamespace, fromsrcName, "", pvcapiKind, &fromNamespace, &coreapiGrp, requestedBytes, "", true),
refGrantList: newRefGrantList("refGrant1", fromNamespace, coreapiGrp, pvcapiKind, toNamespace, coreapiGrp, pvcapiKind, "", false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we do this in our other test files, but I think it's a little difficult to read the test case with this style of constructor because I need to remember the ordering of the arguments and which field they correspond to. It's perhaps more lines, but I think it's more readable to directly reference the object in the test case definition.

If you want to reduce the boilerplate, maybe you can still have a constructor, but have it take a ReferenceGrantFrom and a ReferenceGrantTo as arguments?

Similar comment for generatePVC... Keep the constructor to reduce on the common boilerplate, but have it take DataSourceRef type as an input, which is what changes across each test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

util_test.go created a base test case. And I modified the base test case and modified it to add the test case.
Along with the above modifications, TestProvisionFromSnapshot and TestProvisionFromPVC have also been modified.

In util.go I added the following just in case:
https://github.com/kubernetes-csi/external-provisioner/pull/805/files#diff-ded124c02add3467ce103641d0cd28436ea69cf37c5906470b3d519009d950b7R16-R20

As it turned out to be necessary in the test case below.
https://github.com/kubernetes-csi/external-provisioner/pull/805/files#diff-640957054096359bf678ef9f2789673a60814fba8286165ad298f5eba881536aR166-R170

This condition is unnecessary in the controller because it is acquired by specifying the namespace, but I added it just in case.
https://github.com/kubernetes-csi/external-provisioner/pull/805/files#diff-243ebed2765f75e6a54f57167212fefb08c3b2a85967ad2acbc0eb78919019c1R1936-R1937

@msau42
Copy link
Collaborator

msau42 commented Dec 12, 2022

/lgtm
/approve

Thanks for adding all the comprehensive unit tests!

For the release note, can you update it to say "Add support for cross-namespace data sources alpha feature"

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, ttakahashi21

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
No open projects
Status: PRs Approved
Development

Successfully merging this pull request may close these issues.

6 participants