From 8132d6a8314ca5f50cce68562caeecb072031386 Mon Sep 17 00:00:00 2001
From: "W. Trevor King" <wking@tremily.us>
Date: Tue, 20 Apr 2021 10:32:41 -0700
Subject: [PATCH 1/2] lib/resourcebuilder/apps: Fail on
 ProgressDeadlineExceeded

See in a customer cluster [1]:

  $ yaml2json <namespaces/openshift-marketplace/apps/deployments.yaml | jq -r '.items[].status.conditions'
  [
    {
      "lastTransitionTime": "2021-04-12T22:04:41Z",
      "lastUpdateTime": "2021-04-12T22:04:41Z",
      "message": "Deployment has minimum availability.",
      "reason": "MinimumReplicasAvailable",
      "status": "True",
      "type": "Available"
    },
    {
      "lastTransitionTime": "2021-04-19T17:33:30Z",
      "lastUpdateTime": "2021-04-19T17:33:30Z",
      "message": "ReplicaSet \"marketplace-operator-f7cc88d59\" has timed out progressing.",
      "reason": "ProgressDeadlineExceeded",
      "status": "False",
      "type": "Progressing"
    }
  ]

Previous touch for this logic was 0442094858
(lib/resourcebuilder/apps: Only error on Deployment Available=False
*and* Progressing=False, 2020-08-06, #430), where I said:

  Available=True, Progressing=False is the happy, steady state.

This commit tightens that back down a bit, to account for cases where
the Deployment controller claims the Deployment is still available,
but has given up on rolling out a requested update [2].  We definitely
want to complain about Deployments like that, because admins might
want to investigate the sticking issue, and possibly delete or
otherwise poke the Deployment so we come back in and reconcile it so
the Deployment controller gives it another attempt.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1951339#c0
[2]: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#failed-deployment
---
 lib/resourcebuilder/apps.go | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/resourcebuilder/apps.go b/lib/resourcebuilder/apps.go
index dd5bea2506..31ee5c5bc3 100644
--- a/lib/resourcebuilder/apps.go
+++ b/lib/resourcebuilder/apps.go
@@ -123,6 +123,15 @@ func (b *builder) checkDeploymentHealth(ctx context.Context, deployment *appsv1.
 		}
 	}
 
+	if progressingCondition != nil && progressingCondition.Status == corev1.ConditionFalse && progressingCondition.Reason == "ProgressDeadlineExceeded" {
+		return &payload.UpdateError{
+			Nested:  fmt.Errorf("deployment %s is %s=%s: %s: %s", iden, progressingCondition.Type, progressingCondition.Status, progressingCondition.Reason, progressingCondition.Message),
+			Reason:  "WorkloadNotProgressing",
+			Message: fmt.Sprintf("deployment %s is %s=%s: %s: %s", iden, progressingCondition.Type, progressingCondition.Status, progressingCondition.Reason, progressingCondition.Message),
+			Name:    iden,
+		}
+	}
+
 	if availableCondition == nil && progressingCondition == nil && replicaFailureCondition == nil {
 		klog.Warningf("deployment %s is not setting any expected conditions, and is therefore in an unknown state", iden)
 	}

From 02bb9ba829174d799e418aeedfbd88bf572a35a0 Mon Sep 17 00:00:00 2001
From: "W. Trevor King" <wking@tremily.us>
Date: Tue, 20 Apr 2021 10:41:01 -0700
Subject: [PATCH 2/2] lib/resourcemerge/core: Clear env and envFrom if unset in
 manifest

Even if the manifest authors state no opinions, these are not
properties that we want to allow cluster admins to manipulate.  For
example, a customer cluster recently stuck a Deployment by inserting a
reference to a non-existent secret [1]:

  $ yaml2json <namespaces/openshift-marketplace/apps/deployments.yaml | jq -r '.items[].spec.template.spec.containers[].envFrom[]'
  {
    "secretRef": {
      "name": "openshift-reg"
    }
  }
  $ yaml2json <namespaces/openshift-marketplace/pods/marketplace-operator-f7cc88d59-hhh75/marketplace-operator-f7cc88d59-hhh75.yaml | jq -r '.status.containerStatuses[].state'
  {
    "waiting": {
      "message": "secret \"openshift-reg\" not found",
      "reason": "CreateContainerConfigError"
    }
  }

The outgoing logic dates back to the beginning of reconciling these
properties in 14fab0b2c8 (add generic 2-way merge handler for random
types, 2018-09-27, #26), and this commit's tightening follows on a
number of reconciliation tightenings like 29b92d2b02
(lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil
in required, 2020-01-16, #298).

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1951339#c0
---
 lib/resourcemerge/core.go | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go
index 01451e65bb..665cabf4f7 100644
--- a/lib/resourcemerge/core.go
+++ b/lib/resourcemerge/core.go
@@ -141,9 +141,6 @@ func ensureContainer(modified *bool, existing *corev1.Container, required corev1
 }
 
 func ensureEnvVar(modified *bool, existing *[]corev1.EnvVar, required []corev1.EnvVar) {
-	if required == nil {
-		return
-	}
 	if !equality.Semantic.DeepEqual(required, *existing) {
 		*existing = required
 		*modified = true
@@ -151,9 +148,6 @@ func ensureEnvVar(modified *bool, existing *[]corev1.EnvVar, required []corev1.E
 }
 
 func ensureEnvFromSource(modified *bool, existing *[]corev1.EnvFromSource, required []corev1.EnvFromSource) {
-	if required == nil {
-		return
-	}
 	if !equality.Semantic.DeepEqual(required, *existing) {
 		*existing = required
 		*modified = true