feat: bound all reconcile loops and network comms with context

Author: devigned

.dockerignore

@@ -9,6 +9,7 @@
 !/feature/**
 !/pkg/**
 !/version/**
+!/util/**
 !/main.go
 !/go.mod
 !/go.sum

.golangci.yml

@@ -1,5 +1,5 @@
 run:
-  deadline: 3m
+  deadline: 5m
   skip-dirs:
     - mock*
   skip-files:

cloud/scope/cluster.go

@@ -68,7 +68,6 @@ func NewClusterScope(params ClusterScopeParams) (*ClusterScope, error) {
 		Cluster:      params.Cluster,
 		AzureCluster: params.AzureCluster,
 		patchHelper:  helper,
-		Context:      context.Background(),
 	}, nil
 }
 
@@ -81,7 +80,6 @@ type ClusterScope struct {
 	AzureClients
 	Cluster      *clusterv1.Cluster
 	AzureCluster *infrav1.AzureCluster
-	Context      context.Context
 }
 
 // Network returns the cluster network object.
@@ -142,13 +140,13 @@ func (s *ClusterScope) ListOptionsLabelSelector() client.ListOption {
 }
 
 // PatchObject persists the cluster configuration and status.
-func (s *ClusterScope) PatchObject() error {
-	return s.patchHelper.Patch(context.TODO(), s.AzureCluster)
+func (s *ClusterScope) PatchObject(ctx context.Context) error {
+	return s.patchHelper.Patch(ctx, s.AzureCluster)
 }
 
 // Close closes the current scope persisting the cluster configuration and status.
-func (s *ClusterScope) Close() error {
-	return s.patchHelper.Patch(context.TODO(), s.AzureCluster)
+func (s *ClusterScope) Close(ctx context.Context) error {
+	return s.patchHelper.Patch(ctx, s.AzureCluster)
 }
 
 // AdditionalTags returns AdditionalTags from the scope's AzureCluster.

cloud/scope/machine.go

@@ -210,13 +210,13 @@ func (m *MachineScope) SetAddresses(addrs []corev1.NodeAddress) {
 }
 
 // PatchObject persists the machine spec and status.
-func (m *MachineScope) PatchObject() error {
-	return m.patchHelper.Patch(context.TODO(), m.AzureMachine)
+func (m *MachineScope) PatchObject(ctx context.Context) error {
+	return m.patchHelper.Patch(ctx, m.AzureMachine)
 }
 
 // Close the MachineScope by updating the machine spec, machine status.
-func (m *MachineScope) Close() error {
-	return m.patchHelper.Patch(context.TODO(), m.AzureMachine)
+func (m *MachineScope) Close(ctx context.Context) error {
+	return m.patchHelper.Patch(ctx, m.AzureMachine)
 }
 
 // AdditionalTags merges AdditionalTags from the scope's AzureCluster and AzureMachine. If the same key is present in both,
@@ -233,13 +233,13 @@ func (m *MachineScope) AdditionalTags() infrav1.Tags {
 }
 
 // GetBootstrapData returns the bootstrap data from the secret in the Machine's bootstrap.dataSecretName.
-func (m *MachineScope) GetBootstrapData() (string, error) {
+func (m *MachineScope) GetBootstrapData(ctx context.Context) (string, error) {
 	if m.Machine.Spec.Bootstrap.DataSecretName == nil {
 		return "", errors.New("error retrieving bootstrap data: linked Machine's bootstrap.dataSecretName is nil")
 	}
 	secret := &corev1.Secret{}
 	key := types.NamespacedName{Namespace: m.Namespace(), Name: *m.Machine.Spec.Bootstrap.DataSecretName}
-	if err := m.client.Get(context.TODO(), key, secret); err != nil {
+	if err := m.client.Get(ctx, key, secret); err != nil {
 		return "", errors.Wrapf(err, "failed to retrieve bootstrap data secret for AzureMachine %s/%s", m.Namespace(), m.Name())
 	}
 

cloud/scope/machinepool.go

@@ -152,9 +152,8 @@ func (m *MachinePoolScope) SetAnnotation(key, value string) {
 }
 
 // PatchObject persists the machine spec and status.
-func (m *MachinePoolScope) PatchObject() error {
-	// TODO[dj]: any function we are building where we are not adding context to the signature is welcoming unbound execution times. This needs to be addressed.
-	return m.patchHelper.Patch(context.TODO(), m.AzureMachinePool)
+func (m *MachinePoolScope) PatchObject(ctx context.Context) error {
+	return m.patchHelper.Patch(ctx, m.AzureMachinePool)
 }
 
 func (m *MachinePoolScope) AzureMachineTemplate(ctx context.Context) (*infrav1.AzureMachineTemplate, error) {
@@ -163,8 +162,8 @@ func (m *MachinePoolScope) AzureMachineTemplate(ctx context.Context) (*infrav1.A
 }
 
 // Close the MachineScope by updating the machine spec, machine status.
-func (m *MachinePoolScope) Close() error {
-	return m.patchHelper.Patch(context.TODO(), m.AzureMachinePool)
+func (m *MachinePoolScope) Close(ctx context.Context) error {
+	return m.patchHelper.Patch(ctx, m.AzureMachinePool)
 }
 
 func getAzureMachineTemplate(ctx context.Context, c client.Client, name, namespace string) (*infrav1.AzureMachineTemplate, error) {
@@ -177,14 +176,14 @@ func getAzureMachineTemplate(ctx context.Context, c client.Client, name, namespa
 }
 
 // GetBootstrapData returns the bootstrap data from the secret in the Machine's bootstrap.dataSecretName.
-func (m *MachinePoolScope) GetBootstrapData() (string, error) {
+func (m *MachinePoolScope) GetBootstrapData(ctx context.Context) (string, error) {
 	dataSecretName := m.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName
 	if dataSecretName == nil {
 		return "", errors.New("error retrieving bootstrap data: linked Machine Spec's bootstrap.dataSecretName is nil")
 	}
 	secret := &corev1.Secret{}
 	key := types.NamespacedName{Namespace: m.AzureMachinePool.Namespace, Name: *dataSecretName}
-	if err := m.client.Get(context.TODO(), key, secret); err != nil {
+	if err := m.client.Get(ctx, key, secret); err != nil {
 		return "", errors.Wrapf(err, "failed to retrieve bootstrap data secret for AzureMachinePool %s/%s", m.AzureMachinePool.Namespace, m.Name())
 	}
 

controllers/azurecluster_controller.go

@@ -26,6 +26,8 @@ import (
 	"k8s.io/client-go/tools/record"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
 	"sigs.k8s.io/cluster-api-provider-azure/cloud/scope"
+	"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
+
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -38,8 +40,9 @@ import (
 // AzureClusterReconciler reconciles a AzureCluster object
 type AzureClusterReconciler struct {
 	client.Client
-	Log      logr.Logger
-	Recorder record.EventRecorder
+	Log              logr.Logger
+	Recorder         record.EventRecorder
+	ReconcileTimeout time.Duration
 }
 
 func (r *AzureClusterReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
@@ -55,7 +58,8 @@ func (r *AzureClusterReconciler) SetupWithManager(mgr ctrl.Manager, options cont
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=azuremachinetemplates;azuremachinetemplates/status,verbs=get;list;watch
 
 func (r *AzureClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr error) {
-	ctx := context.TODO()
+	ctx, cancel := context.WithTimeout(context.Background(), reconciler.DefaultedLoopTimeout(r.ReconcileTimeout))
+	defer cancel()
 	log := r.Log.WithValues("namespace", req.Namespace, "AzureCluster", req.Name)
 
 	// Fetch the AzureCluster instance
@@ -93,33 +97,32 @@ func (r *AzureClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, ret
 
 	// Always close the scope when exiting this function so we can persist any AzureMachine changes.
 	defer func() {
-		if err := clusterScope.Close(); err != nil && reterr == nil {
+		if err := clusterScope.Close(ctx); err != nil && reterr == nil {
 			reterr = err
 		}
 	}()
 
 	// Handle deleted clusters
 	if !azureCluster.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(clusterScope)
+		return r.reconcileDelete(ctx, clusterScope)
 	}
 
 	// Handle non-deleted clusters
-	return r.reconcileNormal(clusterScope)
+	return r.reconcileNormal(ctx, clusterScope)
 }
 
-func (r *AzureClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScope) (reconcile.Result, error) {
+func (r *AzureClusterReconciler) reconcileNormal(ctx context.Context, clusterScope *scope.ClusterScope) (reconcile.Result, error) {
 	clusterScope.Info("Reconciling AzureCluster")
-
 	azureCluster := clusterScope.AzureCluster
 
 	// If the AzureCluster doesn't have our finalizer, add it.
 	controllerutil.AddFinalizer(azureCluster, infrav1.ClusterFinalizer)
 	// Register the finalizer immediately to avoid orphaning Azure resources on delete
-	if err := clusterScope.PatchObject(); err != nil {
+	if err := clusterScope.PatchObject(ctx); err != nil {
 		return reconcile.Result{}, err
 	}
 
-	err := newAzureClusterReconciler(clusterScope).Reconcile()
+	err := newAzureClusterReconciler(clusterScope).Reconcile(ctx)
 	if err != nil {
 		return reconcile.Result{}, errors.Wrap(err, "failed to reconcile cluster services")
 	}
@@ -141,12 +144,12 @@ func (r *AzureClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScop
 	return reconcile.Result{}, nil
 }
 
-func (r *AzureClusterReconciler) reconcileDelete(clusterScope *scope.ClusterScope) (reconcile.Result, error) {
+func (r *AzureClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (reconcile.Result, error) {
 	clusterScope.Info("Reconciling AzureCluster delete")
 
 	azureCluster := clusterScope.AzureCluster
 
-	if err := newAzureClusterReconciler(clusterScope).Delete(); err != nil {
+	if err := newAzureClusterReconciler(clusterScope).Delete(ctx); err != nil {
 		return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureCluster %s/%s", azureCluster.Namespace, azureCluster.Name)
 	}
 

controllers/azurecluster_controller_test.go

@@ -17,12 +17,19 @@ limitations under the License.
 package controllers
 
 import (
+	"time"
+
+	"github.com/go-logr/logr"
+	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
 	"golang.org/x/net/context"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
+	"sigs.k8s.io/cluster-api-provider-azure/internal/test/mock_log"
+
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -56,5 +63,29 @@ var _ = Describe("AzureClusterReconciler", func() {
 			Expect(err).To(BeNil())
 			Expect(result.RequeueAfter).To(BeZero())
 		})
+
+		It("should fail with context timeout error if context expires", func() {
+			log := mock_log.NewMockLogger(gomock.NewController(GinkgoT()))
+			log.EXPECT().WithValues(gomock.Any()).DoAndReturn(func(args ...interface{}) logr.Logger {
+				time.Sleep(1 * time.Second)
+				return log
+			})
+
+			reconciler := &AzureClusterReconciler{
+				Client:           k8sClient,
+				Log:              log,
+				ReconcileTimeout: 1 * time.Second,
+			}
+
+			instance := &infrav1.AzureCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
+			_, err := reconciler.Reconcile(ctrl.Request{
+				NamespacedName: client.ObjectKey{
+					Namespace: instance.Namespace,
+					Name:      instance.Name,
+				},
+			})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Or(Equal("context deadline exceeded"), Equal("rate: Wait(n=1) would exceed context deadline")))
+		})
 	})
 })