fix: we were in a reconcile loop because we misunderstood how ctrl.re…

…sult{} worked (#45)

.github/workflows/build.yaml

@@ -20,14 +20,3 @@ jobs:
 
       - name: Build Docker Image
         run: make docker-build
-
-      - name: Convert coverage to lcov
-        uses: jandelgado/gcov2lcov-action@v1
-        with:
-          infile: cover.out
-
-      - name: Coveralls GitHub Action
-        uses: coverallsapp/[email protected]
-        with:
-          github-token: ${{ secrets.github_token }}
-          path-to-lcov: coverage.lcov

.github/workflows/test.yaml

@@ -23,3 +23,14 @@ jobs:
 
       - name: Report coverage
         run: make cover
+
+      - name: Convert coverage to lcov
+        uses: jandelgado/gcov2lcov-action@v1
+        with:
+          infile: cover.out
+
+      - name: Coveralls GitHub Action
+        uses: coverallsapp/[email protected]
+        with:
+          github-token: ${{ secrets.github_token }}
+          path-to-lcov: coverage.lcov

Makefile

@@ -110,7 +110,7 @@ test: manifests generate fmt vet envtest ## Run tests.
 ##@ Build
 
 .PHONY: build
-build: generate fmt vet cli ## Build manager binary.
+build: generate fmt vet ## Build manager binary.
 	go build -o bin/manager main.go
 
 .PHONY: clean
@@ -125,7 +125,7 @@ run: manifests generate fmt vet ## Run a controller from your host.
 # (i.e. docker build --platform linux/arm64 ). However, you must enable docker buildKit for it.
 # More info: https://docs.docker.com/develop/develop-images/build_enhancements/
 .PHONY: docker-build
-docker-build: test ## Build docker image with the manager.
+docker-build: ## Build docker image with the manager.
 	docker build -t $(IMG) .
 
 .PHONY: docker-push

controllers/builders/pod_access_builder.go

@@ -105,7 +105,7 @@ func (b *PodAccessBuilder) VerifyAccessResources() (statusString string, err err
 
 	// Next, get the Pod. If the pod-get fails, then we need to return that failure.
 	pod := &corev1.Pod{}
-	err = b.Client.Get(b.Ctx, types.NamespacedName{
+	err = b.APIReader.Get(b.Ctx, types.NamespacedName{
 		Name:      b.Request.Status.PodName,
 		Namespace: b.Request.Namespace,
 	}, pod)

controllers/const.go

@@ -10,10 +10,6 @@ const (
 	// checks of the target resources that our controllers are managing.
 	DefaultReconciliationInterval int = 5
 
-	// ErrorReconciliationInterval defines how long (in seconds) in between a failed reconciliation
-	// loop before the next one should kick off.
-	ErrorReconciliationInterval int = 30
-
 	// PodWaitReconciliationInterval is how long between attemps to check
 	// whether or not a Target Pod has come up.
 	PodWaitReconciliationInterval int = 5

controllers/exec_access_request_controller.go

@@ -131,9 +131,7 @@ func (r *ExecAccessRequestReconciler) Reconcile(
 	// set up a 30 second delay before the next reconciliation attempt.
 	_, err = r.verifyAccessResources(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// FINAL: Set Status.Ready state

controllers/exec_access_template_controller.go

@@ -81,17 +81,13 @@ func (r *ExecAccessTemplateReconciler) Reconcile(
 	// VERIFICATION: Make sure that the TargetRef is valid and points to an active controller
 	err = r.VerifyTargetRef(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// VERIFICATION: Make sure the DefaultDuration and MaxDuration settings are valid
 	err = r.VerifyMiscSettings(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// TODO:
@@ -100,9 +96,7 @@ func (r *ExecAccessTemplateReconciler) Reconcile(
 	// FINAL: Set Status.Ready state
 	err = r.setReadyStatus(ctx, resource)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{
 		RequeueAfter: time.Duration(r.ReconcililationInterval * int(time.Minute)),

controllers/pod_access_request_controller.go

@@ -113,9 +113,7 @@ func (r *PodAccessRequestReconciler) Reconcile(
 	// VERIFICATION: Verifies the requested duration
 	err = r.verifyDuration(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// VERIFICATION: Handle whether or not the access is expired at this point! If so, delete it.
@@ -129,25 +127,32 @@ func (r *PodAccessRequestReconciler) Reconcile(
 	// set up a 30 second delay before the next reconciliation attempt.
 	_, err = r.verifyAccessResources(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// VERIFICATION: Make sure the access resources (pod) are actually up and ready
 	_, err = r.verifyAccessResourcesReady(builder)
 	if err != nil {
+		// SPECIAL TREATMENT: An error here means, requeue this and run it
+		// again in few seconds while we wait for the Pod to come up. It does
+		// not necessarily mean a real failure happened.
+		//
+		// The requeue logic of the requeueAfter setting only takes effect if
+		// err==nil:
+		//
+		//   https://github.com/kubernetes-sigs/controller-runtime/blob/053233652960f536727e1980eb0ad8ceb9bef096/pkg/internal/controller/controller.go#L214-L235
+		//
+		// TODO: Check the error type. If it's a non-failure, then be more
+		// intelligent
 		return ctrl.Result{
 			RequeueAfter: time.Duration(time.Duration(PodWaitReconciliationInterval) * time.Second),
-		}, err
+		}, nil
 	}
 
 	// FINAL: Set Status.Ready state
 	err = r.setReadyStatus(ctx, resource)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// Exit Reconciliation Loop

controllers/pod_access_request_controller_test.go

@@ -173,9 +173,11 @@ var _ = Describe("PodAccessRequestController", Ordered, func() {
 					Namespace: request.Namespace,
 				},
 			})
-			// We should have errored out with a "Pod in Pending" phase
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(Equal("Pod in Pending Phase"))
+			Expect(err).To(Not(HaveOccurred()))
+
+			// Verify that the Request is still in not-ready state, even though
+			// the reconcile didn't error out.
+			Expect(request.Status.IsReady()).To(BeFalse())
 
 			// Refetch our Request object... reconiliation has mutated its
 			// .Status fields.

controllers/pod_access_template_controller.go

@@ -81,17 +81,13 @@ func (r *PodAccessTemplateReconciler) Reconcile(
 	// VERIFICATION: Make sure that the TargetRef is valid and points to an active controller
 	err = r.VerifyTargetRef(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// VERIFICATION: Make sure the DefaultDuration and MaxDuration settings are valid
 	err = r.VerifyMiscSettings(builder)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 
 	// // TODO:
@@ -100,9 +96,7 @@ func (r *PodAccessTemplateReconciler) Reconcile(
 	// FINAL: Set Status.Ready state
 	err = r.setReadyStatus(ctx, resource)
 	if err != nil {
-		return ctrl.Result{
-			RequeueAfter: time.Duration(time.Duration(ErrorReconciliationInterval) * time.Second),
-		}, err
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{
 		RequeueAfter: time.Duration(r.ReconcililationInterval * int(time.Minute)),