Call coding/decoding methods directly, drop yaml encoding

Drop the actuator's codec interface and inject the aws codec data type directly. Do not run coding/decoding through utils functions that are just fency wrappers.
openshift · Dec 3, 2018 · 8ea3d24 · 8ea3d24
1 parent fb5c960
commit 8ea3d24
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 51 deletions.
diff --git a/pkg/apis/awsproviderconfig/v1alpha1/register.go b/pkg/apis/awsproviderconfig/v1alpha1/register.go
@@ -27,6 +27,7 @@ package v1alpha1
 import (
 	"bytes"
 	"fmt"
+
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -103,7 +104,7 @@ func (codec *AWSProviderConfigCodec) EncodeProviderStatus(in runtime.Object) (*r
 	return &runtime.RawExtension{Raw: buf.Bytes()}, nil
 }
 
-// DecodeProviderStatus serialises the provider status
+// DecodeProviderStatus deserialises the provider status
 func (codec *AWSProviderConfigCodec) DecodeProviderStatus(providerStatus *runtime.RawExtension, out runtime.Object) error {
 	if providerStatus != nil {
 		_, _, err := codec.decoder.Decode(providerStatus.Raw, nil, out)

diff --git a/pkg/cloud/aws/actuators/machine/actuator.go b/pkg/cloud/aws/actuators/machine/actuator.go
@@ -37,7 +37,6 @@ import (
 
 	"github.com/aws/aws-sdk-go/service/ec2"
 
-	"k8s.io/apimachinery/pkg/runtime"
 	awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/client"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -62,25 +61,20 @@ type Actuator struct {
 	kubeClient       kubernetes.Interface
 	client           client.Client
 	awsClientBuilder awsclient.AwsClientBuilderFuncType
-	codec            codec
-	eventRecorder    record.EventRecorder
+
+	codec         *providerconfigv1.AWSProviderConfigCodec
+	eventRecorder record.EventRecorder
 }
 
 // ActuatorParams holds parameter information for Actuator
 type ActuatorParams struct {
 	KubeClient       kubernetes.Interface
 	Client           client.Client
 	AwsClientBuilder awsclient.AwsClientBuilderFuncType
-	Codec            codec
+	Codec            *providerconfigv1.AWSProviderConfigCodec
 	EventRecorder    record.EventRecorder
 }
 
-type codec interface {
-	DecodeProviderConfig(*clusterv1.ProviderConfig, runtime.Object) error
-	DecodeProviderStatus(*runtime.RawExtension, runtime.Object) error
-	EncodeProviderStatus(runtime.Object) (*runtime.RawExtension, error)
-}
-
 // NewActuator returns a new AWS Actuator
 func NewActuator(params ActuatorParams) (*Actuator, error) {
 	actuator := &Actuator{
@@ -126,7 +120,7 @@ func (a *Actuator) Create(context context.Context, cluster *clusterv1.Cluster, m
 }
 
 func (a *Actuator) updateMachineStatus(machine *clusterv1.Machine, awsStatus *providerconfigv1.AWSMachineProviderStatus, networkAddresses []corev1.NodeAddress) error {
-	awsStatusRaw, err := EncodeProviderStatus(a.codec, awsStatus)
+	awsStatusRaw, err := a.codec.EncodeProviderStatus(awsStatus)
 	if err != nil {
 		glog.Errorf("error encoding AWS provider status: %v", err)
 		return err
@@ -137,11 +131,13 @@ func (a *Actuator) updateMachineStatus(machine *clusterv1.Machine, awsStatus *pr
 	if networkAddresses != nil {
 		machineCopy.Status.Addresses = networkAddresses
 	}
-	oldAWSStatus, err := ProviderStatusFromMachine(a.codec, machine)
-	if err != nil {
+
+	oldAWSStatus := &providerconfigv1.AWSMachineProviderStatus{}
+	if err := a.codec.DecodeProviderStatus(machine.Status.ProviderStatus, oldAWSStatus); err != nil {
 		glog.Errorf("error updating machine status: %v", err)
 		return err
 	}
+
 	// TODO(vikasc): Revisit to compare complete machine status objects
 	if !equality.Semantic.DeepEqual(awsStatus, oldAWSStatus) || !equality.Semantic.DeepEqual(machine.Status.Addresses, machineCopy.Status.Addresses) {
 		glog.Infof("machine status has changed, updating")
@@ -164,16 +160,15 @@ func (a *Actuator) updateMachineProviderConditions(machine *clusterv1.Machine, c
 
 	glog.Info("updating machine conditions")
 
-	awsStatus, err := ProviderStatusFromMachine(a.codec, machine)
-	if err != nil {
+	awsStatus := &providerconfigv1.AWSMachineProviderStatus{}
+	if err := a.codec.DecodeProviderStatus(machine.Status.ProviderStatus, awsStatus); err != nil {
 		glog.Errorf("error decoding machine provider status: %v", err)
 		return err
 	}
 
 	awsStatus.Conditions = SetAWSMachineProviderCondition(awsStatus.Conditions, conditionType, corev1.ConditionTrue, reason, msg, UpdateConditionIfReasonOrMessageChange)
 
-	err = a.updateMachineStatus(machine, awsStatus, nil)
-	if err != nil {
+	if err := a.updateMachineStatus(machine, awsStatus, nil); err != nil {
 		return err
 	}
 
@@ -182,7 +177,7 @@ func (a *Actuator) updateMachineProviderConditions(machine *clusterv1.Machine, c
 
 // CreateMachine starts a new AWS instance as described by the cluster and machine resources
 func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1.Machine) (*ec2.Instance, error) {
-	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine)
+	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine, a.codec)
 	if err != nil {
 		return nil, a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), createEventAction)
 	}
@@ -246,7 +241,7 @@ func (a *Actuator) Delete(context context.Context, cluster *clusterv1.Cluster, m
 
 // DeleteMachine deletes an AWS instance
 func (a *Actuator) DeleteMachine(cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
-	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine)
+	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine, a.codec)
 	if err != nil {
 		return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), deleteEventAction)
 	}
@@ -287,7 +282,7 @@ func (a *Actuator) DeleteMachine(cluster *clusterv1.Cluster, machine *clusterv1.
 func (a *Actuator) Update(context context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
 	glog.Info("updating machine")
 
-	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine)
+	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine, a.codec)
 	if err != nil {
 		return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), noEventAction)
 	}
@@ -390,7 +385,7 @@ func (a *Actuator) Describe(cluster *clusterv1.Cluster, machine *clusterv1.Machi
 }
 
 func (a *Actuator) getMachineInstances(cluster *clusterv1.Cluster, machine *clusterv1.Machine) ([]*ec2.Instance, error) {
-	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine)
+	machineProviderConfig, err := ProviderConfigFromMachine(a.client, machine, a.codec)
 	if err != nil {
 		glog.Errorf("error decoding MachineProviderConfig: %v", err)
 		return nil, err
@@ -455,11 +450,12 @@ func (a *Actuator) updateStatus(machine *clusterv1.Machine, instance *ec2.Instan
 	glog.Info("updating status")
 
 	// Starting with a fresh status as we assume full control of it here.
-	awsStatus, err := ProviderStatusFromMachine(a.codec, machine)
-	if err != nil {
+	awsStatus := &providerconfigv1.AWSMachineProviderStatus{}
+	if err := a.codec.DecodeProviderStatus(machine.Status.ProviderStatus, awsStatus); err != nil {
 		glog.Errorf("error decoding machine provider status: %v", err)
 		return err
 	}
+
 	// Save this, we need to check if it changed later.
 	networkAddresses := []corev1.NodeAddress{}
 
@@ -505,8 +501,7 @@ func (a *Actuator) updateStatus(machine *clusterv1.Machine, instance *ec2.Instan
 	// 	awsStatus.LastELBSync = nil
 	// }
 
-	err = a.updateMachineStatus(machine, awsStatus, networkAddresses)
-	if err != nil {
+	if err := a.updateMachineStatus(machine, awsStatus, networkAddresses); err != nil {
 		return err
 	}
 

diff --git a/pkg/cloud/aws/actuators/machine/actuator_test.go b/pkg/cloud/aws/actuators/machine/actuator_test.go
@@ -239,9 +239,10 @@ func TestCreateAndDeleteMachine(t *testing.T) {
 			if err != nil {
 				t.Fatalf("error creating codec: %v", err)
 			}
-			machineStatus, err := ProviderStatusFromMachine(codec, &updatedMachine)
-			if err != nil {
-				t.Fatalf("error getting machineStatus: %v", err)
+
+			machineStatus := &providerconfigv1.AWSMachineProviderStatus{}
+			if err := codec.DecodeProviderStatus(updatedMachine.Status.ProviderStatus, machineStatus); err != nil {
+				t.Fatalf("error decoding machine provider status: %v", err)
 			}
 
 			if tc.createErrorExpected {

diff --git a/pkg/cloud/aws/actuators/machine/utils.go b/pkg/cloud/aws/actuators/machine/utils.go
@@ -19,16 +19,14 @@ package machine
 import (
 	"fmt"
 
-	"k8s.io/apimachinery/pkg/runtime"
-
 	"github.com/golang/glog"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	"github.com/ghodss/yaml"
 	"golang.org/x/net/context"
 	"k8s.io/apimachinery/pkg/types"
 	providerconfigv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1alpha1"
@@ -140,7 +138,7 @@ func terminateInstances(client awsclient.Client, instances []*ec2.Instance) erro
 
 // ProviderConfigFromMachine gets the machine provider config MachineSetSpec from the
 // specified cluster-api MachineSpec.
-func ProviderConfigFromMachine(client client.Client, machine *clusterv1.Machine) (*providerconfigv1.AWSMachineProviderConfig, error) {
+func ProviderConfigFromMachine(client client.Client, machine *clusterv1.Machine, codec *providerconfigv1.AWSProviderConfigCodec) (*providerconfigv1.AWSMachineProviderConfig, error) {
 	var providerConfig runtime.RawExtension
 
 	if machine.Spec.ProviderConfig.Value == nil && machine.Spec.ProviderConfig.ValueFrom == nil {
@@ -166,24 +164,12 @@ func ProviderConfigFromMachine(client client.Client, machine *clusterv1.Machine)
 	}
 
 	var config providerconfigv1.AWSMachineProviderConfig
-	if err := yaml.Unmarshal(providerConfig.Raw, &config); err != nil {
+	if err := codec.DecodeProviderConfig(&clusterv1.ProviderConfig{Value: &providerConfig}, &config); err != nil {
 		return nil, err
 	}
 	return &config, nil
 }
 
-// ProviderStatusFromMachine gets the machine provider status from the specified machine.
-func ProviderStatusFromMachine(codec codec, m *clusterv1.Machine) (*providerconfigv1.AWSMachineProviderStatus, error) {
-	status := &providerconfigv1.AWSMachineProviderStatus{}
-	err := codec.DecodeProviderStatus(m.Status.ProviderStatus, status)
-	return status, err
-}
-
-// EncodeProviderStatus encodes the machine status into RawExtension
-func EncodeProviderStatus(codec codec, awsStatus *providerconfigv1.AWSMachineProviderStatus) (*runtime.RawExtension, error) {
-	return codec.EncodeProviderStatus(awsStatus)
-}
-
 // IsMaster returns true if the machine is part of a cluster's control plane
 func IsMaster(machine *clusterv1.Machine) bool {
 	if machineType, exists := machine.ObjectMeta.Labels[providerconfigv1.MachineTypeLabel]; exists && machineType == "master" {

diff --git a/pkg/cloud/aws/actuators/machine/utils_test.go b/pkg/cloud/aws/actuators/machine/utils_test.go
@@ -1,14 +1,15 @@
 package machine
 
 import (
+	"reflect"
+	"testing"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
-	"reflect"
 	providerconfigv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1alpha1"
 	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"testing"
 )
 
 func init() {
@@ -113,7 +114,7 @@ func TestProviderConfigFromMachine(t *testing.T) {
 
 	client := fake.NewFakeClient(machineClass)
 	for _, tc := range testCases {
-		decodedProviderConfig, err := ProviderConfigFromMachine(client, tc.machine)
+		decodedProviderConfig, err := ProviderConfigFromMachine(client, tc.machine, codec)
 		if err != nil {
 			t.Error(err)
 		}

diff --git a/test/machines/utils.go b/test/machines/utils.go
@@ -10,7 +10,6 @@ import (
 	"github.com/openshift/cluster-api-actuator-pkg/pkg/e2e/framework"
 
 	providerconfigv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1alpha1"
-	machineutils "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/actuators/machine"
 )
 
 func createSecretAndWait(f *framework.Framework, secret *apiv1.Secret) {
@@ -33,7 +32,8 @@ func getMachineProviderStatus(f *framework.Framework, machine *clusterv1alpha1.M
 	codec, err := providerconfigv1.NewCodec()
 	Expect(err).NotTo(HaveOccurred())
 
-	machineProviderStatus, err := machineutils.ProviderStatusFromMachine(codec, machine)
+	machineProviderStatus := &providerconfigv1.AWSMachineProviderStatus{}
+	err = codec.DecodeProviderStatus(machine.Status.ProviderStatus, machineProviderStatus)
 	Expect(err).NotTo(HaveOccurred())
 
 	return machineProviderStatus