Repository: argoproj-labs/argo-rollouts-manager (go)

Issue:
reflect.DeepEqual does not work well when comparing expected Rollouts Deployment resource and live Rollouts Deployment resource
We have logic in `deployment.go` which attempts to compare the expected (target) contents of the Argo Rollouts `Deployment` resource with the live on-cluster contents of that resource, and iff they differ it will update the live copy.

However, at present, that logic will *always* cause an Update, because `reflect.DeepEqual()` is matching fields with default values that are set in the live object, but not set in the expected object.


Pull request description:
Improve logic used to detect changes in Deployment resource
**What does this PR do / why we need it**:
- Deployment changes
    - Update the logic used to detect Deployment changes. See comment on 'normalizeDeployment' for details. TL;DR: to normalize a Deployment for comparison, we create a new Deployment struct, and copy in only the fields we care about (from the un-normalized Deployment).
    - Unit tests for these changes

- Related E2E test changes:
    - Move operator e2e test logic into shell script
    - Operator E2E tests should fail if the operator itself has unexpected ERROR log entries: added a check for this
    - Ability to set SKIP_RUN_STEP to skip starting operator in upstream rollouts test
    - Improve logic of 'waitAndGetE2EFileContents' to now check last 50 lines, rather than last 1 line 

**Have you updated the necessary documentation?**

* [ ] Documentation update is required by this PR, and has been updated.

**Which issue(s) this PR fixes**:
Fixes #47

<!-- This must link to a GitHub issue. If one does not exist, create one. -->

**How to test changes / Special notes to the reviewer**:


Patch:
diff --git a/Makefile b/Makefile
index 40bd59a..1924da8 100644
--- a/Makefile
+++ b/Makefile
@@ -107,9 +107,14 @@ test: manifests generate fmt vet envtest ## Run tests.
 	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -coverprofile cover.out `go list ./... | grep -v 'tests/e2e'`
 	
 
+.PHONY: start-e2e
+start-e2e: ## Start the operator, to run the e2e tests
+	hack/start-rollouts-manager-for-e2e-tests.sh
+
+
 .PHONY: test-e2e
 test-e2e: ## Run operator e2e tests
-	go test -v -p=1 -timeout=10m -race -count=1 -coverprofile=coverage.out ./tests/e2e
+	hack/run-rollouts-manager-e2e-tests.sh
 
 ##@ Build
 
diff --git a/controllers/argorollouts_controller.go b/controllers/argorollouts_controller.go
index 3021d24..182c4f1 100644
--- a/controllers/argorollouts_controller.go
+++ b/controllers/argorollouts_controller.go
@@ -79,7 +79,7 @@ var log = logr.Log.WithName("rollouts-controller")
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.1/pkg/reconcile
 func (r *RolloutManagerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	reqLogger := logr.FromContext(ctx, "Request.Namespace", req.Namespace, "Request.Name", req.Name)
-	reqLogger.Info("Reconciling Rollout Managers")
+	reqLogger.Info("Reconciling RolloutManager")
 
 	// Fetch the RolloutManager instance
 	rollouts := &rolloutsmanagerv1alpha1.RolloutManager{}
diff --git a/controllers/deployment.go b/controllers/deployment.go
index 5987f75..3c6a639 100644
--- a/controllers/deployment.go
+++ b/controllers/deployment.go
@@ -15,10 +15,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
-// Reconcile the Rollouts controller deployment.
-func (r *RolloutManagerReconciler) reconcileRolloutsDeployment(ctx context.Context, cr *rolloutsmanagerv1alpha1.RolloutManager, sa *corev1.ServiceAccount) error {
+func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager, sa corev1.ServiceAccount) appsv1.Deployment {
+
+	// NOTE: When updating this function, ensure that normalizeDeployment is updated as well. See that function for details.
+
 	// Configuration for the desired deployment
-	desiredDeployment := &appsv1.Deployment{
+	desiredDeployment := appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      DefaultArgoRolloutsResourceName,
 			Namespace: cr.Namespace,
@@ -72,50 +74,113 @@ func (r *RolloutManagerReconciler) reconcileRolloutsDeployment(ctx context.Conte
 		},
 	}
 
+	return desiredDeployment
+}
+
+// Reconcile the Rollouts controller deployment.
+func (r *RolloutManagerReconciler) reconcileRolloutsDeployment(ctx context.Context, cr *rolloutsmanagerv1alpha1.RolloutManager, sa corev1.ServiceAccount) error {
+
+	desiredDeployment := generateDesiredRolloutsDeployment(*cr, sa)
+
+	normalizedDesiredDeployment, err := normalizeDeployment(desiredDeployment)
+	if err != nil {
+		// If you see this warning in the logs, verify that normalizedDeployment is fully consistent with generateDesiredRolloutsDeployment. See normalizeDeployment for details.
+		log.Error(fmt.Errorf("unexpected fail on normalizing generated rollouts Deployment"), "", "err", err)
+		// We intentionally continue without returning, as the error is non-fatal at runtime
+	}
+
+	if !reflect.DeepEqual(normalizedDesiredDeployment, desiredDeployment) { // sanity test to verify that generateDesiredRolloutsDeployments and normalizeDeployment are consistent.
+		// If you see this warning in the logs, verify that normalizedDeployment is fully consistent with generateDesiredRolloutsDeployment. See normalizeDeployment for details.
+		deploymentsDifferent := identifyDeploymentDifference(normalizedDesiredDeployment, desiredDeployment)
+		log.Error(fmt.Errorf("normalized form of desired Deployment was not equal: %v", deploymentsDifferent), "")
+		// We intentionally continue without returning, as the error is non-fatal at runtime
+	}
+
 	// If the deployment for rollouts does not exist, create one.
 	actualDeployment := &appsv1.Deployment{}
 
 	if err := fetchObject(ctx, r.Client, cr.Namespace, DefaultArgoRolloutsResourceName, actualDeployment); err != nil {
 		if !errors.IsNotFound(err) {
-			return fmt.Errorf("failed to get the deployment %s : %s", DefaultArgoRolloutsResourceName, err)
+			return fmt.Errorf("failed to get the Deployment %s: %s", DefaultArgoRolloutsResourceName, err)
 		}
 
-		if err := controllerutil.SetControllerReference(cr, desiredDeployment, r.Scheme); err != nil {
+		if err := controllerutil.SetControllerReference(cr, &desiredDeployment, r.Scheme); err != nil {
 			return err
 		}
-		log.Info(fmt.Sprintf("Creating deployment %s", DefaultArgoRolloutsResourceName))
-		return r.Client.Create(ctx, desiredDeployment)
-	}
-
-	actualPodSpec := actualDeployment.Spec.Template.Spec
-
-	// If the Deployment already exists, make sure the values we care about are correct.
-	deploymentsDifferent := !reflect.DeepEqual(actualPodSpec.Containers, desiredPodSpec.Containers) ||
-		actualPodSpec.ServiceAccountName != desiredPodSpec.ServiceAccountName ||
-		!reflect.DeepEqual(actualDeployment.Labels, desiredDeployment.Labels) ||
-		!reflect.DeepEqual(actualDeployment.Spec.Template.Labels, desiredDeployment.Spec.Template.Labels) ||
-		!reflect.DeepEqual(actualDeployment.Spec.Selector, desiredDeployment.Spec.Selector) ||
-		!reflect.DeepEqual(actualDeployment.Spec.Template.Spec.NodeSelector, desiredDeployment.Spec.Template.Spec.NodeSelector) ||
-		!reflect.DeepEqual(actualDeployment.Spec.Template.Spec.Tolerations, desiredDeployment.Spec.Template.Spec.Tolerations) ||
-		!reflect.DeepEqual(actualPodSpec.SecurityContext, desiredPodSpec.SecurityContext) ||
-		!reflect.DeepEqual(actualDeployment.Spec.Template.Spec.Volumes, desiredDeployment.Spec.Template.Spec.Volumes)
-
-	if deploymentsDifferent {
-		actualDeployment.Spec.Template.Spec.Containers = desiredPodSpec.Containers
-		actualDeployment.Spec.Template.Spec.ServiceAccountName = desiredPodSpec.ServiceAccountName
+		log.Info(fmt.Sprintf("Creating Deployment %s", DefaultArgoRolloutsResourceName))
+		return r.Client.Create(ctx, &desiredDeployment)
+	}
+
+	normalizedActualDeployment, err := normalizeDeployment(*actualDeployment)
+
+	if err != nil || !reflect.DeepEqual(normalizedActualDeployment, normalizedDesiredDeployment) {
+
+		deploymentsDifferent := identifyDeploymentDifference(normalizedActualDeployment, normalizedDesiredDeployment)
+
+		log.Info("updating Deployment due to detected difference: " + deploymentsDifferent)
+
+		actualDeployment.Spec.Template.Spec.Containers = desiredDeployment.Spec.Template.Spec.Containers
+		actualDeployment.Spec.Template.Spec.ServiceAccountName = desiredDeployment.Spec.Template.Spec.ServiceAccountName
 		actualDeployment.Labels = desiredDeployment.Labels
 		actualDeployment.Spec.Template.Labels = desiredDeployment.Spec.Template.Labels
 		actualDeployment.Spec.Selector = desiredDeployment.Spec.Selector
 		actualDeployment.Spec.Template.Spec.NodeSelector = desiredDeployment.Spec.Template.Spec.NodeSelector
 		actualDeployment.Spec.Template.Spec.Tolerations = desiredDeployment.Spec.Template.Spec.Tolerations
-		actualDeployment.Spec.Template.Spec.SecurityContext = desiredPodSpec.SecurityContext
+		actualDeployment.Spec.Template.Spec.SecurityContext = desiredDeployment.Spec.Template.Spec.SecurityContext
 		actualDeployment.Spec.Template.Spec.Volumes = desiredDeployment.Spec.Template.Spec.Volumes
 		return r.Client.Update(ctx, actualDeployment)
 	}
 	return nil
 }
 
-func rolloutsContainer(cr *rolloutsmanagerv1alpha1.RolloutManager) corev1.Container {
+// identifyDeploymentDifference is a simple comparison of the contents of two deployments, returning "" if they are the same, otherwise returning the name of the field that changed.
+func identifyDeploymentDifference(x appsv1.Deployment, y appsv1.Deployment) string {
+
+	xPodSpec := x.Spec.Template.Spec
+	yPodSpec := y.Spec.Template.Spec
+
+	if !reflect.DeepEqual(xPodSpec.Containers, yPodSpec.Containers) {
+		return "Spec.Template.Spec.Containers"
+	}
+
+	if xPodSpec.ServiceAccountName != yPodSpec.ServiceAccountName {
+		return "ServiceAccountName"
+	}
+
+	if !reflect.DeepEqual(x.Labels, y.Labels) {
+		return "Labels"
+	}
+
+	if !reflect.DeepEqual(x.Spec.Template.Labels, y.Spec.Template.Labels) {
+		return ".Spec.Template.Labels"
+	}
+
+	if !reflect.DeepEqual(x.Spec.Selector, y.Spec.Selector) {
+		return ".Spec.Selector"
+	}
+
+	if !reflect.DeepEqual(x.Spec.Template.Spec.NodeSelector, y.Spec.Template.Spec.NodeSelector) {
+		return "Spec.Template.Spec.NodeSelector"
+	}
+
+	if !reflect.DeepEqual(x.Spec.Template.Spec.Tolerations, y.Spec.Template.Spec.Tolerations) {
+		return "Spec.Template.Spec.Tolerations"
+	}
+
+	if !reflect.DeepEqual(xPodSpec.SecurityContext, yPodSpec.SecurityContext) {
+		return "Spec.Template.Spec.SecurityContext"
+	}
+
+	if !reflect.DeepEqual(x.Spec.Template.Spec.Volumes, y.Spec.Template.Spec.Volumes) {
+		return "Spec.Template.Spec.Volumes"
+	}
+
+	return ""
+}
+
+func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Container {
+
+	// NOTE: When updating this function, ensure that normalizeDeployment is updated as well. See that function for details.
 
 	// Global proxy env vars go firstArgoRollouts
 	rolloutsEnv := cr.Spec.Env
@@ -185,13 +250,183 @@ func rolloutsContainer(cr *rolloutsmanagerv1alpha1.RolloutManager) corev1.Contai
 
 }
 
+// One of the goals of an operator is to reconcile the live state of a resource on the cluster, with a target state for that resource. However, one of the challenges in doing so is that some fields of the resource will naturally differ from the values that are generated: for example, some field have default values which are only set after creation. This can make it challenging to compare the live/target status. Various strategies exist to handle.
+//
+// The strategy used in this file is implemented here in normalizeDeployment: normalizeDeployment will created a normalized representation of any input Deployment: the normal form will only contains fields which are relevant/useful to the operator. All other fields will be ingorewd.
+//
+// You can then use:
+//
+//	reflect.DeepEqual( normalizeDeployment( /* desired deployment */), normalizeDeployment( /* actual deployment from k8s*/))
+//
+// To determine if the actual deployment from k8s needs to be updated.
+//
+// NOTE: When updating 'generateDesiredRolloutsDeployment', ensure this function is updated as well.
+// - Specifically, in generateDesiredRolloutsDeployment, if a new field of Deployment is modified, it should be added to the copy logic here.
+func normalizeDeployment(inputParam appsv1.Deployment) (appsv1.Deployment, error) {
+
+	input := inputParam.DeepCopy()
+
+	res := appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      input.ObjectMeta.Name,
+			Namespace: input.ObjectMeta.Namespace,
+			Labels:    input.ObjectMeta.Labels,
+		},
+	}
+
+	if input.Spec.Selector == nil {
+		return appsv1.Deployment{}, fmt.Errorf("missing .spec.selector")
+	}
+
+	inputSpecSecurityContext := input.Spec.Template.Spec.SecurityContext
+
+	if inputSpecSecurityContext == nil {
+		return appsv1.Deployment{}, fmt.Errorf("missing .spec.template.spec.securityContext")
+	}
+
+	inputSpecVolumes := input.Spec.Template.Spec.Volumes
+	if inputSpecVolumes == nil || len(inputSpecVolumes) != 1 {
+		return appsv1.Deployment{}, fmt.Errorf("missing .spec.template.spec.volumes")
+	}
+
+	res.Spec = appsv1.DeploymentSpec{
+		Selector: &metav1.LabelSelector{
+			MatchLabels: input.Spec.Selector.MatchLabels,
+		},
+		Template: corev1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: input.Spec.Template.Labels,
+			},
+			Spec: corev1.PodSpec{
+				NodeSelector:       input.Spec.Template.Spec.NodeSelector,
+				Tolerations:        input.Spec.Template.Spec.Tolerations,
+				ServiceAccountName: input.Spec.Template.Spec.ServiceAccountName,
+				SecurityContext: &corev1.PodSecurityContext{
+					RunAsNonRoot: input.Spec.Template.Spec.SecurityContext.RunAsNonRoot,
+				},
+				Volumes: []corev1.Volume{{
+					Name: inputSpecVolumes[0].Name,
+				}},
+			},
+		},
+	}
+
+	if len(input.Spec.Template.Spec.Containers) != 1 {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect number of .spec.template.spec.containers")
+	}
+
+	inputContainer := input.Spec.Template.Spec.Containers[0]
+	inputLivenessProbe := inputContainer.LivenessProbe
+	inputPorts := inputContainer.Ports
+	inputReadinessProbe := inputContainer.ReadinessProbe
+	inputSecurityContext := inputContainer.SecurityContext
+	inputVolumeMounts := inputContainer.VolumeMounts
+
+	if inputLivenessProbe == nil {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect liveness probe")
+	}
+
+	if inputLivenessProbe.ProbeHandler.HTTPGet == nil {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect http get in liveness probe")
+	}
+
+	if inputReadinessProbe == nil {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect readiness probe")
+	}
+
+	if inputReadinessProbe.ProbeHandler.HTTPGet == nil {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect http get in readiness probe")
+	}
+
+	if inputPorts == nil || len(inputPorts) != 2 {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect input ports")
+	}
+
+	if inputSecurityContext == nil || inputSecurityContext.Capabilities == nil {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect security context")
+	}
+
+	if inputVolumeMounts == nil || len(inputVolumeMounts) != 1 {
+		return appsv1.Deployment{}, fmt.Errorf("incorrect volume mounts")
+	}
+
+	// Nil string slices need to be converted to empty string slices, because  reflect.DeepEqual(nil, []string{}) is false, despite being functionally the same, here.
+	if len(inputContainer.Args) == 0 {
+		inputContainer.Args = make([]string, 0)
+	}
+
+	if len(inputContainer.Env) == 0 {
+		inputContainer.Env = make([]corev1.EnvVar, 0)
+	}
+
+	res.Spec.Template.Spec.Containers = []corev1.Container{{
+		Args:            inputContainer.Args,
+		Env:             inputContainer.Env,
+		Image:           inputContainer.Image,
+		ImagePullPolicy: inputContainer.ImagePullPolicy,
+		LivenessProbe: &corev1.Probe{
+			FailureThreshold: inputLivenessProbe.FailureThreshold,
+			ProbeHandler: corev1.ProbeHandler{
+				HTTPGet: &corev1.HTTPGetAction{
+					Path: inputLivenessProbe.ProbeHandler.HTTPGet.Path,
+					Port: inputLivenessProbe.ProbeHandler.HTTPGet.Port,
+				},
+			},
+			InitialDelaySeconds: inputLivenessProbe.InitialDelaySeconds,
+			PeriodSeconds:       inputLivenessProbe.PeriodSeconds,
+			SuccessThreshold:    inputLivenessProbe.SuccessThreshold,
+			TimeoutSeconds:      inputLivenessProbe.TimeoutSeconds,
+		},
+		Name: inputContainer.Name,
+		Ports: []corev1.ContainerPort{
+			{
+				ContainerPort: inputPorts[0].ContainerPort,
+				Name:          inputPorts[0].Name,
+			},
+			{
+				ContainerPort: inputPorts[1].ContainerPort,
+				Name:          inputPorts[1].Name,
+			},
+		},
+		ReadinessProbe: &corev1.Probe{
+			FailureThreshold: inputReadinessProbe.FailureThreshold,
+			ProbeHandler: corev1.ProbeHandler{
+				HTTPGet: &corev1.HTTPGetAction{
+					Path: inputReadinessProbe.ProbeHandler.HTTPGet.Path,
+					Port: inputReadinessProbe.ProbeHandler.HTTPGet.Port,
+				},
+			},
+			InitialDelaySeconds: inputReadinessProbe.InitialDelaySeconds,
+			PeriodSeconds:       inputReadinessProbe.PeriodSeconds,
+			SuccessThreshold:    inputReadinessProbe.SuccessThreshold,
+			TimeoutSeconds:      inputReadinessProbe.TimeoutSeconds,
+		},
+		SecurityContext: &corev1.SecurityContext{
+			Capabilities: &corev1.Capabilities{
+				Drop: inputSecurityContext.Capabilities.Drop,
+			},
+			AllowPrivilegeEscalation: inputSecurityContext.AllowPrivilegeEscalation,
+			ReadOnlyRootFilesystem:   inputSecurityContext.ReadOnlyRootFilesystem,
+			RunAsNonRoot:             inputSecurityContext.RunAsNonRoot,
+		},
+		VolumeMounts: []corev1.VolumeMount{
+			{
+				Name:      inputVolumeMounts[0].Name,
+				MountPath: inputVolumeMounts[0].MountPath,
+			}},
+	}}
+
+	return res, nil
+
+}
+
 // boolPtr returns a pointer to val
 func boolPtr(val bool) *bool {
 	return &val
 }
 
 // Returns the container image for rollouts controller.
-func getRolloutsContainerImage(cr *rolloutsmanagerv1alpha1.RolloutManager) string {
+func getRolloutsContainerImage(cr rolloutsmanagerv1alpha1.RolloutManager) string {
 	defaultImg, defaultTag := false, false
 
 	img := cr.Spec.Image
@@ -215,7 +450,7 @@ func getRolloutsContainerImage(cr *rolloutsmanagerv1alpha1.RolloutManager) strin
 }
 
 // getRolloutsCommand will return the command for the Rollouts controller component.
-func getRolloutsCommandArgs(cr *rolloutsmanagerv1alpha1.RolloutManager) []string {
+func getRolloutsCommandArgs(cr rolloutsmanagerv1alpha1.RolloutManager) []string {
 	args := make([]string, 0)
 
 	args = append(args, "--namespaced")
diff --git a/controllers/reconcile.go b/controllers/reconcile.go
index 6584504..ad430c9 100644
--- a/controllers/reconcile.go
+++ b/controllers/reconcile.go
@@ -52,7 +52,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsManager(ctx context.Context,
 	}
 
 	log.Info("reconciling rollouts deployment")
-	if err := r.reconcileRolloutsDeployment(ctx, cr, sa); err != nil {
+	if err := r.reconcileRolloutsDeployment(ctx, cr, *sa); err != nil {
 		return err
 	}
 


Test patch:
diff --git a/.github/workflows/e2e_tests.yml b/.github/workflows/e2e_tests.yml
index c9396c8..ca25572 100644
--- a/.github/workflows/e2e_tests.yml
+++ b/.github/workflows/e2e_tests.yml
@@ -45,9 +45,7 @@ jobs:
           go mod download
       - name: Run the operator locally
         run: |
-          set -o pipefail
-          make install generate fmt vet
-          go run ./main.go 2>&1 | tee /tmp/e2e-operator-run.log &
+          make start-e2e &
       - name: Run tests
         run: |
           set -o pipefail
diff --git a/controllers/deployment_test.go b/controllers/deployment_test.go
index 69f8c30..ab5c0e8 100644
--- a/controllers/deployment_test.go
+++ b/controllers/deployment_test.go
@@ -2,6 +2,8 @@ package rollouts
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 
 	"github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1"
 	. "github.com/onsi/ginkgo/v2"
@@ -9,6 +11,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var _ = Describe("Deployment Test", func() {
@@ -36,7 +39,7 @@ var _ = Describe("Deployment Test", func() {
 	It("should create a new deployment if it does not exist", func() {
 
 		By("calling reconcileRolloutsDeployment")
-		Expect(r.reconcileRolloutsDeployment(ctx, a, sa)).To(Succeed())
+		Expect(r.reconcileRolloutsDeployment(ctx, a, *sa)).To(Succeed())
 
 		By("fetch the Deployment")
 		fetchedDeployment := &appsv1.Deployment{}
@@ -64,7 +67,7 @@ var _ = Describe("Deployment Test", func() {
 		Expect(r.Client.Create(ctx, existingDeployment)).To(Succeed())
 
 		By("calling reconcileRolloutsDeployment")
-		Expect(r.reconcileRolloutsDeployment(ctx, a, sa)).To(Succeed())
+		Expect(r.reconcileRolloutsDeployment(ctx, a, *sa)).To(Succeed())
 
 		By("fetch the Deployment")
 		fetchedDeployment := &appsv1.Deployment{}
@@ -85,6 +88,131 @@ var _ = Describe("Deployment Test", func() {
 
 	})
 
+	When("the deployment has not changed", func() {
+		It("should not report any difference, both before and after normalization", func() {
+
+			expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, "tmp", "linux", DefaultArgoRolloutsResourceName, a)
+			Expect(identifyDeploymentDifference(*expectedDeployment, *expectedDeployment)).To(Equal(""))
+
+			expectedDeployment = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, "tmp", "linux", DefaultArgoRolloutsResourceName, a)
+			expectedDeploymentNormalized, err := normalizeDeployment(*expectedDeployment)
+			Expect(err).To(Succeed())
+
+			Expect(identifyDeploymentDifference(expectedDeploymentNormalized, *expectedDeployment)).To(Equal(""))
+			Expect(identifyDeploymentDifference(*expectedDeployment, expectedDeploymentNormalized)).To(Equal(""))
+			Expect(identifyDeploymentDifference(*expectedDeployment, *expectedDeployment)).To(Equal(""))
+
+		})
+	})
+
+	When("the Rollouts Deployment resource is changed by the user, outside of the operator", func() {
+
+		areEqual := func(x appsv1.Deployment, y appsv1.Deployment) bool {
+			xRes, xErr := normalizeDeployment(x)
+			yRes, yErr := normalizeDeployment(y)
+
+			if fmt.Sprintf("%v", xErr) != fmt.Sprintf("%v", yErr) {
+				return false
+			}
+
+			res := reflect.DeepEqual(xRes, yRes)
+
+			// Sanity test that identifyDeploymentDifference gives the same result as reflect.DeepEqual
+			deploymentDiff := identifyDeploymentDifference(x, y)
+			Expect(res == (deploymentDiff == "")).To(BeTrue())
+
+			return res
+		}
+
+		DescribeTable("controller should detect and revert the change", func(fxn func(deployment *appsv1.Deployment)) {
+
+			By("ensuring that deploymentCR properly detects the change")
+			defaultDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, "tmp", "linux", DefaultArgoRolloutsResourceName, a)
+
+			defaultDeploymentModified := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, "tmp", "linux", DefaultArgoRolloutsResourceName, a)
+
+			Expect(identifyDeploymentDifference(*defaultDeployment, *defaultDeploymentModified)).To(BeEmpty(), "they should be the same before one is modified")
+
+			fxn(defaultDeploymentModified)
+			Expect(identifyDeploymentDifference(*defaultDeployment, *defaultDeploymentModified)).ToNot(BeEmpty(), "after being modified, they should no longer be the same")
+
+			By("ensuring the reconcileRolloutsDeployment will detect and revert the change")
+			{
+				By("calling reconcileRolloutsDeployment to create a default Deployment")
+				expectedDepl := appsv1.Deployment{
+					ObjectMeta: metav1.ObjectMeta{Name: DefaultArgoRolloutsResourceName, Namespace: a.Namespace},
+				}
+				Expect(r.reconcileRolloutsDeployment(context.Background(), a, *sa)).To(Succeed())
+				Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(&expectedDepl), &expectedDepl)).To(Succeed())
+				updatedDepl := expectedDepl.DeepCopy()
+				Expect(areEqual(*updatedDepl, expectedDepl)).To(BeTrue(), "copy should be same as original")
+
+				By("updating the Deployment using the function, and then updating the cluster resource")
+				fxn(updatedDepl)
+				Expect(r.Client.Update(context.Background(), updatedDepl)).To(Succeed())
+
+				By("retrieving the cluster resource after the update, to ensure it is equal to the updated version")
+				updatedDeplFromClient := appsv1.Deployment{
+					ObjectMeta: metav1.ObjectMeta{Name: DefaultArgoRolloutsResourceName, Namespace: a.Namespace},
+				}
+				Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(&updatedDeplFromClient), &updatedDeplFromClient)).To(Succeed())
+				Expect(areEqual(*updatedDepl, updatedDeplFromClient)).To(BeTrue(), "resource on cluster should match the resource we called Update with")
+
+				Expect(areEqual(updatedDeplFromClient, expectedDepl)).ToNot(BeTrue(), "resource on cluster should NOT match the original Deployment that was created by the call to reconcileRolloutsDeployment")
+
+				By("calling reconcileRolloutsDeployment again, it should revert the change back to default")
+				Expect(r.reconcileRolloutsDeployment(context.Background(), a, *sa)).To(Succeed())
+
+				finalDeplFromClient := appsv1.Deployment{
+					ObjectMeta: metav1.ObjectMeta{Name: DefaultArgoRolloutsResourceName, Namespace: a.Namespace},
+				}
+
+				By("retrieving the Deployment version from the cluster")
+				Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(&finalDeplFromClient), &finalDeplFromClient)).To(Succeed())
+				Expect(areEqual(finalDeplFromClient, expectedDepl)).To(BeTrue(), "version from cluster should have been reconciled back to the default")
+
+			}
+
+		},
+			Entry("label", func(deployment *appsv1.Deployment) {
+				deployment.ObjectMeta.Labels = map[string]string{"my": "label"}
+			}),
+			Entry("spec.selector", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Selector = &metav1.LabelSelector{
+					MatchLabels: map[string]string{"my": "label"},
+				}
+			}),
+			Entry(".spec.template.spec.containers.args", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Containers[0].Args = []string{"new", "args"}
+			}),
+			Entry(".spec.template.spec.containers.env", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{
+					{Name: "my-env", Value: "my-env-value"}}
+			}),
+			Entry(".spec.template.spec.serviceAccountName", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.ServiceAccountName = "different-service-account-name"
+			}),
+			Entry(".spec.template.labels", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Labels = map[string]string{"new": "label"}
+			}),
+			Entry(".spec.template.spec.nodeSelector", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.NodeSelector = map[string]string{"my": "node"}
+			}),
+			Entry(".spec.template.spec.tolerations", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Tolerations = []corev1.Toleration{{Key: "value"}}
+			}),
+			Entry(".spec.template.spec.securityContext", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.SecurityContext = &corev1.PodSecurityContext{
+					SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeLocalhost},
+				}
+			}),
+			Entry(".spec.template.spec.volumes", func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Volumes = []corev1.Volume{{Name: "my-volume"}}
+			}),
+		)
+
+	})
+
 })
 
 func deploymentCR(name string, namespace string, label string, volumeName string, nodeSelector string, serviceAccount string, rolloutManager *v1alpha1.RolloutManager) *appsv1.Deployment {
@@ -118,7 +246,7 @@ func deploymentCR(name string, namespace string, label string, volumeName string
 					"kubernetes.io/os": nodeSelector,
 				},
 				Containers: []corev1.Container{
-					rolloutsContainer(rolloutManager),
+					rolloutsContainer(*rolloutManager),
 				},
 				ServiceAccountName: serviceAccount,
 				SecurityContext: &corev1.PodSecurityContext{
diff --git a/hack/run-rollouts-manager-e2e-tests.sh b/hack/run-rollouts-manager-e2e-tests.sh
new file mode 100755
index 0000000..52d430b
--- /dev/null
+++ b/hack/run-rollouts-manager-e2e-tests.sh
@@ -0,0 +1,32 @@
+#!/bin/bash
+
+SCRIPTPATH="$(
+  cd -- "$(dirname "$0")" >/dev/null 2>&1 || exit
+  pwd -P
+)"
+
+cd "$SCRIPTPATH/.."
+
+set -o pipefail
+set -ex
+
+go test -v -p=1 -timeout=10m -race -count=1 -coverprofile=coverage.out ./tests/e2e
+
+set +e
+
+# If the output from the E2E operator is available, then check it for errors
+if [ -f "/tmp/e2e-operator-run.log" ]; then
+
+    # Grep the log for unexpected errors
+    # - Ignore errors that are expected to occur
+
+    ERRORS_FOUND_TEXT=`cat /tmp/e2e-operator-run.log | grep "ERROR" | grep -v "unable to create new content in namespace argo-rollouts because it is being terminated" | grep -v "the object has been modified; please apply your changes to the latest version and try again"`
+
+    ERRORS_FOUND=`echo $ERRORS_FOUND_TEXT | grep "ERROR" | wc -l`
+
+    if [ "$ERRORS_FOUND" != "0" ]; then
+        echo "Unexpected errors found: $ERRORS_FOUND_TEXT"
+        exit 1
+    fi
+
+fi
diff --git a/hack/run-upstream-argo-rollouts-e2e-tests.sh b/hack/run-upstream-argo-rollouts-e2e-tests.sh
index be288c8..1d60955 100755
--- a/hack/run-upstream-argo-rollouts-e2e-tests.sh
+++ b/hack/run-upstream-argo-rollouts-e2e-tests.sh
@@ -4,8 +4,8 @@ CURRENT_ROLLOUTS_VERSION=v1.6.6
 
 function cleanup {
   echo "* Cleaning up"
-	killall main
-  killall go
+  killall main || true
+  killall go || true
 }
 
 set -x
@@ -37,16 +37,22 @@ kubectl config set-context --current --namespace=argo-rollouts
 
 # 3) Build, install, and start the argo-rollouts-manager controller
 cd $SCRIPT_DIR/..
-make generate fmt vet install
 
-set +e
 
-rm -f /tmp/e2e-operator-run.log || true
-go run ./main.go 2>&1 | tee /tmp/e2e-operator-run.log &
+# Only start the controller if SKIP_RUN_STEP is empty
+# - Otherwise, we assume that Argo Rollouts operator is already installed and running (for example, via OpenShift GitOps)
+if [ -z "$SKIP_RUN_STEP" ]; then
+  make generate fmt vet install
 
-set -e
+  set +e
+
+  rm -f /tmp/e2e-operator-run.log || true
+  go run ./main.go 2>&1 | tee /tmp/e2e-operator-run.log &
+
+  set -e
+fi
 
-# 4) Install Argo Rollouts into the Namespace via RolloutManater CR
+# 4) Install Argo Rollouts into the Namespace via RolloutManager CR
 
 cd $TMP_DIR/argo-rollouts
 
diff --git a/hack/start-rollouts-manager-for-e2e-tests.sh b/hack/start-rollouts-manager-for-e2e-tests.sh
new file mode 100755
index 0000000..94ab0ee
--- /dev/null
+++ b/hack/start-rollouts-manager-for-e2e-tests.sh
@@ -0,0 +1,16 @@
+#!/bin/bash
+
+SCRIPTPATH="$(
+  cd -- "$(dirname "$0")" >/dev/null 2>&1 || exit
+  pwd -P
+)"
+
+cd "$SCRIPTPATH/.."
+
+rm -f /tmp/e2e-operator-run.log || true
+
+set -o pipefail
+set -ex
+
+make install generate fmt vet
+go run ./main.go 2>&1 | tee /tmp/e2e-operator-run.log
diff --git a/hack/verify-rollouts-e2e-tests/main.go b/hack/verify-rollouts-e2e-tests/main.go
index d2d0bac..807175b 100644
--- a/hack/verify-rollouts-e2e-tests/main.go
+++ b/hack/verify-rollouts-e2e-tests/main.go
@@ -190,10 +190,13 @@ func parseTestResultsFromFile(fileContents []string, testsExpectedToFailList []s
 	return testResults, nil
 }
 
-// waitAndGetE2EFileContents waits for the last line of the file to start with 'DONE' before returning the contents
-// This allows us to avoid a race condition with tee where the file contents of the file may not have been fully flushed.
+// waitAndGetE2EFileContents waits for one of the last 50 lines of the file to start with 'DONE' before returning the contents.
+// This allows us to avoid a race condition with tee where the file contents of the file may not have been fully written.
 func waitAndGetE2EFileContents(testsE2EResultsLogPath string) ([]string, error) {
 
+	// Return whatever we have after 1 minute has elapsed
+	expireTime := time.Now().Add(1 * time.Minute)
+
 	for {
 
 		fileLines, err := readFileIntoListOfLines(testsE2EResultsLogPath, true)
@@ -201,16 +204,26 @@ func waitAndGetE2EFileContents(testsE2EResultsLogPath string) ([]string, error)
 			return []string{}, err
 		}
 
-		lastLine := fileLines[len(fileLines)-1]
+		lineIndexStart := len(fileLines) - 50
+		if lineIndexStart < 0 {
+			lineIndexStart = 0
+		}
+
+		for _, line := range fileLines[lineIndexStart:] {
+			// example line: DONE 6 runs, 144 tests, 6 skipped, 47 failures in 2279.668s
+			if strings.HasPrefix(line, "DONE ") {
+				fmt.Println("E2E tests file is complete:", line)
+				return fileLines, nil
+			}
+		}
 
-		// example: DONE 6 runs, 144 tests, 6 skipped, 47 failures in 2279.668s
-		if strings.HasPrefix(lastLine, "DONE ") {
-			fmt.Println("E2E tests file is complete:", lastLine)
+		if time.Now().After(expireTime) {
+			fmt.Println("E2E tests file timed out waiting. Previous X lines were:", fileLines[lineIndexStart:])
 			return fileLines, nil
 		}
 
 		// Otherwise, wait a second then check again.
-		fmt.Println("* Waiting for E2E test file to be complete:", lastLine)
+		fmt.Println("* Waiting for E2E test file to be complete")
 		time.Sleep(time.Second)
 	}