From fc58124f8c6b56e1f680767b873ab0d66081d3c0 Mon Sep 17 00:00:00 2001 From: Mario Constanti Date: Mon, 18 Sep 2023 08:42:23 +0200 Subject: [PATCH] fix: reject pool creation if pool with spec exists in garm, a pool is uniq on imageName, flavor, provider and scope. This fixes the current implementation where we compared the image.tag name with the name of the image referenc itself. Signed-off-by: Mario Constanti --- Makefile | 2 +- README.md | 2 + api/v1alpha1/pool_types_test.go | 148 ++++++++++++++++++++++++++++++++ api/v1alpha1/pool_webhook.go | 23 +++-- go.mod | 2 +- 5 files changed, 163 insertions(+), 14 deletions(-) create mode 100644 api/v1alpha1/pool_types_test.go diff --git a/Makefile b/Makefile index 83adb85..b70bb18 100644 --- a/Makefile +++ b/Makefile @@ -197,7 +197,7 @@ $(GOLANGCI_LINT): $(LOCALBIN) .PHONY: mockgen mockgen: $(MOCKGEN) ## Download mockgen locally if necessary. If wrong version is installed, it will be overwritten. $(MOCKGEN): $(LOCALBIN) - test -s $(LOCALBIN)/mockgen && $(LOCALBIN)/mockget --version | grep -q $(MOCKGEN_VERSION) || \ + test -s $(LOCALBIN)/mockgen && $(LOCALBIN)/mockgen --version | grep -q $(MOCKGEN_VERSION) || \ GOBIN=$(LOCALBIN) go install go.uber.org/mock/mockgen@$(MOCKGEN_VERSION) .PHONY: goreleaser diff --git a/README.md b/README.md index 7722364..0582b91 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,7 @@ +[![Go Report Card](https://goreportcard.com/badge/github.com/mercedes-benz/garm-operator)](https://goreportcard.com/report/github.com/mercedes-benz/garm-operator) ![GitHub release (latest SemVer)](https://img.shields.io/github/v/release/mercedes-benz/garm-operator?sort=semver) + # garm-operator diff --git a/api/v1alpha1/pool_types_test.go b/api/v1alpha1/pool_types_test.go new file mode 100644 index 0000000..43addaa --- /dev/null +++ b/api/v1alpha1/pool_types_test.go @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: MIT + +package v1alpha1 + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +func TestPoolList_FilterByFields(t *testing.T) { + type fields struct { + TypeMeta metav1.TypeMeta + ListMeta metav1.ListMeta + Items []Pool + } + type args struct { + predicates []Predicate + } + tests := []struct { + name string + fields fields + args args + length int + }{ + { + name: "pool with spec already exist", + fields: fields{ + TypeMeta: metav1.TypeMeta{}, + ListMeta: metav1.ListMeta{}, + Items: []Pool{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ubuntu-2004-large", + Namespace: "test", + }, + Spec: PoolSpec{ + ImageName: "ubuntu-2004", + Flavor: "large", + ProviderName: "openstack", + GitHubScopeRef: corev1.TypedLocalObjectReference{ + Name: "test", + Kind: "Enterprise", + APIGroup: ptr.To[string]("github.com"), + }, + }, + }, + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ubuntu-2204-large", + Namespace: "test", + }, + Spec: PoolSpec{ + ImageName: "ubuntu-2204", + Flavor: "large", + ProviderName: "openstack", + GitHubScopeRef: corev1.TypedLocalObjectReference{ + Name: "test", + Kind: "Enterprise", + APIGroup: ptr.To[string]("github.com"), + }, + }, + }, + }, + }, + args: args{ + predicates: []Predicate{ + MatchesImage("ubuntu-2204"), + MatchesFlavour("large"), + MatchesProvider("openstack"), + MatchesGitHubScope("test", "Enterprise"), + }, + }, + length: 1, + }, + { + name: "pool with spec does not exist", + fields: fields{ + TypeMeta: metav1.TypeMeta{}, + ListMeta: metav1.ListMeta{}, + Items: []Pool{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ubuntu-2004-large", + Namespace: "test", + }, + Spec: PoolSpec{ + ImageName: "ubuntu-2004", + Flavor: "large", + ProviderName: "openstack", + GitHubScopeRef: corev1.TypedLocalObjectReference{ + Name: "test", + Kind: "Enterprise", + APIGroup: ptr.To[string]("github.com"), + }, + }, + }, + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ubuntu-2204-large", + Namespace: "test", + }, + Spec: PoolSpec{ + ImageName: "ubuntu-2204", + Flavor: "large", + ProviderName: "openstack", + GitHubScopeRef: corev1.TypedLocalObjectReference{ + Name: "test", + Kind: "Enterprise", + APIGroup: ptr.To[string]("github.com"), + }, + }, + }, + }, + }, + args: args{ + predicates: []Predicate{ + MatchesImage("ubuntu-2404"), + MatchesFlavour("large"), + MatchesProvider("openstack"), + MatchesGitHubScope("test", "Enterprise"), + }, + }, + length: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &PoolList{ + TypeMeta: tt.fields.TypeMeta, + ListMeta: tt.fields.ListMeta, + Items: tt.fields.Items, + } + + p.FilterByFields(tt.args.predicates...) + + if len(p.Items) != tt.length { + t.Errorf("FilterByFields() = %v, want %v", len(p.Items), tt.length) + } + }) + } +} diff --git a/api/v1alpha1/pool_webhook.go b/api/v1alpha1/pool_webhook.go index ab04d18..19fe1df 100644 --- a/api/v1alpha1/pool_webhook.go +++ b/api/v1alpha1/pool_webhook.go @@ -40,13 +40,12 @@ var _ webhook.Validator = &Pool{} func (r *Pool) ValidateCreate() (admission.Warnings, error) { poollog.Info("validate create", "name", r.Name) ctx := context.TODO() - pool := r - image, err := validateImageName(ctx, pool) + err := validateImageName(ctx, r) if err != nil { return nil, apierrors.NewInvalid( schema.GroupKind{Group: GroupVersion.Group, Kind: "Pool"}, - pool.Name, + r.Name, field.ErrorList{err}, ) } @@ -59,7 +58,7 @@ func (r *Pool) ValidateCreate() (admission.Warnings, error) { } listOpts := &client.ListOptions{ - Namespace: pool.Namespace, + Namespace: r.Namespace, } poolList := &PoolList{} @@ -69,10 +68,10 @@ func (r *Pool) ValidateCreate() (admission.Warnings, error) { } poolList.FilterByFields( - MatchesFlavour(pool.Spec.Flavor), - MatchesImage(image.Spec.Tag), - MatchesProvider(pool.Spec.ProviderName), - MatchesGitHubScope(pool.Spec.GitHubScopeRef.Name, pool.Spec.GitHubScopeRef.Kind), + MatchesFlavour(r.Spec.Flavor), + MatchesImage(r.Spec.ImageName), + MatchesProvider(r.Spec.ProviderName), + MatchesGitHubScope(r.Spec.GitHubScopeRef.Name, r.Spec.GitHubScopeRef.Kind), ) if len(poolList.Items) > 0 { @@ -93,7 +92,7 @@ func (r *Pool) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { return nil, apierrors.NewBadRequest("failed to convert runtime.Object to Pool CRD") } - _, err := validateImageName(context.Background(), r) + err := validateImageName(context.Background(), r) if err != nil { return nil, apierrors.NewInvalid( schema.GroupKind{Group: GroupVersion.Group, Kind: "Pool"}, @@ -128,17 +127,17 @@ func (r *Pool) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { return nil, nil } -func validateImageName(ctx context.Context, r *Pool) (*Image, *field.Error) { +func validateImageName(ctx context.Context, r *Pool) *field.Error { image := Image{} err := c.Get(ctx, client.ObjectKey{Name: r.Spec.ImageName, Namespace: r.Namespace}, &image) if err != nil { - return nil, field.Invalid( + return field.Invalid( field.NewPath("spec").Child("imageName"), r.Spec.ImageName, err.Error(), ) } - return &image, nil + return nil } func (r *Pool) validateProviderName(old *Pool) *field.Error { diff --git a/go.mod b/go.mod index f05fd7b..a23a98b 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( k8s.io/apimachinery v0.27.2 k8s.io/client-go v0.27.2 k8s.io/klog/v2 v2.90.1 + k8s.io/utils v0.0.0-20230726121419-3b25d923346b sigs.k8s.io/controller-runtime v0.15.0 ) @@ -82,7 +83,6 @@ require ( k8s.io/apiextensions-apiserver v0.27.2 // indirect k8s.io/component-base v0.27.2 // indirect k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f // indirect - k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect sigs.k8s.io/yaml v1.3.0 // indirect