⚒️ fix CAPBK file conversion, add validating webhook #3100

alexeldeib · 2020-05-26T22:09:40Z

What this PR does / why we need it:
Follow up from #3083

CecileRobertMichon · 2020-05-26T22:14:11Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

-		file := restored.Spec.Files[i]
+
+	// Track files successfully up-converted.
+	// We need this


incomplete sentence?

We need this

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

vincepri · 2020-05-26T22:35:05Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

+	for i, file := range src.Spec.Files {
+		if file.ContentFrom != nil {
+			// Cut file with referenced content.
+			// It's already in the marshaled data.
+			dst.Spec.Files = append(dst.Spec.Files[:i], dst.Spec.Files[i+1:]...)
+		}
+	}


I'm not sure if this is advisable, if I'm reading the code here and above correctly, we're removing all the files with file.ContentFrom and then restoring it later?

Another thing we should probably do is to backport the changes we made on making Content non-required to v1alpah2

We can index on path name and set contentFrom where content is unset. Unfortunately, content is a string, not a string pointer, so detecting unset vs. empty is not really possible otherwise.

The alternative is converting the secret content into raw exposed string, but that doesn't make much sense to me.

Thoughts on options?

We can change content to string pointer, but I wasn't sure about that since it would require client changes

I see, if we relax the Content field in v1alpha2, allowing it to be empty we should be able to set ContentFrom back regardless? It's probably fair to say that ContentFrom takes precedence over Content

We could also start validating against empty string for content? But I could imagine sentinel files being useful for stuff, I don't really want to introduce that dependency.

I didn't run into anything that seemed to cause issues with empty string in v1a2, let me know if that's somehow mistaken.

Our current tests won't run into OpenAPI validation for v1alpha2 types anymore, but given that the v1alpha2 field is required https://github.com/kubernetes-sigs/cluster-api/blob/master/bootstrap/kubeadm/api/v1alpha2/kubeadmbootstrapconfig_types.go#L140 we need to relax that to allow v1alpha2 controllers to operate

Isn’t an empty string valid? I thought that would mean the field would get serialized as empty string, and adding omitempty to string would omit the field?

I think you're right, having it without omitempty makes an empty string a valid value

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

CecileRobertMichon · 2020-05-27T17:46:36Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

+	// them as pointers for later modification without paying for second
+	// lookup.
+	dstPaths := make(map[string]*kubeadmbootstrapv1alpha3.File, len(dst.Spec.Files))
+	for i := range dst.Spec.Files {


Suggested change

for i := range dst.Spec.Files {

for _, file := range dst.Spec.Files {

The first time you left this in review I agreed but for this one I don’t think I do because we need the pointer to the indexed value?

yeah makes sense

CecileRobertMichon · 2020-05-27T17:47:36Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

+	// file with no content, we should restore contentFrom to that file.
+	for _, restoredFile := range restored.Spec.Files {
+		dstFile, exists := dstPaths[restoredFile.Path]
+		if exists && dstFile.Content == "" {


I read the other thread but I'm still not sure I follow why we're checking for empty Content here instead of checking for set ContentFrom... isn't empty string content a valid File use case?

So if you set a file content to empty in either v1a2 or v1a3, that still works. This will only try to match files where a v1a3 file was downconverted into empty content and then re-unconverted. It requires a match on the path.

Realistically, I don’t think this makes any difference. When does downconversion even happen in this scenario? V1a3 is the storage version, this is mostly for the sake of maintaining roundtrip-ability?

That said I know string pointer is more correct and this feels weird, but I think it works okay in practice

CecileRobertMichon

/lgtm

vincepri

/approve
/milestone v0.3.7

k8s-ci-robot · 2020-05-27T20:57:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, vincepri

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

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 26, 2020

k8s-ci-robot requested review from CecileRobertMichon and justinsb May 26, 2020 22:10

CecileRobertMichon reviewed May 26, 2020

View reviewed changes

alexeldeib force-pushed the ace/convert branch 2 times, most recently from 08aeaf6 to 55161db Compare May 26, 2020 22:23

⚒️ fix conversion, add validating webhook

be4c8a9

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

alexeldeib force-pushed the ace/convert branch from 55161db to be4c8a9 Compare May 26, 2020 22:32

vincepri reviewed May 26, 2020

View reviewed changes

alexeldeib force-pushed the ace/convert branch 2 times, most recently from 12d0864 to 03ce2cf Compare May 26, 2020 23:35

⚒️ adjust conversion based on review

de469ca

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

alexeldeib force-pushed the ace/convert branch 2 times, most recently from fa71c57 to de469ca Compare May 26, 2020 23:35

CecileRobertMichon reviewed May 27, 2020

View reviewed changes

CecileRobertMichon approved these changes May 27, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon May 27, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2020

vincepri approved these changes May 27, 2020

View reviewed changes

k8s-ci-robot added this to the v0.3.7 milestone May 27, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020

k8s-ci-robot merged commit b37f94b into kubernetes-sigs:master May 27, 2020

alexeldeib mentioned this pull request May 28, 2020

🐛 fix CAPBK conversion bug #3114

Merged

	for i := range dst.Spec.Files {
	for _, file := range dst.Spec.Files {

⚒️ fix CAPBK file conversion, add validating webhook #3100

⚒️ fix CAPBK file conversion, add validating webhook #3100

Conversation

alexeldeib commented May 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincepri May 27, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vincepri May 27, 2020 •

edited

Loading