Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions bootstrap/kubeadm/api/v1alpha2/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,23 @@ func (src *KubeadmConfig) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Verbosity = restored.Spec.Verbosity
dst.Spec.UseExperimentalRetryJoin = restored.Spec.UseExperimentalRetryJoin
dst.Spec.Files = restored.Spec.Files
for i := range restored.Spec.Files {
file := restored.Spec.Files[i]
if file.ContentFrom != nil {
dst.Spec.Files = append(dst.Spec.Files, file)

// Track files successfully up-converted. We need this to dedupe
// restored files from user-updated files on up-conversion. We store
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah makes sense

file := dst.Spec.Files[i]
dstPaths[file.Path] = &file
}

// If we find a restored file matching the file path of a v1alpha2
// 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 == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

dstFile.ContentFrom = restoredFile.ContentFrom
}
}

Expand Down
26 changes: 22 additions & 4 deletions bootstrap/kubeadm/api/v1alpha2/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package v1alpha2
import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
. "github.com/onsi/gomega"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -33,11 +35,16 @@ func TestConvertKubeadmConfig(t *testing.T) {

src := &v1alpha3.KubeadmConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "hub",
Name: "hub",
Annotations: map[string]string{},
},
Spec: v1alpha3.KubeadmConfigSpec{
Files: []v1alpha3.File{
{
Path: "/etc/another/file",
Owner: "ubuntu:ubuntu",
Encoding: v1alpha3.GzipBase64,
Permissions: "0600",
ContentFrom: &v1alpha3.FileSource{
Secret: v1alpha3.SecretFileSource{
Name: "foo",
Expand All @@ -46,7 +53,11 @@ func TestConvertKubeadmConfig(t *testing.T) {
},
},
{
Content: "baz",
Path: "/etc/kubernetes/azure.json",
Owner: "root:root",
Encoding: v1alpha3.Base64,
Permissions: "0644",
Content: "baz",
},
},
},
Expand All @@ -55,16 +66,23 @@ func TestConvertKubeadmConfig(t *testing.T) {
DataSecretName: pointer.StringPtr("secret-data"),
},
}
dst := &KubeadmConfig{}

g.Expect(dst.ConvertFrom(src)).To(Succeed())
dst := &KubeadmConfig{}
g.Expect(dst.ConvertFrom(src.DeepCopy())).To(Succeed())
restored := &v1alpha3.KubeadmConfig{}
g.Expect(dst.ConvertTo(restored)).To(Succeed())

// Test field restored fields.
g.Expect(restored.Name).To(Equal(src.Name))
g.Expect(restored.Status.Ready).To(Equal(src.Status.Ready))
g.Expect(restored.Status.DataSecretName).To(Equal(src.Status.DataSecretName))

diff := cmp.Diff(src.Spec.Files, restored.Spec.Files, cmpopts.SortSlices(func(i, j v1alpha3.File) bool {
return i.Path < j.Path
}))
if diff != "" {
t.Fatalf(diff)
}
})
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,25 @@ func TestClusterValidate(t *testing.T) {
},
expectErr: true,
},
"invalid with duplicate file path": {
in: &KubeadmConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "default",
},
Spec: KubeadmConfigSpec{
Files: []File{
{
Content: "foo",
},
{
Content: "bar",
},
},
},
},
expectErr: true,
},
}

for name, tt := range cases {
Expand Down
15 changes: 15 additions & 0 deletions bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var (
MissingFileSourceMsg = "source for file content must be specified if contenFrom is non-nil"
MissingSecretNameMsg = "secret file source must specify non-empty secret name"
MissingSecretKeyMsg = "secret file source must specify non-empty secret key"
PathConflictMsg = "path property must be unique among all files"
)

func (c *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error {
Expand Down Expand Up @@ -61,6 +62,8 @@ func (c *KubeadmConfig) ValidateDelete() error {
func (c *KubeadmConfigSpec) validate(name string) error {
var allErrs field.ErrorList

knownPaths := map[string]struct{}{}

for i := range c.Files {
file := c.Files[i]
if file.Content != "" && file.ContentFrom != nil {
Expand Down Expand Up @@ -98,6 +101,18 @@ func (c *KubeadmConfigSpec) validate(name string) error {
)
}
}
_, conflict := knownPaths[file.Path]
if conflict {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "files", fmt.Sprintf("%d", i), "path"),
file,
PathConflictMsg,
),
)
}
knownPaths[file.Path] = struct{}{}
}

if len(allErrs) == 0 {
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ require (
github.com/evanphx/json-patch v4.5.0+incompatible
github.com/go-logr/logr v0.1.0
github.com/gogo/protobuf v1.3.1
github.com/google/go-cmp v0.4.0
github.com/google/go-github v17.0.0+incompatible
github.com/google/go-querystring v1.0.0 // indirect
github.com/google/gofuzz v1.1.0
Expand Down