Repository: knative/operator (go)

Issue:
Webhook dependant resources are deployed before deployments are available
<!-- 
Are you using Knative? If you do, we would love to know!
https://github.com/knative/community/issues/new?template=ADOPTERS.yaml&title=%5BADOPTERS%5D%3A+%24%7BCOMPANY+NAME+HERE%7D
-->

**Describe the bug**
A clear and concise description of what the bug is.

Right now a fresh install prints:
```
{"severity":"INFO","timestamp":"2025-03-06T08:31:02.4824378Z","logger":"knative-operator.manifestival","caller":"manifestival@v0.7.2/manifestival.go:208","message":"Updating","commit":"71d7c8c-dirty","knative.dev/pod":"knative-operator-5797f68df9-lmw8c","name":"knative-serving/routing-serving-certs","type":"networking.internal.knative.dev/v1alpha1, Kind=Certificate"}
{"severity":"ERROR","timestamp":"2025-03-06T08:31:02.677900778Z","logger":"knative-operator","caller":"knativeserving/reconciler.go:295","message":"Returned an error","commit":"71d7c8c-dirty","knative.dev/pod":"knative-operator-5797f68df9-lmw8c","knative.dev/controller":"knative.dev.operator.pkg.reconciler.knativeserving.Reconciler","knative.dev/kind":"operator.knative.dev.KnativeServing","knative.dev/traceid":"0f01d0c0-8482-4980-9129-651122f20dc3","knative.dev/key":"knative-serving/knative-serving","targetMethod":"ReconcileKind","error":"failed to apply non rbac manifest: Internal error occurred: failed calling webhook \"webhook.serving.knative.dev\": failed to call webhook: Post \"https://webhook.knative-serving.svc:443/?timeout=10s\": dial tcp 10.109.239.115:443: connect: connection refused","stacktrace":"knative.dev/operator/pkg/client/injection/reconciler/operator/v1beta1/knativeserving.(*reconcilerImpl).Reconcile\n\tknative.dev/operator/pkg/client/injection/reconciler/operator/v1beta1/knativeserving/reconciler.go:295\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/pkg@v0.0.0-20250117084104-c43477f0052b/controller/controller.go:540\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\tknative.dev/pkg@v0.0.0-20250117084104-c43477f0052b/controller/controller.go:489"}
```
Downstream we see a similar issue:
```
{"severity":"ERROR","timestamp":"2025-03-05T14:51:27.11460294Z","logger":"knative-operator","caller":"knativeserving/reconciler.go:295","message":"Returned an error","commit":"437a902","knative.dev/pod":"knative-operator-webhook-7458bff575-l6gm9","knative.dev/controller":"knative.dev.operator.pkg.reconciler.knativeserving.Reconciler","knative.dev/kind":"operator.knative.dev.KnativeServing","knative.dev/traceid":"bcd23919-595c-40e0-9728-f6f89ff570db","knative.dev/key":"knative-serving/knative-serving","targetMethod":"ReconcileKind","error":"failed to apply non rbac manifest: Internal error occurred: failed calling webhook \"webhook.serving.knative.dev\": failed to call webhook: Post \"https://webhook.knative-serving.svc:443/?timeout=10s\": no endpoints available for service \"webhook\"","stacktrace":"knative.dev/operator/pkg/client/injection/reconciler/operator/v1beta1/knativeserving.(*reconcilerImpl).Reconcile\n\t/workspace/vendor/knative.dev/operator/pkg/client/injection/reconciler/operator/v1beta1/knativeserving/reconciler.go:295\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\t/workspace/vendor/knative.dev/pkg/controller/controller.go:540\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\t/workspace/vendor/knative.dev/pkg/controller/controller.go:489"}
```

**Expected behavior**
A clear and concise description of what you expected to happen.
Manifest install should enforce proper order and webhook dependent resources should be deployed only after webhook services are ready. 

**To Reproduce**
Steps to reproduce the behavior.
Install operator as usual and check the logs.
**Knative release version**
The Knative version, where you find this issue.

**Additional context**
Add any other context about the problem here such as proposed priority

This has caused issues in other scenarios where there was a non clean uninstall (validation webhook configs were still around) and due to the validation on deletion (Serving has that for legacy reasons), the webhook was called even if there was no deployment available.

Pull request:
Wait for deployments
<!-- 
Are you using Knative? If you do, we would love to know!
https://github.com/knative/community/issues/new?template=ADOPTERS.yaml&title=%5BADOPTERS%5D%3A+%24%7BCOMPANY+NAME+HERE%7D
-->

<!--
Request Prow to automatically lint any go code in this PR:

/lint
-->

Fixes #2009 

## Proposed Changes

* Skip applying webhook resources until deployments are available. This makes sure resources can be reconciled properly and any resources depending on webhooks will be deployed after webhooks are up.


**Release Note**

<!-- Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE". -->

```release-note
Proper order is enforced now during manifest installation.
```


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/images.go b/pkg/reconciler/common/images.go
index 12ebd53ba..18721e6f8 100644
--- a/pkg/reconciler/common/images.go
+++ b/pkg/reconciler/common/images.go
@@ -121,6 +121,7 @@ func ImageTransform(registry *base.Registry, log *zap.SugaredLogger) mf.Transfor
 				env := &container.Env[j]
 				if image, ok := registry.Override[env.Name]; ok {
 					env.Value = image
+					env.ValueFrom = nil
 				}
 			}
 		}
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)
 		})