From a8a31b9d65f9a712e7718960ebe3109da42fda10 Mon Sep 17 00:00:00 2001 From: Ivan Mikheykin Date: Fri, 3 Jul 2026 20:45:54 +0300 Subject: [PATCH] fix(vm): make virtualMachineCIDRs optional Allow ModuleConfig without spec.settings.virtualMachineCIDRs. What changed: - Make virtualMachineCIDRs optional in OpenAPI schema. - Stop blocking module enablement when CIDRs are not configured. - Make ModuleConfig CIDR parsing safe when the field is absent. - Skip CIDR overlap checks when virtualMachineCIDRs is not set. - New behavior when CIDRs are not configured: - Reject VMs with explicit Main network. - Reject VMIP usage and VirtualMachineIPAddress resources. - Validate changing ModuleConfig: protect removing CIDRs if VMIP leases are present. Also updated docs and tests. Signed-off-by: Ivan Mikheykin --- .../pkg/hooks/validate-module-config/hook.go | 7 +- .../hooks/validate-module-config/hook_test.go | 31 ++++++- images/hooks/pkg/settings/module_config.go | 4 - .../hooks/pkg/settings/module_config_test.go | 18 ++++ .../pkg/controller/moduleconfig/checks.go | 17 +++- .../controller/moduleconfig/checks_test.go | 41 +++++++++ .../moduleconfig/cidrs_validator.go | 3 + .../controller/moduleconfig/config_state.go | 31 +++++++ .../moduleconfig/validators_test.go | 73 +++++++++++++++ .../vm/internal/validators/ipam_validator.go | 29 +++++- .../validators/ipam_validator_test.go | 65 ++++++++++++++ .../internal/validators/networks_validator.go | 27 +++++- .../validators/networks_validator_test.go | 89 ++++++++++++++++++- .../pkg/controller/vmip/vmip_webhook.go | 26 +++++- .../pkg/controller/vmip/vmip_webhook_test.go | 44 +++++++++ openapi/config-values.yaml | 9 +- openapi/doc-ru-config-values.yaml | 7 +- openapi/openapi-case-tests.yaml | 4 + 18 files changed, 501 insertions(+), 24 deletions(-) create mode 100644 images/virtualization-artifact/pkg/controller/moduleconfig/checks_test.go create mode 100644 images/virtualization-artifact/pkg/controller/moduleconfig/config_state.go create mode 100644 images/virtualization-artifact/pkg/controller/moduleconfig/validators_test.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator_test.go create mode 100644 images/virtualization-artifact/pkg/controller/vmip/vmip_webhook_test.go diff --git a/images/hooks/pkg/hooks/validate-module-config/hook.go b/images/hooks/pkg/hooks/validate-module-config/hook.go index c6fbf9dee2..3bdd6b73c0 100644 --- a/images/hooks/pkg/hooks/validate-module-config/hook.go +++ b/images/hooks/pkg/hooks/validate-module-config/hook.go @@ -121,6 +121,9 @@ func validateModuleConfigSettings(mc *mcapi.ModuleConfig, nodes []corev1.Node, p if err != nil { return err } + if len(CIDRs) == 0 { + return nil + } err = moduleconfig.CheckCIDRsOverlap(CIDRs) if err != nil { @@ -161,10 +164,6 @@ func validateRequiredModuleConfigSettings(mc *mcapi.ModuleConfig) error { return fmt.Errorf("spec.settings is empty") } - if _, ok := mc.Spec.Settings["virtualMachineCIDRs"]; !ok { - return fmt.Errorf("spec.settings.virtualMachineCIDRs is required") - } - if _, ok := mc.Spec.Settings["dvcr"]; !ok { return fmt.Errorf("spec.settings.dvcr is required") } diff --git a/images/hooks/pkg/hooks/validate-module-config/hook_test.go b/images/hooks/pkg/hooks/validate-module-config/hook_test.go index fa950ea89d..4dbca6c152 100644 --- a/images/hooks/pkg/hooks/validate-module-config/hook_test.go +++ b/images/hooks/pkg/hooks/validate-module-config/hook_test.go @@ -222,12 +222,41 @@ var _ = Describe("ModuleConfig validation hook", func() { Expect(validationErrorSet).To(BeTrue(), "should set validation error") }, Entry("settings are nil", nil, "spec.settings is empty"), - Entry("settings are empty", map[string]any{}, "spec.settings.virtualMachineCIDRs is required"), + Entry("settings are empty", map[string]any{}, "spec.settings.dvcr is required"), Entry("dvcr is missing", map[string]any{ "virtualMachineCIDRs": validVirtualMachineCIDRs, }, "spec.settings.dvcr is required"), ) + It("Should allow module config without virtualMachineCIDRs", func() { + settingsWithoutCIDRs := map[string]any{ + "dvcr": validSettings["dvcr"], + } + prepareValues(defaultPodSubnet, defaultServiceSubnet) + prepareConfigValues(settingsWithoutCIDRs) + prepareSnapshots( + newModuleConfigSnapshot(settingsWithoutCIDRs), + newNodesSnapshot(defaultNodeAddresses), + ) + + values.SetMock.Set(func(path string, v any) { + switch path { + case settings.InternalValuesConfigCopyPath: + Expect(v).ToNot(HaveKey("virtualMachineCIDRs")) + Expect(v).To(HaveKey("dvcr")) + case settings.InternalValuesConfigValidationPath: + Fail("Should not set validation error") + default: + Fail("unexpected path") + } + }) + values.RemoveMock.Set(func(path string) { + Expect(path).To(Equal(settings.InternalValuesConfigValidationPath)) + }) + + Expect(Reconcile(context.Background(), newInput())).To(Succeed()) + }) + DescribeTable("Should set validation error if virtualMachineCIDRs overlap with other addresses in cluster", func(cidrs []any) { prepareValues(defaultPodSubnet, defaultServiceSubnet) invalidSettings := map[string]any{ diff --git a/images/hooks/pkg/settings/module_config.go b/images/hooks/pkg/settings/module_config.go index 35316f4f82..66bfaed640 100644 --- a/images/hooks/pkg/settings/module_config.go +++ b/images/hooks/pkg/settings/module_config.go @@ -55,10 +55,6 @@ func CanRunWithModuleConfig(ctx context.Context, input *pkg.HookInput) (bool, er return false, nil } - if _, ok := moduleConfig.Spec.Settings["virtualMachineCIDRs"]; !ok { - return false, nil - } - if _, ok := moduleConfig.Spec.Settings["dvcr"]; !ok { return false, nil } diff --git a/images/hooks/pkg/settings/module_config_test.go b/images/hooks/pkg/settings/module_config_test.go index 782155401b..faaab5c61a 100644 --- a/images/hooks/pkg/settings/module_config_test.go +++ b/images/hooks/pkg/settings/module_config_test.go @@ -116,6 +116,24 @@ func TestCanRunWithModuleConfig(t *testing.T) { }) t.Run("returns true when required settings exist", func(t *testing.T) { + input := newInput(&fakeKubernetesClient{get: func(ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object) error { + mc := obj.(*mcapi.ModuleConfig) + *mc = *NewModuleConfigForTest(map[string]any{ + "dvcr": map[string]any{}, + }) + return nil + }}, nil) + + ok, err := CanRunWithModuleConfig(context.Background(), input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatalf("expected CanRunWithModuleConfig to return true") + } + }) + + t.Run("returns true when required settings and cidrs exist", func(t *testing.T) { input := newInput(&fakeKubernetesClient{get: func(ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object) error { mc := obj.(*mcapi.ModuleConfig) *mc = *NewModuleConfigForTest(map[string]any{ diff --git a/images/virtualization-artifact/pkg/controller/moduleconfig/checks.go b/images/virtualization-artifact/pkg/controller/moduleconfig/checks.go index e9a52aa5a2..d429f1f958 100644 --- a/images/virtualization-artifact/pkg/controller/moduleconfig/checks.go +++ b/images/virtualization-artifact/pkg/controller/moduleconfig/checks.go @@ -97,8 +97,21 @@ func CheckCIDRsOverlapWithServiceSubnet(cidrs []netip.Prefix, subnet netip.Prefi } func ParseCIDRs(settings mcapi.SettingsValues) ([]netip.Prefix, error) { - raw := settings[virtualMachineCIDRs].([]interface{}) - CIDRs, err := convertToStringSlice(raw) + if settings == nil { + return nil, nil + } + + raw, ok := settings[virtualMachineCIDRs] + if !ok || raw == nil { + return nil, nil + } + + rawCIDRs, ok := raw.([]interface{}) + if !ok { + return nil, fmt.Errorf("failed to convert %q to []interface{}", virtualMachineCIDRs) + } + + CIDRs, err := convertToStringSlice(rawCIDRs) if err != nil { return nil, err } diff --git a/images/virtualization-artifact/pkg/controller/moduleconfig/checks_test.go b/images/virtualization-artifact/pkg/controller/moduleconfig/checks_test.go new file mode 100644 index 0000000000..cbf9ab6d53 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/moduleconfig/checks_test.go @@ -0,0 +1,41 @@ +package moduleconfig + +import ( + "testing" + + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" +) + +func TestParseCIDRs(t *testing.T) { + tests := []struct { + name string + settings mcapi.SettingsValues + wantLen int + wantErr bool + }{ + {name: "nil settings", settings: nil, wantLen: 0}, + {name: "missing key", settings: mcapi.SettingsValues{"dvcr": map[string]any{}}, wantLen: 0}, + {name: "nil value", settings: mcapi.SettingsValues{"virtualMachineCIDRs": nil}, wantLen: 0}, + {name: "invalid type", settings: mcapi.SettingsValues{"virtualMachineCIDRs": []string{"10.0.0.0/24"}}, wantErr: true}, + {name: "invalid cidr", settings: mcapi.SettingsValues{"virtualMachineCIDRs": []any{"bad"}}, wantErr: true}, + {name: "valid cidrs", settings: mcapi.SettingsValues{"virtualMachineCIDRs": []any{"10.0.0.0/24", "10.0.1.0/24"}}, wantLen: 2}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cidrs, err := ParseCIDRs(tt.settings) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cidrs) != tt.wantLen { + t.Fatalf("expected %d cidrs, got %d", tt.wantLen, len(cidrs)) + } + }) + } +} diff --git a/images/virtualization-artifact/pkg/controller/moduleconfig/cidrs_validator.go b/images/virtualization-artifact/pkg/controller/moduleconfig/cidrs_validator.go index a97fc7cff4..300e12b234 100644 --- a/images/virtualization-artifact/pkg/controller/moduleconfig/cidrs_validator.go +++ b/images/virtualization-artifact/pkg/controller/moduleconfig/cidrs_validator.go @@ -46,6 +46,9 @@ func (v cidrsValidator) ValidateUpdate(ctx context.Context, _, newMC *mcapi.Modu if err != nil { return admission.Warnings{}, err } + if len(cidrs) == 0 { + return admission.Warnings{}, nil + } err = CheckCIDRsOverlap(cidrs) if err != nil { diff --git a/images/virtualization-artifact/pkg/controller/moduleconfig/config_state.go b/images/virtualization-artifact/pkg/controller/moduleconfig/config_state.go new file mode 100644 index 0000000000..bce1ad9071 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/moduleconfig/config_state.go @@ -0,0 +1,31 @@ +package moduleconfig + +import ( + "context" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" + + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" +) + +func HasVirtualMachineCIDRs(settings mcapi.SettingsValues) (bool, error) { + cidrs, err := ParseCIDRs(settings) + if err != nil { + return false, err + } + return len(cidrs) > 0, nil +} + +func ModuleConfigHasVirtualMachineCIDRs(ctx context.Context, c client.Client) (bool, error) { + if c == nil { + return false, fmt.Errorf("kubernetes client is nil") + } + + var moduleConfig mcapi.ModuleConfig + if err := c.Get(ctx, client.ObjectKey{Name: moduleConfigName}, &moduleConfig); err != nil { + return false, fmt.Errorf("get ModuleConfig/%s: %w", moduleConfigName, err) + } + + return HasVirtualMachineCIDRs(moduleConfig.Spec.Settings) +} diff --git a/images/virtualization-artifact/pkg/controller/moduleconfig/validators_test.go b/images/virtualization-artifact/pkg/controller/moduleconfig/validators_test.go new file mode 100644 index 0000000000..a0974059e7 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/moduleconfig/validators_test.go @@ -0,0 +1,73 @@ +package moduleconfig + +import ( + "context" + "net/netip" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + appconfig "github.com/deckhouse/virtualization-controller/pkg/config" + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +func TestCIDRsValidatorValidateUpdate(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + podSubnet := netip.MustParsePrefix("10.111.0.0/16") + serviceSubnet := netip.MustParsePrefix("10.222.0.0/16") + + validator := newCIDRsValidator(fake.NewClientBuilder().WithScheme(scheme).Build(), &appconfig.ClusterSubnets{ + PodSubnet: podSubnet, + ServiceSubnet: serviceSubnet, + }) + + newMC := &mcapi.ModuleConfig{Spec: mcapi.ModuleConfigSpec{Settings: mcapi.SettingsValues{"dvcr": map[string]any{}}}} + if _, err := validator.ValidateUpdate(context.Background(), nil, newMC); err != nil { + t.Fatalf("expected no error without CIDRs, got: %v", err) + } + + newMC.Spec.Settings["virtualMachineCIDRs"] = []any{"10.111.0.0/24", "10.111.0.0/25"} + if _, err := validator.ValidateUpdate(context.Background(), nil, newMC); err == nil { + t.Fatalf("expected overlap validation error") + } +} + +func TestRemoveCIDRsValidatorValidateUpdate(t *testing.T) { + scheme := runtime.NewScheme() + if err := v1alpha2.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + oldNone := &mcapi.ModuleConfig{Spec: mcapi.ModuleConfigSpec{Settings: mcapi.SettingsValues{"dvcr": map[string]any{}}}} + newNone := &mcapi.ModuleConfig{Spec: mcapi.ModuleConfigSpec{Settings: mcapi.SettingsValues{"dvcr": map[string]any{}}}} + newSome := &mcapi.ModuleConfig{Spec: mcapi.ModuleConfigSpec{Settings: mcapi.SettingsValues{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}}} + oldSome := &mcapi.ModuleConfig{Spec: mcapi.ModuleConfigSpec{Settings: mcapi.SettingsValues{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}}} + + validator := newRemoveCIDRsValidator(fake.NewClientBuilder().WithScheme(scheme).Build()) + for name, pair := range map[string]struct{ oldMC, newMC *mcapi.ModuleConfig }{ + "old none new none": {oldNone, newNone}, + "old none new some": {oldNone, newSome}, + "old some new none without leases": {oldSome, newNone}, + "old some new some same": {oldSome, newSome}, + } { + t.Run(name, func(t *testing.T) { + if _, err := validator.ValidateUpdate(context.Background(), pair.oldMC, pair.newMC); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } + + lease := &v1alpha2.VirtualMachineIPAddressLease{ObjectMeta: metav1.ObjectMeta{Name: "ip-10-0-0-10"}} + validator = newRemoveCIDRsValidator(fake.NewClientBuilder().WithScheme(scheme).WithObjects(lease).Build()) + if _, err := validator.ValidateUpdate(context.Background(), oldSome, newNone); err == nil { + t.Fatalf("expected lease protection error") + } +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go index 6520fa77a7..0befcf8715 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go @@ -26,6 +26,7 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/common/network" "github.com/deckhouse/virtualization-controller/pkg/common/object" + "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -38,8 +39,8 @@ func NewIPAMValidator(client client.Client) *IPAMValidator { } func (v *IPAMValidator) ValidateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { - if vm.Spec.VirtualMachineIPAddress != "" && len(vm.Spec.Networks) > 0 && !network.HasMainNetworkSpec(vm.Spec.Networks) { - return nil, fmt.Errorf("spec.virtualMachineIPAddressName cannot be set without Main network type in spec.networks") + if err := v.validateVMIPUsage(ctx, vm); err != nil { + return nil, err } vmipName := vm.Spec.VirtualMachineIPAddress @@ -67,8 +68,8 @@ func (v *IPAMValidator) ValidateCreate(ctx context.Context, vm *v1alpha2.Virtual } func (v *IPAMValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { - if newVM.Spec.VirtualMachineIPAddress != "" && len(newVM.Spec.Networks) > 0 && !network.HasMainNetworkSpec(newVM.Spec.Networks) { - return nil, fmt.Errorf("spec.virtualMachineIPAddressName cannot be set without Main network type in spec.networks") + if err := v.validateVMIPUsage(ctx, newVM); err != nil { + return nil, err } if oldVM.Spec.VirtualMachineIPAddress == newVM.Spec.VirtualMachineIPAddress { @@ -91,3 +92,23 @@ func (v *IPAMValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alph return nil, nil } + +func (v *IPAMValidator) validateVMIPUsage(ctx context.Context, vm *v1alpha2.VirtualMachine) error { + if vm.Spec.VirtualMachineIPAddress == "" { + return nil + } + + hasCIDRs, err := moduleconfig.ModuleConfigHasVirtualMachineCIDRs(ctx, v.client) + if err != nil { + return fmt.Errorf("unable to check ModuleConfig virtualization virtualMachineCIDRs: %w", err) + } + if !hasCIDRs { + return fmt.Errorf("spec.virtualMachineIPAddressName cannot be set when ModuleConfig/virtualization spec.settings.virtualMachineCIDRs is not configured") + } + + if len(vm.Spec.Networks) > 0 && !network.HasMainNetworkSpec(vm.Spec.Networks) { + return fmt.Errorf("spec.virtualMachineIPAddressName cannot be set without Main network type in spec.networks") + } + + return nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator_test.go new file mode 100644 index 0000000000..7c408a0375 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator_test.go @@ -0,0 +1,65 @@ +package validators + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +func TestIPAMValidatorValidateCreate(t *testing.T) { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme moduleconfig: %v", err) + } + if err := v1alpha2.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme v1alpha2: %v", err) + } + + newValidator := func(settings map[string]any) *IPAMValidator { + return NewIPAMValidator(fake.NewClientBuilder().WithScheme(scheme).WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: settings}, + }).Build()) + } + + vm := &v1alpha2.VirtualMachine{Spec: v1alpha2.VirtualMachineSpec{VirtualMachineIPAddress: "vmip", Networks: []v1alpha2.NetworksSpec{{Type: v1alpha2.NetworksTypeMain}}}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}}).ValidateCreate(t.Context(), vm); err == nil { + t.Fatalf("expected error without CIDRs") + } + + vm.Spec.Networks = []v1alpha2.NetworksSpec{{Type: v1alpha2.NetworksTypeNetwork, Name: "test"}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}).ValidateCreate(t.Context(), vm); err == nil { + t.Fatalf("expected error without Main network") + } + + vm.Spec.Networks = []v1alpha2.NetworksSpec{{Type: v1alpha2.NetworksTypeMain}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}).ValidateCreate(t.Context(), vm); err != nil { + t.Fatalf("expected success, got: %v", err) + } +} + +func TestIPAMValidatorValidateUpdate(t *testing.T) { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme moduleconfig: %v", err) + } + if err := v1alpha2.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme v1alpha2: %v", err) + } + + validator := NewIPAMValidator(fake.NewClientBuilder().WithScheme(scheme).WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: map[string]any{"dvcr": map[string]any{}}}, + }).Build()) + + oldVM := &v1alpha2.VirtualMachine{} + newVM := &v1alpha2.VirtualMachine{Spec: v1alpha2.VirtualMachineSpec{VirtualMachineIPAddress: "vmip", Networks: []v1alpha2.NetworksSpec{{Type: v1alpha2.NetworksTypeMain}}}} + if _, err := validator.ValidateUpdate(t.Context(), oldVM, newVM); err == nil { + t.Fatalf("expected error without CIDRs") + } +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator.go index 5f17e821d0..b4e720abc4 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" commonnetwork "github.com/deckhouse/virtualization-controller/pkg/common/network" + "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig" "github.com/deckhouse/virtualization-controller/pkg/featuregates" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -46,12 +47,16 @@ func NewNetworksValidator(c client.Client, featureGate featuregate.FeatureGate) } } -func (v *NetworksValidator) ValidateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { +func (v *NetworksValidator) ValidateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { networksSpec := vm.Spec.Networks if len(networksSpec) == 0 { return nil, nil } + if err := v.validateMainNetworkAllowed(ctx, networksSpec); err != nil { + return nil, err + } + if !isSingleMainNet(networksSpec) && !v.featureGate.Enabled(featuregates.SDN) { return nil, fmt.Errorf("network configuration requires SDN to be enabled") } @@ -65,6 +70,10 @@ func (v *NetworksValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1 return nil, nil } + if err := v.validateMainNetworkAllowed(ctx, newNetworksSpec); err != nil { + return nil, err + } + if !isSingleMainNet(newNetworksSpec) && !v.featureGate.Enabled(featuregates.SDN) { return nil, fmt.Errorf("network configuration requires SDN to be enabled") } @@ -271,3 +280,19 @@ func (v *NetworksValidator) getNetworkIdentifier(network v1alpha2.NetworksSpec) } return fmt.Sprintf("%s/%s", network.Type, network.Name) } + +func (v *NetworksValidator) validateMainNetworkAllowed(ctx context.Context, networksSpec []v1alpha2.NetworksSpec) error { + if !commonnetwork.HasMainNetworkSpec(networksSpec) { + return nil + } + + hasCIDRs, err := moduleconfig.ModuleConfigHasVirtualMachineCIDRs(ctx, v.client) + if err != nil { + return fmt.Errorf("unable to check ModuleConfig virtualization virtualMachineCIDRs: %w", err) + } + if !hasCIDRs { + return fmt.Errorf("spec.networks cannot explicitly include Main network type when ModuleConfig/virtualization spec.settings.virtualMachineCIDRs is not configured") + } + + return nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator_test.go index 281f1bfcfa..01cc9ef1eb 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/networks_validator_test.go @@ -20,6 +20,7 @@ import ( "fmt" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" commonnetwork "github.com/deckhouse/virtualization-controller/pkg/common/network" + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" "github.com/deckhouse/virtualization-controller/pkg/featuregates" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -38,6 +40,24 @@ var ( ) func TestNetworksValidateCreate(t *testing.T) { + newValidator := func(settings map[string]any) *NetworksValidator { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + featureGate, _, setFromMap, err := featuregates.New() + if err != nil { + t.Fatalf("featuregates.New: %v", err) + } + if err := setFromMap(map[string]bool{string(featuregates.SDN): true}); err != nil { + t.Fatalf("setFromMap: %v", err) + } + return NewNetworksValidator(fake.NewClientBuilder().WithScheme(scheme).WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: settings}, + }).Build(), featureGate) + } + tests := []struct { networks []v1alpha2.NetworksSpec sdnEnabled bool @@ -93,7 +113,7 @@ func TestNetworksValidateCreate(t *testing.T) { t.Fatalf("setFromMap: %v", err) } } - networkValidator := NewNetworksValidator(nil, featureGate) + networkValidator := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}) _, err = networkValidator.ValidateCreate(t.Context(), vm) if test.valid && err != nil { @@ -107,6 +127,24 @@ func TestNetworksValidateCreate(t *testing.T) { } func TestNetworksValidateUpdate(t *testing.T) { + newValidator := func(settings map[string]any) *NetworksValidator { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + featureGate, _, setFromMap, err := featuregates.New() + if err != nil { + t.Fatalf("featuregates.New: %v", err) + } + if testErr := setFromMap(map[string]bool{string(featuregates.SDN): true}); testErr != nil { + t.Fatalf("setFromMap: %v", testErr) + } + return NewNetworksValidator(fake.NewClientBuilder().WithScheme(scheme).WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: settings}, + }).Build(), featureGate) + } + tests := []struct { oldNetworksSpec []v1alpha2.NetworksSpec newNetworksSpec []v1alpha2.NetworksSpec @@ -345,7 +383,7 @@ func TestNetworksValidateUpdate(t *testing.T) { t.Fatalf("setFromMap: %v", err) } } - networkValidator := NewNetworksValidator(nil, featureGate) + networkValidator := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}) _, err = networkValidator.ValidateUpdate(t.Context(), oldVM, newVM) if test.valid && err != nil { @@ -374,17 +412,62 @@ func newUnstructured(gvk schema.GroupVersionKind, name, namespace string) *unstr return u } +func TestNetworksValidatorMainRequiresCIDRs(t *testing.T) { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + newValidator := func(settings map[string]any) *NetworksValidator { + featureGate, _, setFromMap, err := featuregates.New() + if err != nil { + t.Fatalf("featuregates.New: %v", err) + } + if err := setFromMap(map[string]bool{string(featuregates.SDN): true}); err != nil { + t.Fatalf("setFromMap: %v", err) + } + builder := fake.NewClientBuilder().WithScheme(scheme) + if settings != nil { + builder = builder.WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: settings}, + }) + } + return NewNetworksValidator(builder.Build(), featureGate) + } + + vm := &v1alpha2.VirtualMachine{Spec: v1alpha2.VirtualMachineSpec{Networks: []v1alpha2.NetworksSpec{mainNetwork}}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}}).ValidateCreate(t.Context(), vm); err == nil { + t.Fatalf("expected error for explicit Main without CIDRs") + } + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}).ValidateCreate(t.Context(), vm); err != nil { + t.Fatalf("expected success with CIDRs, got: %v", err) + } + + oldVM := &v1alpha2.VirtualMachine{} + newVM := &v1alpha2.VirtualMachine{Spec: v1alpha2.VirtualMachineSpec{Networks: []v1alpha2.NetworksSpec{mainNetwork}}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}}).ValidateUpdate(t.Context(), oldVM, newVM); err == nil { + t.Fatalf("expected update error for explicit Main without CIDRs") + } +} + func TestNetworksValidatesExistence(t *testing.T) { scheme := runtime.NewScheme() scheme.AddKnownTypeWithName(commonnetwork.ClusterNetworkGVK, &unstructured.Unstructured{}) scheme.AddKnownTypeWithName(commonnetwork.NetworkGVK, &unstructured.Unstructured{}) + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } existingCN := newUnstructured(commonnetwork.ClusterNetworkGVK, "exists-cn", "") existingNet := newUnstructured(commonnetwork.NetworkGVK, "exists-net", "default") cli := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(existingCN, existingNet). + WithObjects(existingCN, existingNet, &mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}}, + }). Build() featureGate, _, setFromMap, err := featuregates.New() diff --git a/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook.go b/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook.go index fb7da2e470..00c10b127f 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook.go @@ -32,6 +32,7 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/common/object" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" + "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig" "github.com/deckhouse/virtualization-controller/pkg/controller/vmip/internal/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/vmipcondition" @@ -60,7 +61,12 @@ func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (adm v.log.Info("Validate VirtualMachineIPAddress creating", "name", vmip.Name, "type", vmip.Spec.Type, "address", vmip.Spec.StaticIP) - err := v.validateSpecFields(vmip) + err := v.validateVirtualMachineCIDRsConfigured(ctx) + if err != nil { + return nil, err + } + + err = v.validateSpecFields(vmip) if err != nil { return nil, fmt.Errorf("the VirtualMachineIPAddress validation is failed: %w", err) } @@ -96,7 +102,12 @@ func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.O "old.address", oldVmip.Spec.StaticIP, "new.address", newVmip.Spec.StaticIP, ) - err := v.validateSpecFields(newVmip) + err := v.validateVirtualMachineCIDRsConfigured(ctx) + if err != nil { + return nil, fmt.Errorf("error validating VirtualMachineIP update: %w", err) + } + + err = v.validateSpecFields(newVmip) if err != nil { return nil, fmt.Errorf("error validating VirtualMachineIP update: %w", err) } @@ -195,6 +206,17 @@ func (v *Validator) validateAllocatedIPAddresses(ctx context.Context, ipAddress return nil } +func (v *Validator) validateVirtualMachineCIDRsConfigured(ctx context.Context) error { + hasCIDRs, err := moduleconfig.ModuleConfigHasVirtualMachineCIDRs(ctx, v.client) + if err != nil { + return fmt.Errorf("unable to check ModuleConfig virtualization virtualMachineCIDRs: %w", err) + } + if !hasCIDRs { + return fmt.Errorf("the VirtualMachineIPAddress cannot be created: ModuleConfig/virtualization spec.settings.virtualMachineCIDRs is not configured") + } + return nil +} + func isValidAddressFormat(address string) bool { return net.ParseIP(address) != nil } diff --git a/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook_test.go b/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook_test.go new file mode 100644 index 0000000000..159b82a190 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vmip/vmip_webhook_test.go @@ -0,0 +1,44 @@ +package vmip + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/deckhouse/deckhouse/pkg/log" + mcapi "github.com/deckhouse/virtualization-controller/pkg/controller/moduleconfig/api" + "github.com/deckhouse/virtualization-controller/pkg/controller/vmip/internal/service" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +func TestValidatorValidateCreateRequiresCIDRs(t *testing.T) { + scheme := runtime.NewScheme() + if err := mcapi.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme moduleconfig: %v", err) + } + if err := v1alpha2.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme v1alpha2: %v", err) + } + + newValidator := func(settings map[string]any) *Validator { + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&mcapi.ModuleConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "virtualization"}, + Spec: mcapi.ModuleConfigSpec{Settings: settings}, + }).Build() + ipService, err := service.NewIPAddressService([]string{"10.0.0.0/24"}, cli, nil) + if err != nil { + t.Fatalf("NewIPAddressService: %v", err) + } + return NewValidator(log.NewNop(), cli, ipService) + } + + vmip := &v1alpha2.VirtualMachineIPAddress{Spec: v1alpha2.VirtualMachineIPAddressSpec{Type: v1alpha2.VirtualMachineIPAddressTypeAuto}} + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}}).ValidateCreate(t.Context(), vmip); err == nil { + t.Fatalf("expected error without CIDRs") + } + if _, err := newValidator(map[string]any{"dvcr": map[string]any{}, "virtualMachineCIDRs": []any{"10.0.0.0/24"}}).ValidateCreate(t.Context(), vmip); err != nil { + t.Fatalf("expected success with CIDRs, got: %v", err) + } +} diff --git a/openapi/config-values.yaml b/openapi/config-values.yaml index 2b2aa2b814..bca8bfe392 100644 --- a/openapi/config-values.yaml +++ b/openapi/config-values.yaml @@ -1,5 +1,5 @@ type: object -required: [virtualMachineCIDRs, dvcr] +required: [dvcr] properties: highAvailability: type: boolean @@ -13,7 +13,12 @@ properties: type: array minItems: 1 description: | - List of CIDRs used to allocate static IP addresses for Virtual Machines. + Optional list of CIDRs used to allocate static IP addresses for Virtual Machines. + + If this parameter is omitted, the module can still be enabled, but IPAM-dependent scenarios are unavailable: + - `VirtualMachineIPAddress` resources cannot be created or used; + - Virtual Machines cannot explicitly request the `Main` network in `spec.networks`; + - Virtual Machines without `spec.networks` remain allowed. > **Warning.** The subnet for virtual machines should not be the same as the subnet used for pods, services and nodes. Address conflicts can lead to unpredictable behavior and networking problems. > diff --git a/openapi/doc-ru-config-values.yaml b/openapi/doc-ru-config-values.yaml index 2e29677fb7..ddc94dd03a 100644 --- a/openapi/doc-ru-config-values.yaml +++ b/openapi/doc-ru-config-values.yaml @@ -8,7 +8,12 @@ properties: Подробнее про режим отказоустойчивости можно прочитать в разделе [Высокая надежность и доступность](/products/kubernetes-platform/documentation/v1/admin/configuration/high-reliability-and-availability/enable.html#включение-режима-ha-для-отдельных-компонентов). virtualMachineCIDRs: description: | - Список подсетей используемых для выделения статических IP-адресов виртуальным машинам. + Необязательный список подсетей, используемых для выделения статических IP-адресов виртуальным машинам. + + Если параметр не задан, модуль всё равно можно включить, но сценарии, зависящие от IPAM, недоступны: + - нельзя создавать или использовать ресурсы `VirtualMachineIPAddress`; + - нельзя создавать или обновлять виртуальные машины с явно указанной сетью `Main` в `spec.networks`; + - виртуальные машины без `spec.networks` остаются допустимыми. > **Внимание.** Подсеть для виртуальных машин не должна совпадать с подсетями подов, сервисов и узлов. Конфликт адресов может привести к непредсказуемому поведению и сетевым проблемам. > diff --git a/openapi/openapi-case-tests.yaml b/openapi/openapi-case-tests.yaml index 11e0a31c2d..bee3330e11 100644 --- a/openapi/openapi-case-tests.yaml +++ b/openapi/openapi-case-tests.yaml @@ -1,7 +1,9 @@ positive: configValues: + - {} - virtualMachineCIDRs: ["10.10.10.0/24"] values: + - internal: {} - virtualMachineCIDRs: ["10.10.10.0/24"] internal: {} - https: @@ -14,6 +16,8 @@ positive: tls.key: plainstring negative: configValues: + - virtualMachineCIDRs: [] - { somethingInConfig: yes } values: + - virtualMachineCIDRs: [] - { somethingInConfig: yes }