From 0caba1934ff63db09c8b7a1a5faa2c4209799657 Mon Sep 17 00:00:00 2001 From: Connor Kuehl Date: Wed, 13 Dec 2023 16:43:48 -0600 Subject: [PATCH] Implement webhook Signed-off-by: Connor Kuehl --- cmd/webhook/main.go | 7 +- pkg/admitter/cloudinit.go | 105 +++++++++++++++++++-- pkg/admitter/cloudinit_test.go | 164 +++++++++++++++++++++++++++++++++ pkg/mutator/cloudinit.go | 47 ++++++++-- pkg/mutator/cloudinit_test.go | 114 +++++++++++++++++++++++ 5 files changed, 422 insertions(+), 15 deletions(-) create mode 100644 pkg/admitter/cloudinit_test.go create mode 100644 pkg/mutator/cloudinit_test.go diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index a0920433..3bce90cb 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -92,8 +92,13 @@ func main() { func runWebhookServer(ctx context.Context, cfg *rest.Config, options *config.Options) error { webhookServer := server.NewWebhookServer(ctx, cfg, webhookName, options) + cloudinitValidator, err := admitter.NewCloudInitValidator(cfg) + if err != nil { + return err + } + var validators = []admission.Validator{ - admitter.NewCloudInitValidator(), + cloudinitValidator, } if err := webhookServer.RegisterValidators(validators...); err != nil { diff --git a/pkg/admitter/cloudinit.go b/pkg/admitter/cloudinit.go index 6766be6d..22a0c7b8 100644 --- a/pkg/admitter/cloudinit.go +++ b/pkg/admitter/cloudinit.go @@ -1,31 +1,120 @@ package admitter import ( + "context" "errors" + "fmt" + "path/filepath" "github.com/harvester/webhook/pkg/server/admission" admissionregv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" v1beta1 "github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1" + clientset "github.com/harvester/node-manager/pkg/generated/clientset/versioned" + cloudinitv1beta1 "github.com/harvester/node-manager/pkg/generated/clientset/versioned/typed/node.harvesterhci.io/v1beta1" ) +var ( + errFilenameTaken = errors.New("filename already in use") + errProtectedFilename = errors.New("filename conflicts with a critical system-owned file") + errMissingExt = errors.New("filename does not end in .yaml or .yml") +) + +var builtinFilenameDenyList = []string{ + "90_custom.yaml", + "99_settings.yaml", + "elemental.config", + "grubenv", + "harvester.config", + "install", +} + type CloudInit struct { admission.DefaultValidator + + cloudinits cloudinitv1beta1.CloudInitInterface +} + +func NewCloudInitValidator(config *rest.Config) (*CloudInit, error) { + client, err := clientset.NewForConfig(config) + if err != nil { + return nil, err + } + + cloudinits := client.NodeV1beta1().CloudInits() + + return &CloudInit{ + cloudinits: cloudinits, + }, nil +} + +func (v *CloudInit) Create(_ *admission.Request, newObj runtime.Object) error { + newCloudInit := newObj.(*v1beta1.CloudInit) + return v.validate(newCloudInit) +} + +func (v *CloudInit) Update(_ *admission.Request, oldObj runtime.Object, newObj runtime.Object) error { + oldCloudInit := oldObj.(*v1beta1.CloudInit) + newCloudInit := newObj.(*v1beta1.CloudInit) + + if oldCloudInit.Spec.Filename == newCloudInit.Spec.Filename { + return nil + } + + return v.validate(newCloudInit) +} + +func (v *CloudInit) validate(cloudinit *v1beta1.CloudInit) error { + if v.missingExtension(cloudinit.Spec.Filename) { + return errMissingExt + } + + if v.isProtectedFilename(cloudinit.Spec.Filename) { + return errProtectedFilename + } + + taken, err := v.isFilenameTaken(cloudinit.Spec.Filename) + if err != nil { + return fmt.Errorf("check for duplicate filename: %w", err) + } + + if taken { + return errFilenameTaken + } + + return nil } -func NewCloudInitValidator() *CloudInit { - return &CloudInit{} +func (v *CloudInit) missingExtension(name string) bool { + ext := filepath.Ext(name) + return ext != ".yaml" && ext != ".yml" } -func (v *CloudInit) Create(request *admission.Request, newObj runtime.Object) error { - _, _ = request, newObj - return errors.New("not implemented") +func (v *CloudInit) isFilenameTaken(name string) (bool, error) { + cloudinits, err := v.cloudinits.List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return true, err + } + + for _, cloudinit := range cloudinits.Items { + if cloudinit.Spec.Filename == name { + return true, nil + } + } + + return false, nil } -func (v *CloudInit) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) error { - _, _, _ = request, oldObj, newObj - return errors.New("not implemented") +func (v *CloudInit) isProtectedFilename(name string) bool { + for _, protected := range builtinFilenameDenyList { + if name == protected { + return true + } + } + return false } func (v *CloudInit) Resource() admission.Resource { diff --git a/pkg/admitter/cloudinit_test.go b/pkg/admitter/cloudinit_test.go new file mode 100644 index 00000000..b2453814 --- /dev/null +++ b/pkg/admitter/cloudinit_test.go @@ -0,0 +1,164 @@ +package admitter + +import ( + "context" + "errors" + "reflect" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/watch" + + v1beta1 "github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1" + "github.com/harvester/webhook/pkg/server/admission" +) + +func TestProtectedFilenames(t *testing.T) { + want := map[string]struct{}{ + "90_custom.yaml": {}, + "99_settings.yaml": {}, + "elemental.config": {}, + "grubenv": {}, + "harvester.config": {}, + "install": {}, + } + + got := make(map[string]struct{}) + for _, f := range builtinFilenameDenyList { + got[f] = struct{}{} + } + + if !reflect.DeepEqual(want, got) { + t.Errorf("want %v, got %v", want, got) + } +} + +func TestCreate(t *testing.T) { + origDenyList := builtinFilenameDenyList + defer func() { builtinFilenameDenyList = origDenyList }() + builtinFilenameDenyList = []string{ + "helloworld.yaml", + } + + existing := []v1beta1.CloudInit{ + {ObjectMeta: metav1.ObjectMeta{Name: "ssh-access"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}}, + } + + tests := []struct { + name string + input v1beta1.CloudInitSpec + want error + }{ + {"allow yaml", v1beta1.CloudInitSpec{Filename: "hi.yaml"}, nil}, + {"allow yml", v1beta1.CloudInitSpec{Filename: "hi.yml"}, nil}, + {"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken}, + {"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename}, + {"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctl := &CloudInit{cloudinits: &mockClient{list: existing}} + + cloudinit := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cloudinit"}, + Spec: tt.input, + } + + got := ctl.Create(new(admission.Request), cloudinit) + if !errors.Is(got, tt.want) { + t.Errorf("want err=%v, got err=%v", tt.want, got) + } + }) + } +} + +func TestUpdate(t *testing.T) { + origDenyList := builtinFilenameDenyList + defer func() { builtinFilenameDenyList = origDenyList }() + builtinFilenameDenyList = []string{ + "helloworld.yaml", + } + + existing := []v1beta1.CloudInit{ + {ObjectMeta: metav1.ObjectMeta{Name: "ssh-access"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}}, + } + + tests := []struct { + name string + input v1beta1.CloudInitSpec + want error + }{ + {"allow yaml", v1beta1.CloudInitSpec{Filename: "hi.yaml"}, nil}, + {"allow yml", v1beta1.CloudInitSpec{Filename: "hi.yml"}, nil}, + {"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken}, + {"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename}, + {"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctl := &CloudInit{cloudinits: &mockClient{list: existing}} + + cloudinit := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cloudinit"}, + Spec: tt.input, + } + + old := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cloudinit"}, + Spec: v1beta1.CloudInitSpec{Filename: "specifically-not-in-use.yaml"}, + } + + got := ctl.Update(new(admission.Request), old, cloudinit) + if !errors.Is(got, tt.want) { + t.Errorf("want err=%v, got err=%v", tt.want, got) + } + }) + } +} + +// Sadly, github.com/rancher/wrangler/pkg/generic/fake package generates mock clients that lack +// the ctx parameter that is required by the CloudInitInterface. + +type mockClient struct { + list []v1beta1.CloudInit +} + +func (m *mockClient) Create(_ context.Context, _ *v1beta1.CloudInit, _ v1.CreateOptions) (*v1beta1.CloudInit, error) { + return nil, errors.New("not implemented") +} + +func (m *mockClient) Update(_ context.Context, _ *v1beta1.CloudInit, _ v1.UpdateOptions) (*v1beta1.CloudInit, error) { + return nil, errors.New("not implemented") +} + +func (m *mockClient) UpdateStatus(_ context.Context, _ *v1beta1.CloudInit, _ v1.UpdateOptions) (*v1beta1.CloudInit, error) { + return nil, errors.New("not implemented") +} + +func (m *mockClient) Delete(_ context.Context, _ string, _ v1.DeleteOptions) error { + return errors.New("not implemented") +} + +func (m *mockClient) DeleteCollection(_ context.Context, _ v1.DeleteOptions, _ v1.ListOptions) error { + return errors.New("not implemented") +} + +func (m *mockClient) Get(_ context.Context, _ string, _ v1.GetOptions) (*v1beta1.CloudInit, error) { + return nil, errors.New("not implemented") +} + +func (m *mockClient) List(_ context.Context, _ v1.ListOptions) (*v1beta1.CloudInitList, error) { + return &v1beta1.CloudInitList{Items: m.list}, nil +} + +func (m *mockClient) Watch(_ context.Context, _ v1.ListOptions) (watch.Interface, error) { + return nil, errors.New("not implemented") +} + +func (m *mockClient) Patch(_ context.Context, _ string, _ types.PatchType, _ []byte, _ v1.PatchOptions, _ ...string) (result *v1beta1.CloudInit, err error) { + return nil, errors.New("not implemented") +} diff --git a/pkg/mutator/cloudinit.go b/pkg/mutator/cloudinit.go index df598158..746b0e9f 100644 --- a/pkg/mutator/cloudinit.go +++ b/pkg/mutator/cloudinit.go @@ -1,6 +1,9 @@ package mutator import ( + "fmt" + "path/filepath" + "github.com/harvester/webhook/pkg/server/admission" admissionregv1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/runtime" @@ -16,18 +19,50 @@ func NewCloudInitMutator() *CloudInit { return &CloudInit{} } -func (m *CloudInit) Create(_ *admission.Request, _ runtime.Object) (admission.Patch, error) { - var patch admission.Patch - // Not implemented, validator will fail request - return patch, nil +func (m *CloudInit) Create(_ *admission.Request, newObj runtime.Object) (admission.Patch, error) { + newCloudInit := newObj.(*v1beta1.CloudInit) + return patchFilenameIfNecessary(newCloudInit) } -func (m *CloudInit) Update(_ *admission.Request, _ runtime.Object, _ runtime.Object) (admission.Patch, error) { +func (m *CloudInit) Update(_ *admission.Request, _ runtime.Object, newObj runtime.Object) (admission.Patch, error) { + newCloudInit := newObj.(*v1beta1.CloudInit) + return patchFilenameIfNecessary(newCloudInit) +} + +func patchFilenameIfNecessary(newCloudInit *v1beta1.CloudInit) (admission.Patch, error) { var patch admission.Patch - // Not implemented, validator will fail request + + filename := ensureFileExtension(filepath.Base(newCloudInit.Spec.Filename)) + if filename == newCloudInit.Spec.Filename { + return patch, nil + } + + p := admission.PatchOp{ + Op: admission.PatchOpReplace, + Path: "/spec/filename", + Value: filename, + } + patch = append(patch, p) + return patch, nil } +func ensureFileExtension(s string) string { + accept := func(extension string) bool { + extensions := []string{".yaml", ".yml"} + for _, ext := range extensions { + if ext == extension { + return true + } + } + return false + } + if accept(filepath.Ext(s)) { + return s + } + return fmt.Sprintf("%s.yaml", s) +} + func (m *CloudInit) Resource() admission.Resource { return admission.Resource{ Names: []string{v1beta1.CloudInitResourceName}, diff --git a/pkg/mutator/cloudinit_test.go b/pkg/mutator/cloudinit_test.go new file mode 100644 index 00000000..28ebde33 --- /dev/null +++ b/pkg/mutator/cloudinit_test.go @@ -0,0 +1,114 @@ +package mutator + +import ( + "reflect" + "testing" + + "github.com/harvester/webhook/pkg/server/admission" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1" +) + +func TestCreate(t *testing.T) { + patchFilename := func(want string) admission.Patch { + return admission.Patch([]admission.PatchOp{ + {Op: admission.PatchOpReplace, Path: "/spec/filename", Value: want}, + }) + } + + var noPatch admission.Patch + + tests := []struct { + input string + want admission.Patch + }{ + {"/baseonly/a.yaml", patchFilename("a.yaml")}, + {"missing_suffix", patchFilename("missing_suffix.yaml")}, + {"/baseonly/andmissingsuffix", patchFilename("andmissingsuffix.yaml")}, + {"b.yml", noPatch}, + {"b.yaml", noPatch}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + m := NewCloudInitMutator() + cloudinit := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: v1beta1.CloudInitSpec{ + MatchSelector: map[string]string{}, + Filename: tt.input, + Contents: "hello, world", + }, + } + got, err := m.Create(new(admission.Request), cloudinit) + if err != nil { + t.Errorf("want err=, got err=%v", err) + } + + if !reflect.DeepEqual(tt.want, got) { + t.Errorf("want patch %+v, got patch %+v", tt.want, got) + } + }) + } +} + +func TestUpdate(t *testing.T) { + patchFilename := func(want string) admission.Patch { + return admission.Patch([]admission.PatchOp{ + {Op: admission.PatchOpReplace, Path: "/spec/filename", Value: want}, + }) + } + + var noPatch admission.Patch + + tests := []struct { + input string + want admission.Patch + }{ + {"/baseonly/a.yaml", patchFilename("a.yaml")}, + {"missing_suffix", patchFilename("missing_suffix.yaml")}, + {"/baseonly/andmissingsuffix", patchFilename("andmissingsuffix.yaml")}, + {"b.yml", noPatch}, + {"b.yaml", noPatch}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + m := NewCloudInitMutator() + + cloudinit := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: v1beta1.CloudInitSpec{ + MatchSelector: map[string]string{}, + Filename: tt.input, + Contents: "hello, world", + }, + } + + old := &v1beta1.CloudInit{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: v1beta1.CloudInitSpec{ + MatchSelector: map[string]string{}, + Filename: "specifically_not_in_use.yaml", + Contents: "hello, world", + }, + } + + got, err := m.Update(new(admission.Request), old, cloudinit) + if err != nil { + t.Errorf("want err=, got err=%v", err) + } + + if !reflect.DeepEqual(tt.want, got) { + t.Errorf("want patch %+v, got patch %+v", tt.want, got) + } + }) + } +}