Cleanup code and add comments

Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>

.obs/chartfile/crds/templates/crds.yaml

@@ -348,39 +348,6 @@ spec:
             type: object
           spec:
             properties:
-              network:
-                description: Network is the network template to be applied during
-                  MachineInventory adoption.
-                properties:
-                  connections:
-                    additionalProperties:
-                      type: string
-                    type: object
-                  ipAddresses:
-                    additionalProperties:
-                      description: |-
-                        TypedLocalObjectReference contains enough information to let you locate the
-                        typed referenced object inside the same namespace.
-                      properties:
-                        apiGroup:
-                          description: |-
-                            APIGroup is the group for the resource being referenced.
-                            If APIGroup is not specified, the specified Kind must be in the core API group.
-                            For any other third-party types, APIGroup is required.
-                          type: string
-                        kind:
-                          description: Kind is the type of resource being referenced
-                          type: string
-                        name:
-                          description: Name is the name of resource being referenced
-                          type: string
-                      required:
-                      - kind
-                      - name
-                      type: object
-                      x-kubernetes-map-type: atomic
-                    type: object
-                type: object
               providerID:
                 description: |-
                   ProviderID the identifier for the elemental instance.
@@ -614,41 +581,6 @@ spec:
                     type: object
                   spec:
                     properties:
-                      network:
-                        description: Network is the network template to be applied
-                          during MachineInventory adoption.
-                        properties:
-                          connections:
-                            additionalProperties:
-                              type: string
-                            type: object
-                          ipAddresses:
-                            additionalProperties:
-                              description: |-
-                                TypedLocalObjectReference contains enough information to let you locate the
-                                typed referenced object inside the same namespace.
-                              properties:
-                                apiGroup:
-                                  description: |-
-                                    APIGroup is the group for the resource being referenced.
-                                    If APIGroup is not specified, the specified Kind must be in the core API group.
-                                    For any other third-party types, APIGroup is required.
-                                  type: string
-                                kind:
-                                  description: Kind is the type of resource being
-                                    referenced
-                                  type: string
-                                name:
-                                  description: Name is the name of resource being
-                                    referenced
-                                  type: string
-                              required:
-                              - kind
-                              - name
-                              type: object
-                              x-kubernetes-map-type: atomic
-                            type: object
-                        type: object
                       providerID:
                         description: |-
                           ProviderID the identifier for the elemental instance.
@@ -1002,9 +934,8 @@ spec:
                         type: object
                     type: object
                   network:
-                    description: |-
-                      NetworkTemplate contains a list of IPAddressPools and a network config template.
-                      This template can be defined in both MachineRegistrations and MachineSelectors.
+                    description: NetworkTemplate contains a map of IPAddressPools
+                      and a map of connection templates.
                     properties:
                       connections:
                         additionalProperties:

api/v1beta1/machineinventory_types.go

@@ -30,7 +30,6 @@ const (
 	PlanTypeReset                                           = "reset"
 	MachineInventoryResettableAnnotation                    = "elemental.cattle.io/resettable"
 	MachineInventoryOSUnmanagedAnnotation                   = "elemental.cattle.io/os.unmanaged"
-	MachineInventoryNetworkConfigApplied                    = "elemental.cattle.io/network.applied"
 )
 
 type MachineInventorySpec struct {

api/v1beta1/machineselector_types.go

@@ -38,9 +38,6 @@ type MachineInventorySelectorSpec struct {
 	ProviderID string `json:"providerID,omitempty"`
 	// Selector selector to choose elemental machines.
 	Selector metav1.LabelSelector `json:"selector,omitempty"`
-	// Network is the network template to be applied during MachineInventory adoption.
-	// +optional
-	Network NetworkTemplate `json:"network,omitempty" yaml:"network"`
 }
 
 type MachineInventorySelectorStatus struct {

api/v1beta1/types.go

@@ -173,17 +173,22 @@ const (
 	DeviceSelectorKeySize DeviceSelectorKey = "Size"
 )
 
-// NetworkTemplate contains a list of IPAddressPools and a network config template.
-// This template can be defined in both MachineRegistrations and MachineSelectors.
+// NetworkTemplate contains a map of IPAddressPools and a map of connection templates.
 type NetworkTemplate struct {
 	IPAddresses map[string]*corev1.TypedLocalObjectReference `json:"ipAddresses,omitempty" yaml:"ipAddresses,omitempty"`
 	Connections map[string]string                            `json:"connections,omitempty" yaml:"connections,omitempty"`
 }
 
-// NetworkConfig contains a list of claimed IPAddresses and a network config template.
+// NetworkConfig contains a map of claimed IPAddresses and a map of connection templates.
 // This config is a digested NetworkTemplate, the MachineInventory Ready condition highlight that
 // this config is ready to be consumed, this means all needed IPAddressClaims for this machine
 // have been created and the IPAM provider served real IPAddresses that can be applied to the machine.
+//
+// Note that Connections still carries the same connection templates from the NetworkTemplate object.
+// An alternative could be to simplify this object to contain final connections where the variable
+// substitution already took place (fully digested).
+// Right now we send both connection templates and real ipaddressed so the consumer (elemental-register)
+// can do the substitution itself.
 type NetworkConfig struct {
 	IPAddresses map[string]string `json:"ipAddresses,omitempty" yaml:"ipAddresses,omitempty"`
 	Connections map[string]string `json:"connections,omitempty" yaml:"connections,omitempty"`

config/crd/bases/elemental.cattle.io_machineinventoryselectors.yaml

@@ -37,39 +37,6 @@ spec:
             type: object
           spec:
             properties:
-              network:
-                description: Network is the network template to be applied during
-                  MachineInventory adoption.
-                properties:
-                  connections:
-                    additionalProperties:
-                      type: string
-                    type: object
-                  ipAddresses:
-                    additionalProperties:
-                      description: |-
-                        TypedLocalObjectReference contains enough information to let you locate the
-                        typed referenced object inside the same namespace.
-                      properties:
-                        apiGroup:
-                          description: |-
-                            APIGroup is the group for the resource being referenced.
-                            If APIGroup is not specified, the specified Kind must be in the core API group.
-                            For any other third-party types, APIGroup is required.
-                          type: string
-                        kind:
-                          description: Kind is the type of resource being referenced
-                          type: string
-                        name:
-                          description: Name is the name of resource being referenced
-                          type: string
-                      required:
-                      - kind
-                      - name
-                      type: object
-                      x-kubernetes-map-type: atomic
-                    type: object
-                type: object
               providerID:
                 description: |-
                   ProviderID the identifier for the elemental instance.

config/crd/bases/elemental.cattle.io_machineinventoryselectortemplates.yaml

@@ -59,41 +59,6 @@ spec:
                     type: object
                   spec:
                     properties:
-                      network:
-                        description: Network is the network template to be applied
-                          during MachineInventory adoption.
-                        properties:
-                          connections:
-                            additionalProperties:
-                              type: string
-                            type: object
-                          ipAddresses:
-                            additionalProperties:
-                              description: |-
-                                TypedLocalObjectReference contains enough information to let you locate the
-                                typed referenced object inside the same namespace.
-                              properties:
-                                apiGroup:
-                                  description: |-
-                                    APIGroup is the group for the resource being referenced.
-                                    If APIGroup is not specified, the specified Kind must be in the core API group.
-                                    For any other third-party types, APIGroup is required.
-                                  type: string
-                                kind:
-                                  description: Kind is the type of resource being
-                                    referenced
-                                  type: string
-                                name:
-                                  description: Name is the name of resource being
-                                    referenced
-                                  type: string
-                              required:
-                              - kind
-                              - name
-                              type: object
-                              x-kubernetes-map-type: atomic
-                            type: object
-                        type: object
                       providerID:
                         description: |-
                           ProviderID the identifier for the elemental instance.

config/crd/bases/elemental.cattle.io_machineregistrations.yaml

@@ -173,9 +173,8 @@ spec:
                         type: object
                     type: object
                   network:
-                    description: |-
-                      NetworkTemplate contains a list of IPAddressPools and a network config template.
-                      This template can be defined in both MachineRegistrations and MachineSelectors.
+                    description: NetworkTemplate contains a map of IPAddressPools
+                      and a map of connection templates.
                     properties:
                       connections:
                         additionalProperties:

controllers/machineinventory_controller.go

@@ -161,6 +161,12 @@ func (r *MachineInventoryReconciler) reconcile(ctx context.Context, mInventory *
 	}
 
 	if r.networkNeedsReconcile(*mInventory) {
+		meta.SetStatusCondition(&mInventory.Status.Conditions, metav1.Condition{
+			Type:    elementalv1.ReadyCondition,
+			Reason:  elementalv1.ReconcilingNetworkConfig,
+			Status:  metav1.ConditionFalse,
+			Message: "NetworkConfig needs reconcile",
+		})
 		result, err := r.reconcileNetworkConfig(ctx, mInventory)
 		if err != nil {
 			meta.SetStatusCondition(&mInventory.Status.Conditions, metav1.Condition{
@@ -310,28 +316,14 @@ func (r *MachineInventoryReconciler) reconcileNetworkConfig(ctx context.Context,
 	logger := ctrl.LoggerFrom(ctx)
 	logger.Info("Reconciling Network Config")
 
-	// Before we do any change, we make sure the MachineInventory is patched with the appropriate status and annotation.
-	// This will put the MachineInventory "on hold" with a Ready=false condition, so that the elemental-register knows
-	// the config is not ready yet to be consumed. IPAddressClaims must be created and IPAddresses must be served first.
-	if mInventory.Annotations[elementalv1.MachineInventoryNetworkConfigApplied] != "false" {
-		logger.Info("Marking Network Config as deprecated")
-		mInventory.Annotations[elementalv1.MachineInventoryNetworkConfigApplied] = "false"
-		meta.SetStatusCondition(&mInventory.Status.Conditions, metav1.Condition{
-			Type:    elementalv1.NetworkConfigReady,
-			Reason:  elementalv1.ReconcilingNetworkConfig,
-			Status:  metav1.ConditionFalse,
-			Message: "NetworkConfig needs changes",
-		})
-		return ctrl.Result{RequeueAfter: time.Second}, nil
-	}
-
-	// Loops over the IPAddressPools and create an IPClaim for each
+	// Loops over the IPAddressPools and create an IPAddressClaim for each
 	for name, ipPoolRef := range mInventory.Spec.IPAddressPools {
 		ipClaimName := fmt.Sprintf("%s-%s", mInventory.Name, name)
 		ipClaim := &ipamv1.IPAddressClaim{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      ipClaimName,
 				Namespace: mInventory.Namespace,
+				// Ownership takes care of IPAddressClaim deletion when the MachineInventory is also deleted.
 				OwnerReferences: []metav1.OwnerReference{
 					{
 						APIVersion: mInventory.APIVersion,
@@ -355,6 +347,7 @@ func (r *MachineInventoryReconciler) reconcileNetworkConfig(ctx context.Context,
 		if err := r.Get(ctx, client.ObjectKeyFromObject(ipClaim), ipClaim); err != nil {
 			return ctrl.Result{}, fmt.Errorf("getting IPAddressClaim '%s': %w", ipClaimName, err)
 		}
+		// Just for safety, prevent usage of any IPClaim that is undergoing deletion (we don't have an IPAddressClaim watch for a further reconcile)
 		if !ipClaim.DeletionTimestamp.IsZero() {
 			return ctrl.Result{}, fmt.Errorf("Waiting for IPAddressClaim deletion '%s'", ipClaimName)
 		}

pkg/install/install.go

@@ -100,24 +100,13 @@ func (i *installer) InstallElemental(config elementalv1.Config, state register.S
 		log.Warningf("Both device and device-selector set, using device-field '%s'", config.Elemental.Install.Device)
 	}
 
-	additionalConfigs, err := i.getCloudInitConfigs(config, state)
+	additionalConfigs, err := i.getCloudInitConfigs(config, state, networkConfig)
 	if err != nil {
 		return fmt.Errorf("generating additional cloud configs: %w", err)
 	}
 
 	config.Elemental.Install.ConfigURLs = append(config.Elemental.Install.ConfigURLs, additionalConfigs...)
 
-	log.Info("Installing Network config")
-	networkYipConfig := i.networkConfigurator.GetNetworkConfigApplicator(networkConfig)
-	networkConfigBytes, err := yaml.Marshal(networkYipConfig)
-	if err != nil {
-		return fmt.Errorf("marshalling network config: %w", err)
-	}
-	if err := i.fs.WriteFile(tempNetworkConfig, networkConfigBytes, 0600); err != nil {
-		return fmt.Errorf("writing file '%s': %w", tempNetworkConfig, err)
-	}
-	config.Elemental.Install.ConfigURLs = append(config.Elemental.Install.ConfigURLs, tempNetworkConfig)
-
 	if err := i.runner.Install(config.Elemental.Install); err != nil {
 		return fmt.Errorf("failed to install elemental: %w", err)
 	}
@@ -131,23 +120,12 @@ func (i *installer) ResetElemental(config elementalv1.Config, state register.Sta
 		config.Elemental.Reset.ConfigURLs = []string{}
 	}
 
-	additionalConfigs, err := i.getCloudInitConfigs(config, state)
+	additionalConfigs, err := i.getCloudInitConfigs(config, state, networkConfig)
 	if err != nil {
 		return fmt.Errorf("generating additional cloud configs: %w", err)
 	}
 	config.Elemental.Reset.ConfigURLs = append(config.Elemental.Reset.ConfigURLs, additionalConfigs...)
 
-	log.Info("Installing Network config")
-	networkYipConfig := i.networkConfigurator.GetNetworkConfigApplicator(networkConfig)
-	networkConfigBytes, err := yaml.Marshal(networkYipConfig)
-	if err != nil {
-		return fmt.Errorf("marshalling network config: %w", err)
-	}
-	if err := i.fs.WriteFile(tempNetworkConfig, networkConfigBytes, 0600); err != nil {
-		return fmt.Errorf("writing file '%s': %w", tempNetworkConfig, err)
-	}
-	config.Elemental.Reset.ConfigURLs = append(config.Elemental.Reset.ConfigURLs, tempNetworkConfig)
-
 	if err := i.runner.Reset(config.Elemental.Reset); err != nil {
 		return fmt.Errorf("failed to reset elemental: %w", err)
 	}
@@ -263,7 +241,7 @@ func matchesGt(disk *block.Disk, req elementalv1.DeviceSelectorRequirement) (boo
 // getCloudInitConfigs creates cloud-init configuration files that can be passed as additional `config-urls`
 // to the `elemental` cli. We exploit this mechanism to persist information during `elemental install`
 // or `elemental reset` calls into the newly installed or resetted system.
-func (i *installer) getCloudInitConfigs(config elementalv1.Config, state register.State) ([]string, error) {
+func (i *installer) getCloudInitConfigs(config elementalv1.Config, state register.State, networkConfig elementalv1.NetworkConfig) ([]string, error) {
 	configs := []string{}
 	agentConfPath, err := i.writeSystemAgentConfig(config.Elemental)
 	if err != nil {
@@ -291,6 +269,12 @@ func (i *installer) getCloudInitConfigs(config elementalv1.Config, state registe
 	}
 	configs = append(configs, registrationStatePath)
 
+	networkConfigPath, err := i.writeNetworkConfig(networkConfig)
+	if err != nil {
+		return nil, fmt.Errorf("writing temporary network config: %w", err)
+	}
+	configs = append(configs, networkConfigPath)
+
 	return configs, nil
 }
 
@@ -393,6 +377,18 @@ func (i *installer) writeCloudInit(cloudConfig map[string]runtime.RawExtension)
 	return f.Name(), nil
 }
 
+func (i *installer) writeNetworkConfig(networkConfig elementalv1.NetworkConfig) (string, error) {
+	networkYipConfig := i.networkConfigurator.GetNetworkConfigApplicator(networkConfig)
+	networkConfigBytes, err := yaml.Marshal(networkYipConfig)
+	if err != nil {
+		return "", fmt.Errorf("marshalling network config: %w", err)
+	}
+	if err := i.fs.WriteFile(tempNetworkConfig, networkConfigBytes, 0600); err != nil {
+		return "", fmt.Errorf("writing file '%s': %w", tempNetworkConfig, err)
+	}
+	return tempNetworkConfig, nil
+}
+
 func (i *installer) getConnectionInfoBytes(config elementalv1.Elemental) ([]byte, error) {
 	kubeConfig := api.Config{
 		Kind:       "Config",