Enable automatic token generation for VirtualMachineExport objects #8418

alromeros · 2022-09-06T17:48:10Z

What this PR does / why we need it

This PR introduces a new mechanism in the export controller so that when a VMExport with no tokenSecretRef is created, a corresponding secret is made using a name specific to the current object.

Which issue(s) this PR fixes: https://issues.redhat.com/browse/CNV-20651

Special notes for your reviewer: Work in progress

Release note:

Enable automatic token generation for VirtualMachineExport objects

kubevirt-bot · 2022-09-06T17:48:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mhenriks

Great start!

mhenriks · 2022-09-07T00:10:45Z

pkg/storage/export/export/export.go

+	// If no tokenSecretRef has been specified, we use one specific to the current export object
+	tokenSecretRef := vmExport.Spec.TokenSecretRef
+	if tokenSecretRef == "" {
+		tokenSecretRef = getDefaultTokenSecretName(vmExport)


I think it would be useful to store the token secret name in the VM export status. So refer to that in this function instead of calling getDefaultTokenSecretName again?

mhenriks · 2022-09-07T00:15:28Z

pkg/storage/export/export/export.go

+	// The secret name is constructed so it can be specific to the current vmExport object
+	tokenSecretName := getDefaultTokenSecretName(vmExport)
+
+	token := rand.String(secretTokenLenght)


I think we have to use a more cryptographically secure rng for the token. Looks like this package uses "math/rand" which "implements pseudo-random number generators unsuitable for security-sensitive work".

hmmm I'm honestly surprised by that since I thought k8s.io/apimachinery/pkg/util/rand was our standard for this kind of thing. I see that k8s.io/client-go/util/cert uses crypto/rand, so I'll find a way to use that one instead (also in virtctl vmexport).

Yeah, math/rand is fine for naming things but not for secrets

mhenriks · 2022-09-07T14:42:53Z

pkg/storage/export/export/export.go

+	token := make([]byte, secretTokenLenght)
+	if _, err := cryptorand.Read(token); err != nil {
+		return err
+	}


We still want this to be string. The value has to be included in an http header. So maybe do something like this: https://gist.github.com/dopey/c69559607800d2f2f90b1b1ed4e550fb#file-main-go-L47-L59

Basically whatever the k8s library function was doing but with a better random source

mhenriks · 2022-09-07T14:45:09Z

staging/src/kubevirt.io/api/export/v1alpha1/types.go

+	// +optional
+	// DefaultSecretExportName is the name of an optional secret specifically created for the current VirtualMachineExport object
+	// in case no other secret name is specified when creating the object
+	DefaultSecretExportName string `json:"defaultSecretExportName,omitempty"`


I would just call this TokenSecretRef and set it to whatever the user set explicitly or the one we create. One source of truth to get the secret name

mhenriks · 2022-09-13T14:14:05Z

staging/src/kubevirt.io/api/export/v1alpha1/types.go

-	TokenSecretRef string `json:"tokenSecretRef"`
+	// +optional
+	// TokenSecretRef is the name of the custom-defined secret that contains the token used by the export server pod
+	TokenSecretRef string `json:"tokenSecretRef,omitempty"`


Should probably be a pointer

Out of curiosity, I'm seeing that we use regular strings for all other fields in the export API (even in the Spec's TokenSecretRef). Why we'd want this one to be a pointer?

I have seen CSI APIs use type LocalObjectReference struct for SecretRef but then on the other hand we have CDI's which are strings (type DataVolumeSourceRegistry struct for example)

Out of curiosity, I'm seeing that we use regular strings for all other fields in the export API (even in the Spec's TokenSecretRef). Why we'd want this one to be a pointer?

we are specifically talking about the spec.tokenSecretRef

Since this field is optional, it should be a pointer per kubernetes api conventions. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Of course we are not always consistent about this

mhenriks · 2022-09-13T14:27:22Z

pkg/storage/export/export/export.go

+	}
+
+	// If not, the secret name is constructed so it can be specific to the current vmExport object
+	vmExport.Status.TokenSecretRef = getDefaultTokenSecretName(vmExport)


I would first check if vmExport.Status.TokenSecretRef is set and use that value if so. This way if, in the unlikely case that we change the naming scheme, existing exports will still work.

akalenyu · 2022-09-14T15:53:54Z

pkg/storage/export/export/export.go

@@ -105,6 +106,11 @@ const (

 	kvm = 107

+	// secretTokenLenght is the lenght of the randomly generated token
+	secretTokenLenght = 20


good catch!

akalenyu · 2022-09-14T15:57:10Z

tests/storage/export.go

@@ -703,6 +724,7 @@ var _ = SIGDescribe("Export", func() {
 			Skip("Skip test when Filesystem storage is not present")
 		}
 		vmExport := createRunningPVCExport(sc, k8sv1.PersistentVolumeFilesystem)
+		Expect(vmExport.Spec.TokenSecretRef).To(Equal(vmExport.Status.TokenSecretRef))


mby should also ensure it has some reasonable length so its not empty?

alromeros · 2022-09-15T16:52:58Z

/retest-required

This commit introduces a new mechanism in the export controller so, when a VMExport with no tokenSecretRef is created, a corresponding secret is created using a name specific to the current object. This commit also modifies the export API to make the tokenSecretRef field optional. Signed-off-by: Alvaro Romero <alromero@redhat.com>

…nts: * The randomly generated token for the default export secret is now created using the more secure package crypto/rand * The default secret name is now stored in the VirtualMachineExport status Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Fixed error in export pod error handling, so if the pod creation fails, now the controller returns the error appropiately. * The name generation mechanism of the default export secret has been fixed, so no names longer than 63 characters are created. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2022-09-16T11:36:31Z

/retest-required

mhenriks

Looks great, just a couple minor comments

mhenriks · 2022-09-16T14:27:32Z

pkg/storage/export/export/export.go

@@ -599,6 +609,65 @@ func (ctrl *VMExportController) createCertSecretManifest(vmExport *exportv1.Virt
 	}, nil
 }

+// handleVMExportSecret checks if a secret has been specified for the current export object and, if not, creates one specific to it
+func (ctrl *VMExportController) handleVMExportSecret(vmExport *exportv1.VirtualMachineExport) error {


I think this should be called handleVMExportToken or handleVMExportTokenGeneration

mhenriks · 2022-09-16T14:31:13Z

pkg/storage/export/export/export.go

@@ -609,6 +678,11 @@ func (ctrl *VMExportController) getExportSecretName(ownerPod *corev1.Pod) string
 	return certSecretName
 }

+// getDefaultTokenSecretName returns a secret name specifically created for the current export object
+func getDefaultTokenSecretName(vme *exportv1.VirtualMachineExport) string {
+	return naming.GetName("export-secret", vme.Name, validation.DNS1035LabelMaxLength)


I think this should be export-token or vmexport-token

mhenriks · 2022-09-16T14:33:08Z

tests/storage/export.go

+		Expect(err).ToNot(HaveOccurred())
+		Expect(token.Name).To(Equal(*export.Status.TokenSecretRef))
+		Expect(*export.Status.TokenSecretRef).ToNot(BeEmpty())
+	})


How about downloading something with generated token?

Good idea, I'll add a new test in the virtctl vmexport section.

alromeros · 2022-09-16T17:19:27Z

/retest-required

mhenriks · 2022-09-19T18:40:36Z

/approve

kubevirt-bot · 2022-09-19T18:40:46Z

[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

mhenriks · 2022-09-19T18:40:59Z

@awels how about giving this a once over?

awels · 2022-09-19T18:44:10Z

Sure let me take a look

awels

looks good a few minor things

awels · 2022-09-19T18:47:21Z

pkg/storage/export/export/export.go

+
+	// If a tokenSecretRef has been specified, we assume that the corresponding
+	// secret has already been created and managed appropiately by the user
+	if isTokenSpecified := vmExport.Spec.TokenSecretRef != nil; isTokenSpecified {


Can't you just write:

if vmExport.Spec.TokenSecretRef != nil { vmExport.Status.TokenSecretRef = vmExport.Spec.TokenSecretRef return nil }

awels · 2022-09-19T19:03:04Z

pkg/storage/export/export/export.go

+func (ctrl *VMExportController) handleVMExportToken(vmExport *exportv1.VirtualMachineExport) error {
+	if vmExport.Status == nil {
+		vmExport.Status = &exportv1.VirtualMachineExportStatus{
+			Phase: exportv1.Pending,


Since we are always setting this here, do we still need to set Pending and the conditions in updateCommonVMExportStatusFields?

I guess we don't, however, I like how we check before dereferencing it. It shouldn't be necessary anyway, so I can remove the one in updateCommonVMExportStatusFields.

awels · 2022-09-19T19:13:23Z

pkg/storage/export/export/export_test.go

@@ -989,7 +1068,7 @@ func createVMVMExport() *exportv1.VirtualMachineExport {
 				Kind:     "VirtualMachine",
 				Name:     testVmName,
 			},
-			TokenSecretRef: "token",
+			TokenSecretRef: &token,


What you did is not wrong, this is just an alternative:

TokenSecretRef: pointer.StringPtr("token")

Saves you from having to create the variable at the top.

awels · 2022-09-19T19:16:30Z

pkg/storage/export/export/export.go

+	if vmExport.Status != nil && vmExport.Status.TokenSecretRef != nil {
+		tokenSecretRef = *vmExport.Status.TokenSecretRef
+	}
+


is it possible that tokenSecretRef is blank here? Not sure if we want to create the pod with a blank secret name?

I would say it's impossible (handleVMExportToken should always populate vme.Status.TokenSecretRef), but still wanted to check before dereferencing the pointer.

* This commit introduces a new functional test so we cover the download of an export created with user-defined TokenSecretRef * It also renames several token-related items in the export controller Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2022-09-20T15:26:32Z

/test pull-kubevirt-e2e-kind-1.23-vgpu-nonroot

awels · 2022-09-20T16:32:00Z

/hold
/lgtm
Putting hold to give others chance to review, if no reviews in a few days I will remove the hold

awels · 2022-09-23T13:46:00Z

/hold cancel

alromeros · 2022-09-23T19:01:05Z

/retest-required

alromeros · 2022-09-24T00:14:34Z

/test pull-kubevirt-e2e-k8s-1.24-sig-storage

kubevirt-bot · 2022-09-24T02:13:23Z

@alromeros: The following tests 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	`c794f87`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot`
pull-kubevirt-e2e-k8s-1.22-sig-storage	`c794f87`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.22-sig-storage`
pull-kubevirt-e2e-k8s-1.23-sig-storage	`c794f87`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.23-sig-storage`

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-09-24T05:27:57Z

/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 requested review from EdDev and maiqueb September 6, 2022 17:48

mhenriks reviewed Sep 7, 2022

View reviewed changes

kubevirt-bot added size/L and removed size/M labels Sep 7, 2022

mhenriks reviewed Sep 7, 2022

View reviewed changes

alromeros force-pushed the make-export-secret-optional branch from f9b40de to 221d9bf Compare September 7, 2022 16:52

alromeros mentioned this pull request Sep 7, 2022

Add vmexport command to virtctl #8197

Merged

alromeros marked this pull request as ready for review September 9, 2022 16:18

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 Sep 9, 2022

kubevirt-bot requested review from jean-edouard and kbidarkar September 9, 2022 16:22

alromeros force-pushed the make-export-secret-optional branch from d3b721d to 64dfff5 Compare September 12, 2022 08:03

mhenriks reviewed Sep 13, 2022

View reviewed changes

akalenyu reviewed Sep 14, 2022

View reviewed changes

alromeros force-pushed the make-export-secret-optional branch 3 times, most recently from 71424ec to 4d7e05a Compare September 15, 2022 13:02

alromeros added 3 commits September 16, 2022 09:52

alromeros force-pushed the make-export-secret-optional branch from 4d7e05a to 6f74394 Compare September 16, 2022 08:55

mhenriks reviewed Sep 16, 2022

View reviewed changes

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

awels reviewed Sep 19, 2022

View reviewed changes

Address review comments

c794f87

* This commit introduces a new functional test so we cover the download of an export created with user-defined TokenSecretRef * It also renames several token-related items in the export controller Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the make-export-secret-optional branch from c54c44b to c794f87 Compare September 20, 2022 10:50

alromeros requested review from awels and removed request for kbidarkar, jean-edouard, EdDev and maiqueb September 20, 2022 16:14

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2022

kubevirt-bot assigned awels Sep 20, 2022

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

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022

kubevirt-bot merged commit af00da8 into kubevirt:main Sep 24, 2022

kubevirt-bot mentioned this pull request Sep 26, 2022

Patching node set kubevirt.io/schedulable to false upon exiting heartbeat #8238

Merged

Enable automatic token generation for VirtualMachineExport objects #8418

Enable automatic token generation for VirtualMachineExport objects #8418

Conversation

alromeros commented Sep 6, 2022

What this PR does / why we need it

kubevirt-bot commented Sep 6, 2022

mhenriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks Sep 7, 2022 • edited

Choose a reason for hiding this comment

alromeros Sep 7, 2022 • edited

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

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

alromeros commented Sep 15, 2022

alromeros commented Sep 16, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros commented Sep 16, 2022

mhenriks commented Sep 19, 2022

kubevirt-bot commented Sep 19, 2022

mhenriks commented Sep 19, 2022

awels commented Sep 19, 2022

awels left a 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

alromeros commented Sep 20, 2022

awels commented Sep 20, 2022

awels commented Sep 23, 2022

alromeros commented Sep 23, 2022

alromeros commented Sep 24, 2022

kubevirt-bot commented Sep 24, 2022 • edited

kubevirt-commenter-bot commented Sep 24, 2022

mhenriks Sep 7, 2022 •

edited

alromeros Sep 7, 2022 •

edited

kubevirt-bot commented Sep 24, 2022 •

edited