Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions pkg/reconciler/common/deployments.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package common
import (
"context"
"errors"
"fmt"

mf "github.com/manifestival/manifestival"
appsv1 "k8s.io/api/apps/v1"
Expand All @@ -27,6 +28,7 @@ import (
"k8s.io/client-go/kubernetes/scheme"

"knative.dev/operator/pkg/apis/operator/base"
"knative.dev/pkg/logging"
)

type deploymentsNotReadyError struct{}
Expand All @@ -43,6 +45,43 @@ func IsDeploymentsNotReadyError(err error) bool {
return errors.Is(err, deploymentsNotReadyError{})
}

// CheckWebhookDeployment checks if the webhook deployment is ready.
// This is needed before creating webhook-dependent resources (e.g., Certificates)
// because the ValidatingWebhookConfiguration with failurePolicy=Fail will reject
// Certificate creation if the webhook pod is not ready.
func CheckWebhookDeployment(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
logger := logging.FromContext(ctx)
logger.Debug("Checking webhook deployment")
status := instance.GetStatus()
webhookDeployment := manifest.Filter(mf.ByKind("Deployment"), mf.ByName("webhook"))

if len(webhookDeployment.Resources()) == 0 {
status.MarkInstallFailed("webhook deployment not found in manifest")
return fmt.Errorf("webhook deployment not found in manifest")
}

for _, u := range webhookDeployment.Resources() {
resource, err := manifest.Client.Get(&u)
if err != nil {
status.MarkDeploymentsNotReady([]string{"webhook"})
if apierrors.IsNotFound(err) {
return deploymentsNotReadyError{}
}
return err
}
deployment := &appsv1.Deployment{}
if err := scheme.Scheme.Convert(resource, deployment, nil); err != nil {
return err
}
if !isDeploymentAvailable(deployment) {
status.MarkDeploymentsNotReady([]string{"webhook"})
return deploymentsNotReadyError{}
}
}

return nil
}

// 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 {
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/common/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func InstallWebhookConfigs(ctx context.Context, manifest *mf.Manifest, instance

// InstallWebhookDependentResources applies the Webhook dependent resources updates the given status accordingly.
func InstallWebhookDependentResources(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
logging.FromContext(ctx).Debug("Installing webhook dependent resources")
status := instance.GetStatus()
if err := manifest.Filter(webhookDependentResources).Apply(); err != nil {
status.MarkInstallFailed(err.Error())
Expand Down
5 changes: 3 additions & 2 deletions pkg/reconciler/knativeserving/knativeserving.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,10 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1beta1.KnativeServi
r.appendExtensionManifests,
r.transform,
manifests.Install,
manifests.SetManifestPaths, // setting path right after applying manifests to populate paths
common.CheckDeployments,
manifests.SetManifestPaths, // setting path right after applying manifests to populate paths
common.CheckWebhookDeployment, // Wait for webhook to be ready before creating Certificate resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knativeeventing.go also need this function, please add it there as well.
The name of the deployment resource for eventing is "eventing-webhook", you probably need to make the name as a param to the function "CheckWebhookDeployment".

Copy link
Contributor Author

@linkvt linkvt Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about eventing needing this as well?

Serving needed it as there are two install steps where the first one blocked the second one (see line 126 and 129):

manifests.Install,
manifests.SetManifestPaths, // setting path right after applying manifests to populate paths
common.CheckDeployments,
common.InstallWebhookDependentResources,
common.MarkStatusSuccess,
common.DeleteObsoleteResources(ctx, ks, r.installed),
}

Eventing has only one install step which means nothing is broken (see line 143):

manifests.Install,
manifests.SetManifestPaths, // setting path right after applying manifests to populate paths
common.CheckDeployments,
common.MarkStatusSuccess,
common.DeleteObsoleteResources(ctx, ke, r.installed),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just deployed an KnativeEventing resource with the operator and a the feature transport-encryption: Permissive causing the Certificates to be created without any issues, I guess it works without further changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

common.InstallWebhookDependentResources,
common.CheckDeployments,
common.MarkStatusSuccess,
common.DeleteObsoleteResources(ctx, ks, r.installed),
}
Expand Down
Loading