Repository: knative/operator
Pull request #2035 (2025-03-18 12:13:30)

[release-1.17] Wait for deployments
This is an automated cherry-pick of #2010

```release-note
Proper order is enforced now during manifest installation.
```

knative-prow[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/operator/pull/2035#" title="Author self-approved">knative-prow-robot</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign [dsimansk](https://github.com/dsimansk) for approval. For more information see [the Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Foperator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/operator/blob/release-1.17/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dsimansk"]} -->
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/knative/operator/pull/2035?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 62.81%. Comparing base [(`fea409a`)](https://app.codecov.io/gh/knative/operator/commit/fea409abf02756511f0da8492b19b3a511553883?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) to head [(`d7fe9a8`)](https://app.codecov.io/gh/knative/operator/commit/d7fe9a8d0ef460d1fd97ad4d90f3d452884185ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

<details><summary>Additional details and impacted files</summary>


```diff
@@               Coverage Diff                @@
##           release-1.17    #2035      +/-   ##
================================================
+ Coverage         62.78%   62.81%   +0.03%     
================================================
  Files                49       49              
  Lines              2292     2294       +2     
================================================
+ Hits               1439     1441       +2     
  Misses              761      761              
  Partials             92       92              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/knative/operator/pull/2035?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

<details><summary>🚀 New features to boost your workflow: </summary>

- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/operator/pull/2035#" title="Author self-approved">knative-prow-robot</a>*, *<a href="https://github.com/knative/operator/pull/2035#pullrequestreview-2694340199" title="Approved">matzew</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/operator/blob/release-1.17/OWNERS)~~ [matzew]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow[bot]: New changes are detected. LGTM label has been removed.
dsimansk: /approve
/lgtm

Patch:
diff --git a/pkg/reconciler/common/deployments.go b/pkg/reconciler/common/deployments.go
index c0c5afcea..ae3c703e6 100644
--- a/pkg/reconciler/common/deployments.go
+++ b/pkg/reconciler/common/deployments.go
@@ -18,15 +18,31 @@ package common
 
 import (
 	"context"
+	"errors"
 
 	mf "github.com/manifestival/manifestival"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/kubernetes/scheme"
+
 	"knative.dev/operator/pkg/apis/operator/base"
 )
 
+type deploymentsNotReadyError struct{}
+
+var _ error = deploymentsNotReadyError{}
+
+// Error implements the Error() interface of error.
+func (err deploymentsNotReadyError) Error() string {
+	return "deployments not ready"
+}
+
+// IsDeploymentsNotReadyError returns true if the given error is a deploymentsNotReadyError.
+func IsDeploymentsNotReadyError(err error) bool {
+	return errors.Is(err, deploymentsNotReadyError{})
+}
+
 // CheckDeployments checks all deployments in the given manifest and updates the given
 // status with the status of the deployments.
 func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
@@ -36,7 +52,7 @@ func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.
 		resource, err := manifest.Client.Get(&u)
 		if err != nil {
 			status.MarkDeploymentsNotReady([]string{"all"})
-			if errors.IsNotFound(err) {
+			if apierrors.IsNotFound(err) {
 				return nil
 			}
 			return err
@@ -52,7 +68,7 @@ func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.
 
 	if len(nonReadyDeployments) > 0 {
 		status.MarkDeploymentsNotReady(nonReadyDeployments)
-		return nil
+		return deploymentsNotReadyError{}
 	}
 
 	status.MarkDeploymentsAvailable()
diff --git a/pkg/reconciler/common/install.go b/pkg/reconciler/common/install.go
index 4b3b3adaf..83170d87c 100644
--- a/pkg/reconciler/common/install.go
+++ b/pkg/reconciler/common/install.go
@@ -22,17 +22,20 @@ import (
 	"strings"
 
 	mf "github.com/manifestival/manifestival"
-	"knative.dev/pkg/logging"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"knative.dev/operator/pkg/apis/operator/base"
 	"knative.dev/operator/pkg/apis/operator/v1beta1"
+	"knative.dev/pkg/logging"
 )
 
 var (
-	role            mf.Predicate = mf.Any(mf.ByKind("ClusterRole"), mf.ByKind("Role"))
-	rolebinding     mf.Predicate = mf.Any(mf.ByKind("ClusterRoleBinding"), mf.ByKind("RoleBinding"))
-	webhook         mf.Predicate = mf.Any(mf.ByKind("MutatingWebhookConfiguration"), mf.ByKind("ValidatingWebhookConfiguration"))
-	gatewayNotMatch              = "no matches for kind \"Gateway\""
+	role                      mf.Predicate = mf.Any(mf.ByKind("ClusterRole"), mf.ByKind("Role"))
+	rolebinding               mf.Predicate = mf.Any(mf.ByKind("ClusterRoleBinding"), mf.ByKind("RoleBinding"))
+	webhook                   mf.Predicate = mf.Any(mf.ByKind("MutatingWebhookConfiguration"), mf.ByKind("ValidatingWebhookConfiguration"))
+	webhookDependentResources mf.Predicate = byGV(schema.GroupKind{Group: "networking.internal.knative.dev", Kind: "Certificate"})
+	gatewayNotMatch                        = "no matches for kind \"Gateway\""
 )
 
 // Install applies the manifest resources for the given version and updates the given
@@ -52,7 +55,7 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 		status.MarkInstallFailed(err.Error())
 		return fmt.Errorf("failed to apply (cluster)rolebindings: %w", err)
 	}
-	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding, webhook))).Apply(); err != nil {
+	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding, webhook, webhookDependentResources))).Apply(); err != nil {
 		status.MarkInstallFailed(err.Error())
 		if ks, ok := instance.(*v1beta1.KnativeServing); ok && strings.Contains(err.Error(), gatewayNotMatch) &&
 			(ks.Spec.Ingress == nil || ks.Spec.Ingress.Istio.Enabled) {
@@ -63,10 +66,32 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 
 		return fmt.Errorf("failed to apply non rbac manifest: %w", err)
 	}
+	return nil
+}
+
+// InstallWebhookConfigs applies the Webhook manifest resources and updates the given status accordingly.
+func InstallWebhookConfigs(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
 	if err := manifest.Filter(webhook).Apply(); err != nil {
 		status.MarkInstallFailed(err.Error())
 		return fmt.Errorf("failed to apply webhooks: %w", err)
 	}
+	return nil
+}
+
+// InstallWebhookDependentResources applies the Webhook dependent resources updates the given status accordingly.
+func InstallWebhookDependentResources(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
+	if err := manifest.Filter(webhookDependentResources).Apply(); err != nil {
+		status.MarkInstallFailed(err.Error())
+		return fmt.Errorf("failed to apply webhooks: %w", err)
+	}
+	return nil
+}
+
+// MarkStatusSuccess updates the component status to success.
+func MarkStatusSuccess(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
 	status.MarkInstallSucceeded()
 	status.SetVersion(TargetVersion(instance))
 	return nil
@@ -83,3 +108,9 @@ func Uninstall(manifest *mf.Manifest) error {
 	}
 	return nil
 }
+
+func byGV(gk schema.GroupKind) mf.Predicate {
+	return func(u *unstructured.Unstructured) bool {
+		return u.GroupVersionKind().GroupKind() == gk
+	}
+}
diff --git a/pkg/reconciler/common/stages.go b/pkg/reconciler/common/stages.go
index 66f1fc7d6..47edd79b1 100644
--- a/pkg/reconciler/common/stages.go
+++ b/pkg/reconciler/common/stages.go
@@ -38,6 +38,9 @@ type Stages []Stage
 func (stages Stages) Execute(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
 	for _, stage := range stages {
 		if err := stage(ctx, manifest, instance); err != nil {
+			if IsDeploymentsNotReadyError(err) {
+				break
+			}
 			return err
 		}
 	}
diff --git a/pkg/reconciler/knativeeventing/knativeeventing.go b/pkg/reconciler/knativeeventing/knativeeventing.go
index 959656ef0..dab94829f 100644
--- a/pkg/reconciler/knativeeventing/knativeeventing.go
+++ b/pkg/reconciler/knativeeventing/knativeeventing.go
@@ -142,6 +142,9 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ke *v1beta1.KnativeEvent
 		r.handleTLSResources,
 		manifests.Install,
 		common.CheckDeployments,
+		common.InstallWebhookConfigs,
+		manifests.SetManifestPaths,
+		common.MarkStatusSuccess,
 		common.DeleteObsoleteResources(ctx, ke, r.installed),
 	}
 	manifest := r.manifest.Append()
diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go
index d554b73dd..cf54f4aeb 100644
--- a/pkg/reconciler/knativeserving/knativeserving.go
+++ b/pkg/reconciler/knativeserving/knativeserving.go
@@ -124,6 +124,10 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1beta1.KnativeServi
 		r.transform,
 		manifests.Install,
 		common.CheckDeployments,
+		common.InstallWebhookConfigs,
+		common.InstallWebhookDependentResources,
+		manifests.SetManifestPaths,
+		common.MarkStatusSuccess,
 		common.DeleteObsoleteResources(ctx, ks, r.installed),
 	}
 	manifest := r.manifest.Append()
diff --git a/pkg/reconciler/manifests/install.go b/pkg/reconciler/manifests/install.go
index dc5bf0d77..0c4c60a2c 100644
--- a/pkg/reconciler/manifests/install.go
+++ b/pkg/reconciler/manifests/install.go
@@ -34,6 +34,11 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 	if err != nil {
 		return err
 	}
+	return nil
+}
+
+// SetManifestPaths set the manifest paths in the status of the component
+func SetManifestPaths(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
 	status := instance.GetStatus()
 	path := common.TargetManifestPathArray(instance)
 	version := common.TargetVersion(instance)


Test patch:
diff --git a/pkg/reconciler/common/deployments_test.go b/pkg/reconciler/common/deployments_test.go
index 5ea3fdedf..865f3768c 100644
--- a/pkg/reconciler/common/deployments_test.go
+++ b/pkg/reconciler/common/deployments_test.go
@@ -78,7 +78,7 @@ func TestCheckDeployments(t *testing.T) {
 			*NamespacedResource("apps/v1", "Deployment", "test", "notReady"),
 		},
 		inAPI:      []runtime.Object{notReadyDeployment},
-		wantError:  false,
+		wantError:  true,
 		wantStatus: corev1.ConditionFalse,
 	}, {
 		name: "ready and not ready deployment",
@@ -86,8 +86,8 @@ func TestCheckDeployments(t *testing.T) {
 			*NamespacedResource("apps/v1", "Deployment", "test", "ready"),
 			*NamespacedResource("apps/v1", "Deployment", "test", "notReady"),
 		},
-		inAPI:      []runtime.Object{},
-		wantError:  false,
+		inAPI:      []runtime.Object{readyDeployment, notReadyDeployment},
+		wantError:  true,
 		wantStatus: corev1.ConditionFalse,
 	}, {
 		name: "not found deployment",
diff --git a/pkg/reconciler/common/install_test.go b/pkg/reconciler/common/install_test.go
index b99a6231f..842562d95 100644
--- a/pkg/reconciler/common/install_test.go
+++ b/pkg/reconciler/common/install_test.go
@@ -81,6 +81,14 @@ func TestInstall(t *testing.T) {
 		t.Fatalf("Install() = %v, want no error", err)
 	}
 
+	if err := InstallWebhookConfigs(context.TODO(), &manifest, instance); err != nil {
+		t.Fatalf("InstallWebhookConfigs() = %v, want no error", err)
+	}
+
+	if err := MarkStatusSuccess(context.TODO(), &manifest, instance); err != nil {
+		t.Fatalf("MarkStatusSuccess() = %v, want no error", err)
+	}
+
 	if !cmp.Equal(client.creates, want) {
 		t.Fatalf("Unexpected creates: %s", cmp.Diff(client.creates, want))
 	}
diff --git a/pkg/reconciler/manifests/install_test.go b/pkg/reconciler/manifests/install_test.go
index b9884b951..4d8b0bf0c 100644
--- a/pkg/reconciler/manifests/install_test.go
+++ b/pkg/reconciler/manifests/install_test.go
@@ -107,6 +107,12 @@ func TestInstall(t *testing.T) {
 			manifest, _ := mf.ManifestFrom(mf.Slice{})
 			err := Install(context.TODO(), &manifest, tt.instance)
 			util.AssertEqual(t, err, nil)
+			err = common.InstallWebhookConfigs(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
+			err = SetManifestPaths(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
+			err = common.MarkStatusSuccess(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
 			util.AssertEqual(t, tt.instance.GetStatus().GetVersion(), tt.version)
 			util.AssertDeepEqual(t, tt.instance.GetStatus().GetManifests(), tt.expectedPath)
 		})