diff --git a/api/v1alpha1/linodemachine_webhook.go b/api/v1alpha1/linodemachine_webhook.go index 281f40497..42d6933b9 100644 --- a/api/v1alpha1/linodemachine_webhook.go +++ b/api/v1alpha1/linodemachine_webhook.go @@ -32,12 +32,24 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + . "github.com/linode/cluster-api-provider-linode/clients" ) var ( - // The list of valid SCSI device paths that user-defined data disks may attach to - // NOTE: sda is reserved for the OS disk - LinodeMachineSCSIPaths = []string{"sdb", "sdc", "sdd", "sde", "sdh"} + // The list of valid device slots that data device disks may attach to + // NOTE: sda is reserved for the OS device disk + LinodeMachineDevicePaths = []string{"sdb", "sdc", "sdd", "sde", "sdf", "sdg", "sdh"} + + // The maximum number of device disks allowed per Configuration Profile per + // Linode’s Instance + // See: https://www.linode.com/docs/api/linode-instances/#configuration-profile-view + LinodeMachineMaxDisk = 8 + + // The maximum number of data device disks allowed in a Linode’s Instance's + // configuration profile + // NOTE: The first device disk is reserved for the OS disk + LinodeMachineMaxDataDisk = LinodeMachineMaxDisk - 1 ) // log is for logging in this package. @@ -62,7 +74,7 @@ func (r *LinodeMachine) ValidateCreate() (admission.Warnings, error) { linodemachinelog.Info("validate create", "name", r.Name) client := linodego.NewClient(http.DefaultClient) - return nil, r.validateLinodeMachine(context.TODO(), client) + return nil, r.validateLinodeMachine(context.TODO(), &client) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type @@ -81,7 +93,7 @@ func (r *LinodeMachine) ValidateDelete() (admission.Warnings, error) { return nil, nil } -func (r *LinodeMachine) validateLinodeMachine(ctx context.Context, client linodego.Client) error { +func (r *LinodeMachine) validateLinodeMachine(ctx context.Context, client LinodeClient) error { var errs field.ErrorList if err := r.validateLinodeMachineSpec(ctx, client); err != nil { errs = slices.Concat(errs, err) @@ -95,7 +107,7 @@ func (r *LinodeMachine) validateLinodeMachine(ctx context.Context, client linode r.Name, errs) } -func (r *LinodeMachine) validateLinodeMachineSpec(ctx context.Context, client linodego.Client) field.ErrorList { +func (r *LinodeMachine) validateLinodeMachineSpec(ctx context.Context, client LinodeClient) field.ErrorList { var ( errs field.ErrorList ) @@ -148,18 +160,23 @@ func validateDataDisks(disks map[string]*InstanceDisk, path *field.Path, availSi ) for dev, disk := range disks { - if !slices.Contains(LinodeMachineSCSIPaths, dev) { - return nil, field.Forbidden(path.Child(dev), fmt.Sprintf("allowed device paths: %v", LinodeMachineSCSIPaths)) + if !slices.Contains(LinodeMachineDevicePaths, dev) { + return nil, field.Forbidden(path.Child(dev), fmt.Sprintf("allowed device paths: %v", LinodeMachineDevicePaths)) } if slices.Contains(devs, dev) { return nil, field.Duplicate(path.Child(dev), "duplicate device path") } + devs = append(devs, dev) + if len(devs) > LinodeMachineMaxDataDisk { + return nil, field.TooMany(path, len(devs), LinodeMachineMaxDataDisk) + } var err *field.Error if remainSize, err = validateDisk(disk, path.Child(dev), remainSize, planSize); err != nil { return nil, err } + } return remainSize, nil } diff --git a/api/v1alpha1/linodemachine_webhook_test.go b/api/v1alpha1/linodemachine_webhook_test.go index 024267f80..29d18e779 100644 --- a/api/v1alpha1/linodemachine_webhook_test.go +++ b/api/v1alpha1/linodemachine_webhook_test.go @@ -17,23 +17,120 @@ limitations under the License. package v1alpha1 import ( - . "github.com/onsi/ginkgo/v2" -) - -var _ = Describe("LinodeMachine Webhook", func() { - - Context("When creating LinodeMachine under Validating Webhook", func() { - It("Should deny if a required field is empty", func() { + "context" + "errors" + "math" + "testing" - // TODO(user): Add your logic here + "github.com/linode/linodego" + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - }) + "github.com/linode/cluster-api-provider-linode/mock" - It("Should admit if all required fields are provided", func() { - - // TODO(user): Add your logic here + . "github.com/linode/cluster-api-provider-linode/mock/mocktest" +) - }) - }) +//nolint:paralleltest // race detected during execution of test +func TestValidateLinodeMachine(t *testing.T) { + var ( + machine = LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: "example", + }, + Spec: LinodeMachineSpec{ + Region: "example", + Type: "example", + }, + } + plan = linodego.LinodeType{Disk: math.MaxInt} + plan_zero = linodego.LinodeType{Disk: 0} + disk = InstanceDisk{Size: resource.MustParse("1G")} + disk_zero = InstanceDisk{Size: *resource.NewQuantity(0, resource.BinarySI)} + ) -}) + NewSuite(t, mock.MockLinodeClient{}).Run( + OneOf( + Path( + Call("valid", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan, nil).AnyTimes() + }), + Result("success", func(ctx context.Context, mck Mock) { + assert.NoError(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + Call("valid with disks", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan, nil).AnyTimes() + }), + Result("success", func(ctx context.Context, mck Mock) { + machine := machine + machine.Spec.OSDisk = &disk + assert.NoError(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + ), + ), + OneOf( + Path(Call("invalid region", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, errors.New("invalid region")).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + })), + Path(Call("invalid type", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(nil, errors.New("invalid type")).AnyTimes() + })), + ), + Result("error", func(ctx context.Context, mck Mock) { + assert.Error(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + OneOf( + Path( + Call("exceed plan storage", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan_zero, nil).AnyTimes() + }), + Result("os disk too large", func(ctx context.Context, mck Mock) { + machine := machine + machine.Spec.OSDisk = &disk + assert.Error(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + ), + Path( + Call("exceed plan storage", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan_zero, nil).AnyTimes() + }), + Result("data disk too large", func(ctx context.Context, mck Mock) { + machine := machine + machine.Spec.DataDisks = map[string]*InstanceDisk{"sdb": &disk} + assert.Error(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + ), + Path( + Call("data disk invalid path", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan, nil).AnyTimes() + }), + Result("error", func(ctx context.Context, mck Mock) { + machine := machine + machine.Spec.DataDisks = map[string]*InstanceDisk{"sda": &disk} + assert.Error(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + ), + Path( + Call("invalid disk size", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mck.LinodeClient.EXPECT().GetType(gomock.Any(), gomock.Any()).Return(&plan, nil).AnyTimes() + }), + Result("error", func(ctx context.Context, mck Mock) { + machine := machine + machine.Spec.OSDisk = &disk_zero + assert.Error(t, machine.validateLinodeMachine(ctx, mck.LinodeClient)) + }), + ), + ), + ) +} diff --git a/api/v1alpha1/webhook_helpers.go b/api/v1alpha1/webhook_helpers.go index 85fa58a73..236c24400 100644 --- a/api/v1alpha1/webhook_helpers.go +++ b/api/v1alpha1/webhook_helpers.go @@ -5,9 +5,11 @@ import ( "github.com/linode/linodego" "k8s.io/apimachinery/pkg/util/validation/field" + + . "github.com/linode/cluster-api-provider-linode/clients" ) -func validateRegion(ctx context.Context, client linodego.Client, id string, path *field.Path) (*linodego.Region, *field.Error) { +func validateRegion(ctx context.Context, client LinodeClient, id string, path *field.Path) (*linodego.Region, *field.Error) { region, err := client.GetRegion(ctx, id) if err != nil { return nil, field.NotFound(path, id) @@ -16,7 +18,7 @@ func validateRegion(ctx context.Context, client linodego.Client, id string, path return region, nil } -func validateLinodeType(ctx context.Context, client linodego.Client, id string, path *field.Path) (*linodego.LinodeType, *field.Error) { +func validateLinodeType(ctx context.Context, client LinodeClient, id string, path *field.Path) (*linodego.LinodeType, *field.Error) { plan, err := client.GetType(ctx, id) if err != nil { return nil, field.NotFound(path, id) diff --git a/clients/clients.go b/clients/clients.go index 779a9a35c..78519990f 100644 --- a/clients/clients.go +++ b/clients/clients.go @@ -33,6 +33,7 @@ type LinodeInstanceClient interface { GetImage(ctx context.Context, imageID string) (*linodego.Image, error) CreateStackscript(ctx context.Context, opts linodego.StackscriptCreateOptions) (*linodego.Stackscript, error) ListStackscripts(ctx context.Context, opts *linodego.ListOptions) ([]linodego.Stackscript, error) + GetType(ctx context.Context, typeID string) (*linodego.LinodeType, error) } // LinodeVPCClient defines the methods that a Linode client must have to interact with Linode's VPC service. diff --git a/mock/client.go b/mock/client.go index b6fbf8b7c..b891b8de0 100644 --- a/mock/client.go +++ b/mock/client.go @@ -294,6 +294,21 @@ func (mr *MockLinodeClientMockRecorder) GetRegion(ctx, regionID any) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRegion", reflect.TypeOf((*MockLinodeClient)(nil).GetRegion), ctx, regionID) } +// GetType mocks base method. +func (m *MockLinodeClient) GetType(ctx context.Context, typeID string) (*linodego.LinodeType, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetType", ctx, typeID) + ret0, _ := ret[0].(*linodego.LinodeType) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetType indicates an expected call of GetType. +func (mr *MockLinodeClientMockRecorder) GetType(ctx, typeID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetType", reflect.TypeOf((*MockLinodeClient)(nil).GetType), ctx, typeID) +} + // GetVPC mocks base method. func (m *MockLinodeClient) GetVPC(ctx context.Context, vpcID int) (*linodego.VPC, error) { m.ctrl.T.Helper() @@ -584,6 +599,21 @@ func (mr *MockLinodeInstanceClientMockRecorder) GetRegion(ctx, regionID any) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRegion", reflect.TypeOf((*MockLinodeInstanceClient)(nil).GetRegion), ctx, regionID) } +// GetType mocks base method. +func (m *MockLinodeInstanceClient) GetType(ctx context.Context, typeID string) (*linodego.LinodeType, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetType", ctx, typeID) + ret0, _ := ret[0].(*linodego.LinodeType) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetType indicates an expected call of GetType. +func (mr *MockLinodeInstanceClientMockRecorder) GetType(ctx, typeID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetType", reflect.TypeOf((*MockLinodeInstanceClient)(nil).GetType), ctx, typeID) +} + // ListInstanceConfigs mocks base method. func (m *MockLinodeInstanceClient) ListInstanceConfigs(ctx context.Context, linodeID int, opts *linodego.ListOptions) ([]linodego.InstanceConfig, error) { m.ctrl.T.Helper()