From 7945032524992604caf4a809ba9d32304a3fdd14 Mon Sep 17 00:00:00 2001 From: Andrea Mazzotti Date: Tue, 22 Oct 2024 14:56:33 +0200 Subject: [PATCH] Only reset network if a network configurator is used (#874) * Only reset network if a network configurator is used Signed-off-by: Andrea Mazzotti --- .../templates/crds.yaml | 6 +- api/v1beta1/machineinventory_types.go | 1 + api/v1beta1/types.go | 8 +- ...lemental.cattle.io_machineinventories.yaml | 3 +- ...mental.cattle.io_machineregistrations.yaml | 3 +- controllers/machineinventory_controller.go | 75 ++++++++++--------- .../machineinventory_controller_test.go | 68 ++++++++++++++++- pkg/network/network.go | 20 +++-- pkg/server/api_registration.go | 15 ++-- pkg/server/server.go | 12 ++- 10 files changed, 152 insertions(+), 59 deletions(-) diff --git a/.obs/chartfile/elemental-operator-crds-helm/templates/crds.yaml b/.obs/chartfile/elemental-operator-crds-helm/templates/crds.yaml index 20f8a233..e4e6bb9e 100644 --- a/.obs/chartfile/elemental-operator-crds-helm/templates/crds.yaml +++ b/.obs/chartfile/elemental-operator-crds-helm/templates/crds.yaml @@ -150,9 +150,10 @@ spec: nmstate, or nmconnections formats) x-kubernetes-preserve-unknown-fields: true configurator: - default: nmc + default: none description: Configurator enum: + - none - nmc - nmstate - nmconnections @@ -956,9 +957,10 @@ spec: nmstate, or nmconnections formats) x-kubernetes-preserve-unknown-fields: true configurator: - default: nmc + default: none description: Configurator enum: + - none - nmc - nmstate - nmconnections diff --git a/api/v1beta1/machineinventory_types.go b/api/v1beta1/machineinventory_types.go index 65502655..4ff52276 100644 --- a/api/v1beta1/machineinventory_types.go +++ b/api/v1beta1/machineinventory_types.go @@ -51,6 +51,7 @@ type MachineInventorySpec struct { // IPAddressPools is a list of IPAddressPool associated to this machine. IPAddressPools map[string]*corev1.TypedLocalObjectReference `json:"ipAddressPools,omitempty"` // NetworkConfig is the final NetworkConfig. + // +optional Network NetworkConfig `json:"network,omitempty"` } diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index b7fd5e61..1a10e209 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -178,8 +178,8 @@ const ( // NetworkTemplate contains a map of IPAddressPools and a schemaless network config template. type NetworkTemplate struct { // Configurator - // +kubebuilder:validation:Enum=nmc;nmstate;nmconnections - // +kubebuilder:default:=nmc + // +kubebuilder:validation:Enum=none;nmc;nmstate;nmconnections + // +kubebuilder:default:=none Configurator string `json:"configurator,omitempty" yaml:"configurator,omitempty"` // IPAddresses contains a map of IPPools references IPAddresses map[string]*corev1.TypedLocalObjectReference `json:"ipAddresses,omitempty" yaml:"ipAddresses,omitempty"` @@ -202,8 +202,8 @@ type NetworkTemplate struct { // can do the substitution itself. type NetworkConfig struct { // Configurator - // +kubebuilder:validation:Enum=nmc;nmstate;nmconnections - // +kubebuilder:default:=nmc + // +kubebuilder:validation:Enum=none;nmc;nmstate;nmconnections + // +kubebuilder:default:=none Configurator string `json:"configurator,omitempty" yaml:"configurator,omitempty"` // IPAddresses contains a map of claimed IPAddresses IPAddresses map[string]string `json:"ipAddresses,omitempty" yaml:"ipAddresses,omitempty"` diff --git a/config/crd/bases/elemental.cattle.io_machineinventories.yaml b/config/crd/bases/elemental.cattle.io_machineinventories.yaml index b91781d2..8310ce4f 100644 --- a/config/crd/bases/elemental.cattle.io_machineinventories.yaml +++ b/config/crd/bases/elemental.cattle.io_machineinventories.yaml @@ -144,9 +144,10 @@ spec: nmstate, or nmconnections formats) x-kubernetes-preserve-unknown-fields: true configurator: - default: nmc + default: none description: Configurator enum: + - none - nmc - nmstate - nmconnections diff --git a/config/crd/bases/elemental.cattle.io_machineregistrations.yaml b/config/crd/bases/elemental.cattle.io_machineregistrations.yaml index daf3faf5..a3be7a8f 100644 --- a/config/crd/bases/elemental.cattle.io_machineregistrations.yaml +++ b/config/crd/bases/elemental.cattle.io_machineregistrations.yaml @@ -190,9 +190,10 @@ spec: nmstate, or nmconnections formats) x-kubernetes-preserve-unknown-fields: true configurator: - default: nmc + default: none description: Configurator enum: + - none - nmc - nmstate - nmconnections diff --git a/controllers/machineinventory_controller.go b/controllers/machineinventory_controller.go index 4e1bf329..5fdc4373 100644 --- a/controllers/machineinventory_controller.go +++ b/controllers/machineinventory_controller.go @@ -47,6 +47,7 @@ import ( elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" systemagent "github.com/rancher/elemental-operator/internal/system-agent" "github.com/rancher/elemental-operator/pkg/log" + "github.com/rancher/elemental-operator/pkg/network" "github.com/rancher/elemental-operator/pkg/util" ) @@ -267,11 +268,13 @@ func (r *MachineInventoryReconciler) updatePlanSecretWithReset(ctx context.Conte var resetPlan []byte var err error + networkNeedsReset := mInventory.Spec.Network.Configurator != network.ConfiguratorNone + unmanaged, unmanagedFound := mInventory.Annotations[elementalv1.MachineInventoryOSUnmanagedAnnotation] if unmanagedFound && unmanaged == "true" { checksum, resetPlan, err = r.newUnmanagedResetPlan(ctx) } else { - checksum, resetPlan, err = r.newResetPlan(ctx) + checksum, resetPlan, err = r.newResetPlan(ctx, networkNeedsReset) } if err != nil { @@ -396,7 +399,7 @@ func (r *MachineInventoryReconciler) reconcileNetworkConfig(ctx context.Context, return ctrl.Result{}, nil } -func (r *MachineInventoryReconciler) newResetPlan(ctx context.Context) (string, []byte, error) { +func (r *MachineInventoryReconciler) newResetPlan(ctx context.Context, resetNetwork bool) (string, []byte, error) { logger := ctrl.LoggerFrom(ctx) logger.Info("Creating new Reset plan secret") @@ -422,6 +425,40 @@ func (r *MachineInventoryReconciler) newResetPlan(ctx context.Context) (string, return "", nil, fmt.Errorf("marshalling local reset cloud-config to yaml: %w", err) } + resetInstructions := []systemagent.OneTimeInstruction{} + + if resetNetwork { + resetInstructions = append(resetInstructions, systemagent.OneTimeInstruction{ + CommonInstruction: systemagent.CommonInstruction{ + Name: "restore first boot config", + Command: "elemental-register", + Args: []string{"--debug", "--reset-network"}, + }, + }) + } + + resetInstructions = append(resetInstructions, systemagent.OneTimeInstruction{ + CommonInstruction: systemagent.CommonInstruction{ + Name: "configure next boot to recovery mode", + Command: "grub2-editenv", + Args: []string{ + "/oem/grubenv", + "set", + "next_entry=recovery", + }, + }, + }) + resetInstructions = append(resetInstructions, systemagent.OneTimeInstruction{ + CommonInstruction: systemagent.CommonInstruction{ + Name: "schedule reboot", + Command: "shutdown", + Args: []string{ + "-r", + "+1", // Need to have time to confirm plan execution before rebooting + }, + }, + }) + // This is the remote plan that should trigger the reboot into recovery and reset resetPlan := systemagent.Plan{ Files: []systemagent.File{ @@ -431,39 +468,7 @@ func (r *MachineInventoryReconciler) newResetPlan(ctx context.Context) (string, Permissions: "0600", }, }, - OneTimeInstructions: []systemagent.OneTimeInstruction{ - { - CommonInstruction: systemagent.CommonInstruction{ - Name: "restore first boot config", - Command: "elemental-register", - Args: []string{ - "--debug", - "--reset-network", - }, - }, - }, - { - CommonInstruction: systemagent.CommonInstruction{ - Name: "configure next boot to recovery mode", - Command: "grub2-editenv", - Args: []string{ - "/oem/grubenv", - "set", - "next_entry=recovery", - }, - }, - }, - { - CommonInstruction: systemagent.CommonInstruction{ - Name: "schedule reboot", - Command: "shutdown", - Args: []string{ - "-r", - "+1", // Need to have time to confirm plan execution before rebooting - }, - }, - }, - }, + OneTimeInstructions: resetInstructions, } var buf bytes.Buffer diff --git a/controllers/machineinventory_controller_test.go b/controllers/machineinventory_controller_test.go index 704a7668..1fbef817 100644 --- a/controllers/machineinventory_controller_test.go +++ b/controllers/machineinventory_controller_test.go @@ -18,6 +18,7 @@ package controllers import ( "context" + "encoding/json" "errors" "time" @@ -33,6 +34,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" + systemagent "github.com/rancher/elemental-operator/internal/system-agent" + "github.com/rancher/elemental-operator/pkg/network" "github.com/rancher/elemental-operator/pkg/test" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -227,6 +230,69 @@ var _ = Describe("createPlanSecret", func() { }) Expect(r.createPlanSecret(ctx, mInventory)).To(Succeed()) }) + + It("should not reset network if configurator none", func() { + inventoryWithNoneNetwork := &elementalv1.MachineInventory{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-inventory-network-none", + Namespace: "default", + }, + Spec: elementalv1.MachineInventorySpec{ + Network: elementalv1.NetworkConfig{ + Configurator: network.ConfiguratorNone, + }, + }, + } + Expect(r.Create(ctx, inventoryWithNoneNetwork)).Should(Succeed()) + Expect(r.createPlanSecret(ctx, inventoryWithNoneNetwork)).To(Succeed()) + Expect(r.Get(ctx, types.NamespacedName{Namespace: inventoryWithNoneNetwork.Namespace, Name: inventoryWithNoneNetwork.Name}, planSecret)).To(Succeed()) + Expect(r.updatePlanSecretWithReset(ctx, inventoryWithNoneNetwork, planSecret)).Should(Succeed()) + Expect(r.Get(ctx, types.NamespacedName{Namespace: inventoryWithNoneNetwork.Namespace, Name: inventoryWithNoneNetwork.Name}, planSecret)).To(Succeed()) + planData, found := planSecret.Data["plan"] + Expect(found).To(BeTrue(), "Secret should contain reset plan data") + + plan := &systemagent.Plan{} + Expect(json.Unmarshal(planData, plan)).Should(Succeed()) + Expect(len(plan.OneTimeInstructions)).Should(Equal(2), "Should contain 2 reset instructions") + Expect(plan.OneTimeInstructions).ShouldNot(ContainElement(systemagent.OneTimeInstruction{ + CommonInstruction: systemagent.CommonInstruction{ + Name: "restore first boot config", + Command: "elemental-register", + Args: []string{"--debug", "--reset-network"}, + }})) + Expect(test.CleanupAndWait(ctx, cl, inventoryWithNoneNetwork)).To(Succeed()) + }) + + It("should reset network if configurator not none", func() { + inventoryWithNetwork := &elementalv1.MachineInventory{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-inventory-network-nmc", + Namespace: "default", + }, + Spec: elementalv1.MachineInventorySpec{ + Network: elementalv1.NetworkConfig{ + Configurator: network.ConfiguratorNmc, + }, + }, + } + Expect(r.Create(ctx, inventoryWithNetwork)).Should(Succeed()) + Expect(r.createPlanSecret(ctx, inventoryWithNetwork)).To(Succeed()) + Expect(r.Get(ctx, types.NamespacedName{Namespace: inventoryWithNetwork.Namespace, Name: inventoryWithNetwork.Name}, planSecret)).To(Succeed()) + Expect(r.updatePlanSecretWithReset(ctx, inventoryWithNetwork, planSecret)).Should(Succeed()) + Expect(r.Get(ctx, types.NamespacedName{Namespace: inventoryWithNetwork.Namespace, Name: inventoryWithNetwork.Name}, planSecret)).To(Succeed()) + planData, found := planSecret.Data["plan"] + Expect(found).To(BeTrue(), "Secret should contain reset plan data") + + plan := &systemagent.Plan{} + Expect(json.Unmarshal(planData, plan)).Should(Succeed()) + Expect(plan.OneTimeInstructions).Should(ContainElement(systemagent.OneTimeInstruction{ + CommonInstruction: systemagent.CommonInstruction{ + Name: "restore first boot config", + Command: "elemental-register", + Args: []string{"--debug", "--reset-network"}, + }})) + Expect(test.CleanupAndWait(ctx, cl, inventoryWithNetwork)).To(Succeed()) + }) }) var _ = Describe("updateInventoryWithAdoptionStatus", func() { @@ -475,7 +541,7 @@ var _ = Describe("handle finalizer", func() { Namespace: mInventory.Namespace, }, mInventory)).To(Succeed()) - wantChecksum, wantPlan, err := r.newResetPlan(ctx) + wantChecksum, wantPlan, err := r.newResetPlan(ctx, false) Expect(err).ToNot(HaveOccurred()) // Check Plan status diff --git a/pkg/network/network.go b/pkg/network/network.go index f80c8093..561b79c4 100644 --- a/pkg/network/network.go +++ b/pkg/network/network.go @@ -29,6 +29,13 @@ import ( "github.com/twpayne/go-vfs" ) +const ( + ConfiguratorNone = "none" + ConfiguratorNmc = "nmc" + ConfiguratorNmstate = "nmstate" + ConfiguratorNmconnections = "nmconnections" +) + const ( // common systemConnectionsDir = "/etc/NetworkManager/system-connections" @@ -70,13 +77,17 @@ func NewConfigurator(fs vfs.FS) Configurator { func (c *configurator) GetNetworkConfigApplicator(networkConfig elementalv1.NetworkConfig) (schema.YipConfig, error) { switch networkConfig.Configurator { - case "nmc": + case "": + return schema.YipConfig{}, nil + case ConfiguratorNone: + return schema.YipConfig{}, nil + case ConfiguratorNmc: nc := nmcConfigurator{fs: c.fs, runner: c.runner} return nc.GetNetworkConfigApplicator(networkConfig) - case "nmstate": + case ConfiguratorNmstate: nc := nmstateConfigurator{fs: c.fs, runner: c.runner} return nc.GetNetworkConfigApplicator(networkConfig) - case "nmconnections": + case ConfiguratorNmconnections: nc := networkManagerConfigurator{fs: c.fs, runner: c.runner} return nc.GetNetworkConfigApplicator(networkConfig) default: @@ -122,8 +133,7 @@ func (c *configurator) ResetNetworkConfig() error { } // Delete all .nmconnection files. This will also delete any "static" connection that the user defined in the base image for example. - // Which means maybe that is not supported anymore, or if we want to support it we should make sure we only delete the ones created by elemental, - // for example prefixing all files with "elemental-" or just parsing the network config again at this stage to determine the file names. + // Note that ResetNetworkConfig() is only called when the network config is Elemental driven, hence we not expect any other custom connection defined. log.Debug("Deleting all .nmconnection configs") if err := c.runner.Run("find", systemConnectionsDir, "-name", "*.nmconnection", "-type", "f", "-delete"); err != nil { return fmt.Errorf("deleting all %s/*.nmconnection: %w", systemConnectionsDir, err) diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index 85a4cebd..bbe3c329 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -36,6 +36,7 @@ import ( elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" "github.com/rancher/elemental-operator/pkg/hostinfo" "github.com/rancher/elemental-operator/pkg/log" + "github.com/rancher/elemental-operator/pkg/network" "github.com/rancher/elemental-operator/pkg/register" "github.com/rancher/elemental-operator/pkg/templater" ) @@ -125,7 +126,7 @@ func (i *InventoryServer) unauthenticatedResponse(registration *elementalv1.Mach Encode(config) } -func (i *InventoryServer) writeMachineInventoryCloudConfig(conn *websocket.Conn, protoVersion register.MessageType, inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration, netConf *elementalv1.NetworkConfig) error { +func (i *InventoryServer) writeMachineInventoryCloudConfig(conn *websocket.Conn, protoVersion register.MessageType, inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration, netConf elementalv1.NetworkConfig) error { sa := &corev1.ServiceAccount{} if err := i.Get(i, types.NamespacedName{ @@ -195,7 +196,7 @@ func (i *InventoryServer) writeMachineInventoryCloudConfig(conn *websocket.Conn, return err } - if netConf != nil { + if netConf.Configurator != network.ConfiguratorNone && netConf.Configurator != "" { netData, err := yaml.Marshal(netConf) if err != nil { log.Errorf("error marshalling network config: %v", err) @@ -321,19 +322,19 @@ func (i *InventoryServer) handleUpdate(conn *websocket.Conn, protoVersion regist return nil } -func (i *InventoryServer) handleGetNetworkConfig(inventory *elementalv1.MachineInventory) (*elementalv1.NetworkConfig, error) { +func (i *InventoryServer) handleGetNetworkConfig(inventory *elementalv1.MachineInventory) (elementalv1.NetworkConfig, error) { ctx, cancel := context.WithTimeout(context.TODO(), register.RegistrationDeadlineSeconds*time.Second) defer cancel() for { select { case <-ctx.Done(): - return nil, fmt.Errorf("NewtworkConfig not ready") + return elementalv1.NetworkConfig{}, fmt.Errorf("NewtworkConfig not ready") default: time.Sleep(time.Second) } if err := i.Get(ctx, client.ObjectKeyFromObject(inventory), inventory); err != nil { - return nil, fmt.Errorf("getting machine inventory: %w", err) + return elementalv1.NetworkConfig{}, fmt.Errorf("getting machine inventory: %w", err) } conditionFound := false for _, condition := range inventory.Status.Conditions { @@ -352,12 +353,12 @@ func (i *InventoryServer) handleGetNetworkConfig(inventory *elementalv1.MachineI break } - return &inventory.Spec.Network, nil + return inventory.Spec.Network, nil } func (i *InventoryServer) handleGet(conn *websocket.Conn, protoVersion register.MessageType, inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration) error { var err error - var netConf *elementalv1.NetworkConfig + var netConf elementalv1.NetworkConfig inventory, err = i.commitMachineInventory(inventory) if err != nil { diff --git a/pkg/server/server.go b/pkg/server/server.go index 4f975ea6..bf658295 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -32,6 +32,7 @@ import ( elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" "github.com/rancher/elemental-operator/pkg/log" + "github.com/rancher/elemental-operator/pkg/network" "github.com/rancher/elemental-operator/pkg/plainauth" "github.com/rancher/elemental-operator/pkg/register" "github.com/rancher/elemental-operator/pkg/tpm" @@ -170,9 +171,14 @@ func initInventory(inventory *elementalv1.MachineInventory, registration *elemen } // Forward network config from Registration - inventory.Spec.Network.Configurator = registration.Spec.Config.Network.Configurator - inventory.Spec.Network.Config = registration.Spec.Config.Network.Config - inventory.Spec.IPAddressPools = registration.Spec.Config.Network.IPAddresses + if registration.Spec.Config.Network.Configurator != network.ConfiguratorNone { + inventory.Spec.Network.Configurator = registration.Spec.Config.Network.Configurator + inventory.Spec.Network.Config = registration.Spec.Config.Network.Config + inventory.Spec.IPAddressPools = registration.Spec.Config.Network.IPAddresses + } + if registration.Spec.Config.Network.Configurator == "" { + inventory.Spec.Network.Configurator = network.ConfiguratorNone + } } func (i *InventoryServer) createMachineInventory(inventory *elementalv1.MachineInventory) (*elementalv1.MachineInventory, error) {