From 46e8990c9c6e115000f1f16dba835f92d457d18a Mon Sep 17 00:00:00 2001 From: Ashley Dumaine Date: Thu, 7 Mar 2024 16:10:33 -0500 Subject: [PATCH] refactoring to reduce cyclomatic complexity in machine controller --- .golangci.yml | 4 +- api/v1alpha1/zz_generated.deepcopy.go | 5 + cloud/services/loadbalancers.go | 5 +- ...cture.cluster.x-k8s.io_linodemachines.yaml | 6 +- ...uster.x-k8s.io_linodemachinetemplates.yaml | 6 +- controller/linodemachine_controller.go | 114 ++---------------- .../linodemachine_controller_helpers.go | 86 ++++++++++++- 7 files changed, 111 insertions(+), 115 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 9dfcd57a0..827195f44 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -51,7 +51,7 @@ linters-settings: local-prefixes: github.com/crossplane/provider-template cyclop: - max-complexity: 17 + max-complexity: 15 maligned: # print struct with more effective memory layout or not, false by default @@ -168,7 +168,7 @@ linters: - nestif - nilerr - nilnil - # - nlreturn + - nlreturn - noctx - nolintlint - paralleltest diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 8b8680438..9ffa79e3b 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -387,6 +387,11 @@ func (in *LinodeMachineSpec) DeepCopyInto(out *LinodeMachineSpec) { *out = new(InstanceMetadataOptions) **out = **in } + if in.CredentialsRef != nil { + in, out := &in.CredentialsRef, &out.CredentialsRef + *out = new(v1.SecretReference) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LinodeMachineSpec. diff --git a/cloud/services/loadbalancers.go b/cloud/services/loadbalancers.go index e7123db46..a281ed7bf 100644 --- a/cloud/services/loadbalancers.go +++ b/cloud/services/loadbalancers.go @@ -111,7 +111,6 @@ func AddNodeToNB( ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope, - clusterScope *scope.ClusterScope, ) error { // Update the NB backend with the new instance if it's a control plane node if !kutil.IsControlPlaneMachine(machineScope.Machine) { @@ -132,8 +131,8 @@ func AddNodeToNB( } lbPort := defaultLBPort - if clusterScope.LinodeCluster.Spec.Network.LoadBalancerPort != 0 { - lbPort = clusterScope.LinodeCluster.Spec.Network.LoadBalancerPort + if machineScope.LinodeCluster.Spec.Network.LoadBalancerPort != 0 { + lbPort = machineScope.LinodeCluster.Spec.Network.LoadBalancerPort } if machineScope.LinodeCluster.Spec.Network.NodeBalancerConfigID == nil { err := errors.New("nil NodeBalancer Config ID") diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml index 6d340bc22..698b7ded4 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml @@ -93,9 +93,9 @@ spec: CredentialsRef is a reference to a Secret that contains the credentials to use for provisioning this machine. If not supplied then these credentials will be used in-order: - 1. Machine - 2. Cluster - 2. Controller + 1. LinodeMachine + 2. Owner LinodeCluster + 3. Controller properties: name: description: name is unique within a namespace to reference a diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml index 41e9977ee..739461352 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml @@ -80,9 +80,9 @@ spec: CredentialsRef is a reference to a Secret that contains the credentials to use for provisioning this machine. If not supplied then these credentials will be used in-order: - 1. Machine - 2. Cluster - 2. Controller + 1. LinodeMachine + 2. Owner LinodeCluster + 3. Controller properties: name: description: name is unique within a namespace to reference diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 759fac361..6bb3fc254 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -103,68 +103,23 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques linodeMachine := &infrav1alpha1.LinodeMachine{} if err := r.Client.Get(ctx, req.NamespacedName, linodeMachine); err != nil { if err = client.IgnoreNotFound(err); err != nil { - log.Error(err, "Failed to fetch Linode machine") + log.Error(err, "Failed to fetch LinodeMachine") } return ctrl.Result{}, err } - machine, err := kutil.GetOwnerMachine(ctx, r.Client, linodeMachine.ObjectMeta) - switch { - case err != nil: - if err = client.IgnoreNotFound(err); err != nil { - log.Error(err, "Failed to fetch owner machine") - } - + machine, err := r.getOwnerMachine(ctx, *linodeMachine, log) + if err != nil || machine == nil { return ctrl.Result{}, err - case machine == nil: - log.Info("Machine Controller has not yet set OwnerRef, skipping reconciliation") - - return ctrl.Result{}, nil - case skippedMachinePhases[machine.Status.Phase]: - - return ctrl.Result{}, nil - default: - match := false - for i := range linodeMachine.OwnerReferences { - if match = linodeMachine.OwnerReferences[i].UID == machine.UID; match { - break - } - } - - if !match { - log.Info("Failed to find the referenced owner machine, skipping reconciliation", "references", linodeMachine.OwnerReferences, "machine", machine.ObjectMeta) - - return ctrl.Result{}, nil - } } + log = log.WithValues("LinodeMachine: ", machine.Name) - log = log.WithValues("Linode machine: ", machine.Name) - - cluster, err := kutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) - switch { - case err != nil: - if err = client.IgnoreNotFound(err); err != nil { - log.Error(err, "Failed to fetch cluster by label") - } - - return ctrl.Result{}, err - case cluster == nil: - err = errors.New("missing cluster") - - log.Error(err, "Missing cluster") - - return ctrl.Result{}, err - case cluster.Spec.InfrastructureRef == nil: - err = errors.New("missing infrastructure reference") - - log.Error(err, "Missing infrastructure reference") - + cluster, err := r.getClusterFromMetadata(ctx, *machine, log) + if err != nil || cluster == nil { return ctrl.Result{}, err } - linodeCluster := &infrav1alpha1.LinodeCluster{} - machineScope, err := scope.NewMachineScope( ctx, r.LinodeApiKey, @@ -172,7 +127,7 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques Client: r.Client, Cluster: cluster, Machine: machine, - LinodeCluster: linodeCluster, + LinodeCluster: &infrav1alpha1.LinodeCluster{}, LinodeMachine: linodeMachine, }, ) @@ -182,29 +137,13 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, fmt.Errorf("failed to create machine scope: %w", err) } - clusterScope, err := scope.NewClusterScope( - ctx, - r.LinodeApiKey, - scope.ClusterScopeParams{ - Client: r.Client, - Cluster: cluster, - LinodeCluster: linodeCluster, - }, - ) - if err != nil { - log.Error(err, "Failed to create cluster scope") - - return ctrl.Result{}, fmt.Errorf("failed to create cluster scope: %w", err) - } - - return r.reconcile(ctx, log, machineScope, clusterScope) + return r.reconcile(ctx, log, machineScope) } func (r *LinodeMachineReconciler) reconcile( ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope, - clusterScope *scope.ClusterScope, ) (res ctrl.Result, err error) { res = ctrl.Result{} @@ -290,7 +229,7 @@ func (r *LinodeMachineReconciler) reconcile( return } - linodeInstance, err = r.reconcileCreate(ctx, logger, machineScope, clusterScope) + linodeInstance, err = r.reconcileCreate(ctx, logger, machineScope) return } @@ -299,7 +238,6 @@ func (r *LinodeMachineReconciler) reconcileCreate( ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope, - clusterScope *scope.ClusterScope, ) (*linodego.Instance, error) { logger.Info("creating machine") @@ -332,17 +270,6 @@ func (r *LinodeMachineReconciler) reconcileCreate( return nil, err } - if machineScope.LinodeCluster.Spec.VPCRef != nil { - iface, err := r.getVPCInterfaceConfig(ctx, machineScope, createOpts.Interfaces, logger) - if err != nil { - logger.Error(err, "Failed to get VPC interface config") - - return nil, err - } - - createOpts.Interfaces = append(createOpts.Interfaces, *iface) - } - linodeInstance, err = machineScope.LinodeClient.CreateInstance(ctx, *createOpts) if err != nil { logger.Error(err, "Failed to create Linode machine instance") @@ -370,31 +297,12 @@ func (r *LinodeMachineReconciler) reconcileCreate( return nil, err } - if linodeInstance == nil { - err = errors.New("missing instance") - - logger.Error(err, "Panic! Failed to create isntance") - - return nil, err - } - machineScope.LinodeMachine.Status.Ready = true machineScope.LinodeMachine.Spec.InstanceID = &linodeInstance.ID machineScope.LinodeMachine.Spec.ProviderID = util.Pointer(fmt.Sprintf("linode://%d", linodeInstance.ID)) + machineScope.LinodeMachine.Status.Addresses = buildInstanceAddrs(linodeInstance) - machineScope.LinodeMachine.Status.Addresses = []clusterv1.MachineAddress{} - for _, addr := range linodeInstance.IPv4 { - addrType := clusterv1.MachineExternalIP - if addr.IsPrivate() { - addrType = clusterv1.MachineInternalIP - } - machineScope.LinodeMachine.Status.Addresses = append(machineScope.LinodeMachine.Status.Addresses, clusterv1.MachineAddress{ - Type: addrType, - Address: addr.String(), - }) - } - - if err = services.AddNodeToNB(ctx, logger, machineScope, clusterScope); err != nil { + if err = services.AddNodeToNB(ctx, logger, machineScope); err != nil { logger.Error(err, "Failed to add instance to Node Balancer backend") return linodeInstance, err diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go index b9a5bac6a..3fbfbf4de 100644 --- a/controller/linodemachine_controller_helpers.go +++ b/controller/linodemachine_controller_helpers.go @@ -46,7 +46,7 @@ import ( // The decoded user_data must not exceed 16384 bytes per the Linode API const maxBootstrapDataBytes = 16384 -func (*LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, tags []string, logger logr.Logger) (*linodego.InstanceCreateOptions, error) { +func (r *LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, tags []string, logger logr.Logger) (*linodego.InstanceCreateOptions, error) { var err error createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope.LinodeMachine.Spec) @@ -94,9 +94,93 @@ func (*LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScop createConfig.RootPass = uuid.NewString() } + if machineScope.LinodeCluster.Spec.VPCRef != nil { + iface, err := r.getVPCInterfaceConfig(ctx, machineScope, createConfig.Interfaces, logger) + if err != nil { + logger.Error(err, "Failed to get VPC interface config") + + return nil, err + } + createConfig.Interfaces = append(createConfig.Interfaces, *iface) + } + return createConfig, nil } +func buildInstanceAddrs(linodeInstance *linodego.Instance) []clusterv1.MachineAddress { + addrs := []clusterv1.MachineAddress{} + for _, addr := range linodeInstance.IPv4 { + addrType := clusterv1.MachineExternalIP + if addr.IsPrivate() { + addrType = clusterv1.MachineInternalIP + } + addrs = append(addrs, clusterv1.MachineAddress{ + Type: addrType, + Address: addr.String(), + }) + } + + return addrs +} + +func (r *LinodeMachineReconciler) getOwnerMachine(ctx context.Context, linodeMachine infrav1alpha1.LinodeMachine, log logr.Logger) (*clusterv1.Machine, error) { + machine, err := kutil.GetOwnerMachine(ctx, r.Client, linodeMachine.ObjectMeta) + switch { + case err != nil: + if err = client.IgnoreNotFound(err); err != nil { + log.Error(err, "Failed to fetch owner machine") + } + + return nil, err + case machine == nil: + log.Info("Machine Controller has not yet set OwnerRef, skipping reconciliation") + + return nil, err + case skippedMachinePhases[machine.Status.Phase]: + + return nil, err + default: + match := false + for i := range linodeMachine.OwnerReferences { + if match = linodeMachine.OwnerReferences[i].UID == machine.UID; match { + break + } + } + if !match { + log.Info("Failed to find the referenced owner machine, skipping reconciliation", "references", linodeMachine.OwnerReferences, "machine", machine.ObjectMeta) + + return nil, err + } + } + + return machine, nil +} + +func (r *LinodeMachineReconciler) getClusterFromMetadata(ctx context.Context, machine clusterv1.Machine, log logr.Logger) (*clusterv1.Cluster, error) { + cluster, err := kutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) + switch { + case err != nil: + if err = client.IgnoreNotFound(err); err != nil { + log.Error(err, "Failed to fetch cluster by label") + } + + return nil, err + case cluster == nil: + err = errors.New("missing cluster") + log.Error(err, "Missing cluster") + + return nil, err + case cluster.Spec.InfrastructureRef == nil: + err = errors.New("missing infrastructure reference") + + log.Error(err, "Missing infrastructure reference") + + return nil, err + } + + return cluster, nil +} + func (r *LinodeMachineReconciler) linodeClusterToLinodeMachines(logger logr.Logger) handler.MapFunc { logger = logger.WithName("LinodeMachineReconciler").WithName("linodeClusterToLinodeMachines")