From f3716b2eec3812e71f2f7daee32764ca8aaed8f3 Mon Sep 17 00:00:00 2001 From: Marcos Candeia Date: Wed, 26 Nov 2025 20:40:06 -0300 Subject: [PATCH 1/2] Adds deployment-id based rollout Signed-off-by: Marcos Candeia --- api/v1alpha1/decofile_types.go | 6 +- .../clusterrole-operator-manager-role.yaml | 16 + ...sourcedefinition-decofiles.deco.sites.yaml | 10 +- config/crd/bases/deco.sites_decofiles.yaml | 10 +- config/rbac/role.yaml | 16 + internal/controller/decofile_controller.go | 41 +-- internal/controller/notifier.go | 16 +- internal/webhook/v1/service_webhook.go | 293 ++++++++++++------ 8 files changed, 270 insertions(+), 138 deletions(-) diff --git a/api/v1alpha1/decofile_types.go b/api/v1alpha1/decofile_types.go index b8aec81..48880b2 100644 --- a/api/v1alpha1/decofile_types.go +++ b/api/v1alpha1/decofile_types.go @@ -39,10 +39,10 @@ type DecofileSpec struct { // +optional GitHub *GitHubSource `json:"github,omitempty"` - // Silent disables pod notifications when ConfigMap changes - // If true, pods will not be notified and must poll or restart to get updates + // DeploymentId is used for pod label matching (defaults to metadata.name if absent) + // Pods are queried using the app.deco/deploymentId label // +optional - Silent bool `json:"silent,omitempty"` + DeploymentId string `json:"deploymentId,omitempty"` } // InlineSource contains direct JSON configuration data diff --git a/chart/templates/clusterrole-operator-manager-role.yaml b/chart/templates/clusterrole-operator-manager-role.yaml index c709305..1957081 100644 --- a/chart/templates/clusterrole-operator-manager-role.yaml +++ b/chart/templates/clusterrole-operator-manager-role.yaml @@ -49,4 +49,20 @@ rules: verbs: - get - patch + - update +- apiGroups: + - serving.knative.dev + resources: + - services + verbs: + - get + - list + - patch + - update + - watch +- apiGroups: + - serving.knative.dev + resources: + - services/finalizers + verbs: - update \ No newline at end of file diff --git a/chart/templates/customresourcedefinition-decofiles.deco.sites.yaml b/chart/templates/customresourcedefinition-decofiles.deco.sites.yaml index 1fa1512..1e6f7d1 100644 --- a/chart/templates/customresourcedefinition-decofiles.deco.sites.yaml +++ b/chart/templates/customresourcedefinition-decofiles.deco.sites.yaml @@ -38,6 +38,11 @@ spec: spec: description: DecofileSpec defines the desired state of Decofile. properties: + deploymentId: + description: |- + DeploymentId is used for pod label matching (defaults to metadata.name if absent) + Pods are queried using the app.deco/deploymentId label + type: string github: description: GitHub contains repository information (used when source=github) properties: @@ -78,11 +83,6 @@ spec: required: - value type: object - silent: - description: |- - Silent disables pod notifications when ConfigMap changes - If true, pods will not be notified and must poll or restart to get updates - type: boolean source: description: Source specifies where to get the configuration data enum: diff --git a/config/crd/bases/deco.sites_decofiles.yaml b/config/crd/bases/deco.sites_decofiles.yaml index 949de78..4ae0913 100644 --- a/config/crd/bases/deco.sites_decofiles.yaml +++ b/config/crd/bases/deco.sites_decofiles.yaml @@ -39,6 +39,11 @@ spec: spec: description: DecofileSpec defines the desired state of Decofile. properties: + deploymentId: + description: |- + DeploymentId is used for pod label matching (defaults to metadata.name if absent) + Pods are queried using the app.deco/deploymentId label + type: string github: description: GitHub contains repository information (used when source=github) properties: @@ -79,11 +84,6 @@ spec: required: - value type: object - silent: - description: |- - Silent disables pod notifications when ConfigMap changes - If true, pods will not be notified and must poll or restart to get updates - type: boolean source: description: Source specifies where to get the configuration data enum: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index c0276c4..474b174 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -51,3 +51,19 @@ rules: - get - patch - update +- apiGroups: + - serving.knative.dev + resources: + - services + verbs: + - get + - list + - patch + - update + - watch +- apiGroups: + - serving.knative.dev + resources: + - services/finalizers + verbs: + - update diff --git a/internal/controller/decofile_controller.go b/internal/controller/decofile_controller.go index 835849e..10b33f4 100644 --- a/internal/controller/decofile_controller.go +++ b/internal/controller/decofile_controller.go @@ -235,8 +235,14 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } } + // Determine deploymentId (default to decofile name if not specified) + deploymentId := decofile.Spec.DeploymentId + if deploymentId == "" { + deploymentId = decofile.Name + } + // Reset PodsNotified condition when change is detected (before notifying) - if dataChanged && !decofile.Spec.Silent { + if dataChanged { // Set condition to InProgress before attempting notification tempDecofile := &decositesv1alpha1.Decofile{} err = r.Get(ctx, req.NamespacedName, tempDecofile) @@ -262,28 +268,23 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } } - // Notify pods if ConfigMap data changed (unless silent mode is enabled) + // Notify pods if ConfigMap data changed var podsNotified bool var notificationError string if dataChanged { - if decofile.Spec.Silent { - log.Info("ConfigMap data changed but notifications disabled (silent mode)", "timestamp", timestamp) + log.Info("ConfigMap data changed, notifying pods", "timestamp", timestamp, "deploymentId", deploymentId) + + notifier := NewNotifier(r.Client) + err = notifier.NotifyPodsForDecofile(ctx, decofile.Namespace, deploymentId, timestamp, jsonContent) + if err != nil { + log.Error(err, "Failed to notify pods", "deploymentId", deploymentId) + notificationError = err.Error() podsNotified = false + // Don't return error - update status with failure condition } else { - log.Info("ConfigMap data changed, notifying pods", "timestamp", timestamp) - - notifier := NewNotifier(r.Client) - err = notifier.NotifyPodsForDecofile(ctx, decofile.Namespace, decofile.Name, timestamp, jsonContent) - if err != nil { - log.Error(err, "Failed to notify pods", "decofile", decofile.Name) - notificationError = err.Error() - podsNotified = false - // Don't return error - update status with failure condition - } else { - log.Info("Successfully notified all pods", "timestamp", timestamp) - podsNotified = true - } + log.Info("Successfully notified all pods", "timestamp", timestamp, "deploymentId", deploymentId) + podsNotified = true } } @@ -316,8 +317,8 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } updateCondition(freshDecofile, readyCondition) - // Update PodsNotified condition (only when not silent) - if !freshDecofile.Spec.Silent && dataChanged { + // Update PodsNotified condition + if dataChanged { var podsNotifiedCondition metav1.Condition // Include commit or timestamp in message for matching @@ -357,7 +358,7 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c log.Info("Successfully reconciled Decofile") // Return error if notifications failed (will requeue) - if dataChanged && !freshDecofile.Spec.Silent && !podsNotified { + if dataChanged && !podsNotified { return ctrl.Result{}, fmt.Errorf("failed to notify pods: %s", notificationError) } diff --git a/internal/controller/notifier.go b/internal/controller/notifier.go index 0b4d513..52fd703 100644 --- a/internal/controller/notifier.go +++ b/internal/controller/notifier.go @@ -35,7 +35,7 @@ const ( reloadTimeout = 30 * time.Second // 30s per pod (simple POST, no long-polling) maxRetries = 3 // 3 attempts per pod initialBackoff = 2 * time.Second - decofileLabel = "deco.sites/decofile" + deploymentIdLabel = "app.deco/deploymentId" maxNotificationTime = 2 * time.Minute // 2 min for entire batch notificationBatchSize = 10 // Parallel notification batch size (reduced to save memory) appContainerName = "app" @@ -72,30 +72,30 @@ func extractReloadToken(pod *corev1.Pod) string { return "" } -// NotifyPodsForDecofile notifies all pods using the given Decofile +// NotifyPodsForDecofile notifies all pods using the given deploymentId // that the ConfigMap has changed and they should reload. // Uses parallel batch processing with 2-minute timeout. -func (n *Notifier) NotifyPodsForDecofile(ctx context.Context, namespace, decofileName, timestamp, decofileContent string) error { +func (n *Notifier) NotifyPodsForDecofile(ctx context.Context, namespace, deploymentId, timestamp, decofileContent string) error { log := logf.FromContext(ctx) - log.Info("Notifying pods for Decofile", "decofile", decofileName, "namespace", namespace) + log.Info("Notifying pods for deploymentId", "deploymentId", deploymentId, "namespace", namespace) // Create timeout context for entire operation notifyCtx, cancel := context.WithTimeout(ctx, maxNotificationTime) defer cancel() - // List pods with the decofile label + // List pods with the deploymentId label podList := &corev1.PodList{} err := n.Client.List(notifyCtx, podList, client.InNamespace(namespace), - client.MatchingLabels{decofileLabel: decofileName}) + client.MatchingLabels{deploymentIdLabel: deploymentId}) if err != nil { - return fmt.Errorf("failed to list pods for decofile %s: %w", decofileName, err) + return fmt.Errorf("failed to list pods for deploymentId %s: %w", deploymentId, err) } if len(podList.Items) == 0 { - log.V(1).Info("No pods found for Decofile", "decofile", decofileName) + log.V(1).Info("No pods found for deploymentId", "deploymentId", deploymentId) return nil } diff --git a/internal/webhook/v1/service_webhook.go b/internal/webhook/v1/service_webhook.go index cdc5621..013d71e 100644 --- a/internal/webhook/v1/service_webhook.go +++ b/internal/webhook/v1/service_webhook.go @@ -27,6 +27,7 @@ import ( servingknativedevv1 "knative.dev/serving/pkg/apis/serving/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -40,14 +41,17 @@ const ( decoReleaseEnvVar = "DECO_RELEASE" decofileInjectAnnot = "deco.sites/decofile-inject" decofileMountPathAnnot = "deco.sites/decofile-mount-path" - decofileLabel = "deco.sites/decofile" + deploymentIdLabel = "app.deco/deploymentId" + decofileFinalizerName = "decofile.deco.sites/finalizer" ) // nolint:unused // log is for logging in this package. var servicelog = logf.Log.WithName("service-resource") -// +kubebuilder:rbac:groups=deco.sites,resources=decofiles,verbs=get;list;watch +// +kubebuilder:rbac:groups=deco.sites,resources=decofiles,verbs=get;list;watch;delete +// +kubebuilder:rbac:groups=serving.knative.dev,resources=services,verbs=get;list;watch;update;patch +// +kubebuilder:rbac:groups=serving.knative.dev,resources=services/finalizers,verbs=update // SetupServiceWebhookWithManager registers the webhook for Service in the manager. func SetupServiceWebhookWithManager(mgr ctrl.Manager) error { @@ -72,80 +76,80 @@ type ServiceCustomDefaulter struct { var _ webhook.CustomDefaulter = &ServiceCustomDefaulter{} -// Default implements webhook.CustomDefaulter so a webhook will be registered for the type Service. -func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { - service, ok := obj.(*servingknativedevv1.Service) - if !ok { - return nil // do nothing +// handleDeletion handles Service deletion and finalizer cleanup +func (d *ServiceCustomDefaulter) handleDeletion(ctx context.Context, service *servingknativedevv1.Service) error { + if !controllerutil.ContainsFinalizer(service, decofileFinalizerName) { + return nil } - servicelog.Info("Mutating Service", "name", service.GetName()) - // Check for deco.sites/decofile-inject annotation - if service.Annotations == nil { - return nil + // Cleanup: delete associated Decofile(s) + if err := d.cleanupDecofiles(ctx, service); err != nil { + servicelog.Error(err, "Failed to cleanup Decofiles", "service", service.Name) + return err } - injectAnnotation, exists := service.Annotations[decofileInjectAnnot] - if !exists || injectAnnotation == "" { - return nil + // Remove finalizer + controllerutil.RemoveFinalizer(service, decofileFinalizerName) + if err := d.Client.Update(ctx, service); err != nil { + return fmt.Errorf("failed to remove finalizer: %w", err) } + servicelog.Info("Removed finalizer from Service", "service", service.Name) + return nil +} - // Resolve Decofile name - decofileName := injectAnnotation - if injectAnnotation == "default" { - // Get site name from namespace by stripping "sites-" prefix - namespace := service.Namespace - if len(namespace) == 0 { - return fmt.Errorf("cannot resolve default Decofile: service has no namespace") - } +// getDeploymentId extracts deploymentId from Service labels +func (d *ServiceCustomDefaulter) getDeploymentId(service *servingknativedevv1.Service) (string, error) { + if service.Labels == nil { + return "", fmt.Errorf("service has deco.sites/decofile-inject annotation but no labels") + } - // Strip "sites-" prefix to get site name - const sitesPrefix = "sites-" - if len(namespace) > len(sitesPrefix) && namespace[:len(sitesPrefix)] == sitesPrefix { - siteName := namespace[len(sitesPrefix):] - decofileName = fmt.Sprintf("decofile-%s-main", siteName) - } else { - return fmt.Errorf("cannot resolve default Decofile: namespace %s does not start with 'sites-'", namespace) - } + deploymentId, exists := service.Labels[deploymentIdLabel] + if !exists || deploymentId == "" { + return "", fmt.Errorf("service has deco.sites/decofile-inject annotation but no app.deco/deploymentId label") } - // Fetch the Decofile - decofile := &decositesv1alpha1.Decofile{} - err := d.Client.Get(ctx, types.NamespacedName{ - Name: decofileName, - Namespace: service.Namespace, - }, decofile) + return deploymentId, nil +} + +// findDecofileByDeploymentId finds a Decofile matching the given deploymentId +func (d *ServiceCustomDefaulter) findDecofileByDeploymentId(ctx context.Context, namespace, deploymentId string) (*decositesv1alpha1.Decofile, error) { + decofileList := &decositesv1alpha1.DecofileList{} + err := d.Client.List(ctx, decofileList, client.InNamespace(namespace)) if err != nil { - return fmt.Errorf("failed to get Decofile %s: %w", decofileName, err) + return nil, fmt.Errorf("failed to list Decofiles: %w", err) } - // Check if ConfigMap is ready - if decofile.Status.ConfigMapName == "" { - return fmt.Errorf("decofile %s does not have a ConfigMap created yet", decofileName) + for i := range decofileList.Items { + df := &decofileList.Items[i] + dfDeploymentId := df.Spec.DeploymentId + if dfDeploymentId == "" { + dfDeploymentId = df.Name + } + if dfDeploymentId == deploymentId { + return df, nil + } } - // Get mount path from annotation or use default directory - mountDir := "/app/decofile" - if customPath, exists := service.Annotations[decofileMountPathAnnot]; exists { - mountDir = customPath - } + return nil, fmt.Errorf("no Decofile found with deploymentId %s in namespace %s", deploymentId, namespace) +} +// injectDecofileVolume injects the Decofile ConfigMap as a volume into the Service +func (d *ServiceCustomDefaulter) injectDecofileVolume(ctx context.Context, service *servingknativedevv1.Service, decofile *decositesv1alpha1.Decofile, mountDir string) error { // Check if ConfigMap is compressed to set correct file extension configMap := &corev1.ConfigMap{} - err = d.Client.Get(ctx, types.NamespacedName{ + err := d.Client.Get(ctx, types.NamespacedName{ Name: decofile.Status.ConfigMapName, Namespace: service.Namespace, }, configMap) fileExtension := "json" if err == nil { - // Check if compressed if _, hasCompressed := configMap.Data["decofile.bin"]; hasCompressed { fileExtension = "bin" } } - // Create DECO_RELEASE environment variable pointing to the correct file + // Create DECO_RELEASE environment variable decoReleaseValue := fmt.Sprintf("file://%s/decofile.%s", mountDir, fileExtension) // Ensure volumes array exists @@ -153,17 +157,31 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object service.Spec.Template.Spec.Volumes = []corev1.Volume{} } - // Check if volume already exists + // Add or update volume + d.addOrUpdateVolume(service, decofile.Status.ConfigMapName) + + // Find target container and add volumeMount + env vars + if len(service.Spec.Template.Spec.Containers) == 0 { + return fmt.Errorf("no containers found in Service spec") + } + + targetContainerIdx := d.findTargetContainer(service) + d.addOrUpdateVolumeMount(service, targetContainerIdx, mountDir) + d.addOrUpdateEnvVars(service, targetContainerIdx, decoReleaseValue) + + return nil +} + +// addOrUpdateVolume adds or updates the decofile volume +func (d *ServiceCustomDefaulter) addOrUpdateVolume(service *servingknativedevv1.Service, configMapName string) { volumeName := "decofile-config" volumeExists := false + for i, vol := range service.Spec.Template.Spec.Volumes { if vol.Name == volumeName { - // Update existing volume - use ConfigMap directly for file mounting service.Spec.Template.Spec.PodSpec.Volumes[i].VolumeSource = corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: decofile.Status.ConfigMapName, - }, + LocalObjectReference: corev1.LocalObjectReference{Name: configMapName}, }, } volumeExists = true @@ -172,48 +190,44 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object } if !volumeExists { - // Add new volume - use ConfigMap directly for file mounting service.Spec.Template.Spec.Volumes = append(service.Spec.Template.Spec.Volumes, corev1.Volume{ Name: volumeName, VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: decofile.Status.ConfigMapName, - }, + LocalObjectReference: corev1.LocalObjectReference{Name: configMapName}, }, }, }) } +} - // Find container and add volumeMount - if len(service.Spec.Template.Spec.Containers) == 0 { - return fmt.Errorf("no containers found in Service spec") - } - - // Find the "app" container or use first container - var targetContainerIdx int +// findTargetContainer finds the "app" container or returns 0 +func (d *ServiceCustomDefaulter) findTargetContainer(service *servingknativedevv1.Service) int { for i, container := range service.Spec.Template.Spec.Containers { if container.Name == appContainerName { - targetContainerIdx = i - break + return i } } + return 0 +} - // Add volumeMount as directory (no subPath so file updates propagate) +// addOrUpdateVolumeMount adds or updates the volume mount +func (d *ServiceCustomDefaulter) addOrUpdateVolumeMount(service *servingknativedevv1.Service, containerIdx int, mountDir string) { + volumeName := "decofile-config" mountExists := false - for i, mount := range service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].VolumeMounts { + + for i, mount := range service.Spec.Template.Spec.PodSpec.Containers[containerIdx].VolumeMounts { if mount.Name == volumeName { - // Update existing mount - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].VolumeMounts[i].MountPath = mountDir - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].VolumeMounts[i].SubPath = "" + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].VolumeMounts[i].MountPath = mountDir + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].VolumeMounts[i].SubPath = "" mountExists = true break } } if !mountExists { - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].VolumeMounts = append( - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].VolumeMounts, + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].VolumeMounts = append( + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].VolumeMounts, corev1.VolumeMount{ Name: volumeName, MountPath: mountDir, @@ -221,57 +235,142 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object }, ) } +} +// addOrUpdateEnvVars adds or updates environment variables +func (d *ServiceCustomDefaulter) addOrUpdateEnvVars(service *servingknativedevv1.Service, containerIdx int, decoReleaseValue string) { // Add DECO_RELEASE environment variable envExists := false - for i, env := range service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env { + for i, env := range service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env { if env.Name == decoReleaseEnvVar { - // Update existing env var - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env[i].Value = decoReleaseValue + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env[i].Value = decoReleaseValue envExists = true break } } if !envExists { - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env = append( - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env, - corev1.EnvVar{ - Name: decoReleaseEnvVar, - Value: decoReleaseValue, - }, + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env = append( + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env, + corev1.EnvVar{Name: decoReleaseEnvVar, Value: decoReleaseValue}, ) } - // Generate and add DECO_RELEASE_RELOAD_TOKEN environment variable + // Add DECO_RELEASE_RELOAD_TOKEN environment variable reloadToken := uuid.New().String() tokenEnvExists := false - for i, env := range service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env { + for i, env := range service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env { if env.Name == reloadTokenEnvVar { - // Update existing env var with new token - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env[i].Value = reloadToken + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env[i].Value = reloadToken tokenEnvExists = true break } } if !tokenEnvExists { - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env = append( - service.Spec.Template.Spec.PodSpec.Containers[targetContainerIdx].Env, - corev1.EnvVar{ - Name: reloadTokenEnvVar, - Value: reloadToken, - }, + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env = append( + service.Spec.Template.Spec.PodSpec.Containers[containerIdx].Env, + corev1.EnvVar{Name: reloadTokenEnvVar, Value: reloadToken}, ) } +} + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the type Service. +func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { + service, ok := obj.(*servingknativedevv1.Service) + if !ok { + return nil // do nothing + } + servicelog.Info("Mutating Service", "name", service.GetName()) + + // Handle finalizer logic for deletion + if !service.DeletionTimestamp.IsZero() { + return d.handleDeletion(ctx, service) + } + + // Check for deco.sites/decofile-inject annotation (boolean) + if service.Annotations == nil { + return nil + } + + injectAnnotation, exists := service.Annotations[decofileInjectAnnot] + if !exists || injectAnnotation != "true" { + return nil + } + + // Get deploymentId from Service labels + deploymentId, err := d.getDeploymentId(service) + if err != nil { + return err + } + + // Find matching Decofile + decofile, err := d.findDecofileByDeploymentId(ctx, service.Namespace, deploymentId) + if err != nil { + return err + } + + // Check if ConfigMap is ready + if decofile.Status.ConfigMapName == "" { + return fmt.Errorf("decofile %s does not have a ConfigMap created yet", decofile.Name) + } + + // Get mount path from annotation or use default directory + mountDir := "/app/decofile" + if customPath, exists := service.Annotations[decofileMountPathAnnot]; exists { + mountDir = customPath + } + + // Inject Decofile volume and env vars + if err := d.injectDecofileVolume(ctx, service, decofile, mountDir); err != nil { + return err + } + + // Add finalizer to Service for cleanup + if !controllerutil.ContainsFinalizer(service, decofileFinalizerName) { + controllerutil.AddFinalizer(service, decofileFinalizerName) + servicelog.Info("Added finalizer to Service", "service", service.Name) + } - // Add label to pod template for Decofile tracking - if service.Spec.Template.Labels == nil { - service.Spec.Template.Labels = make(map[string]string) + servicelog.Info("Successfully injected Decofile into Service", "service", service.Name, "deploymentId", deploymentId, "configmap", decofile.Status.ConfigMapName) + + return nil +} + +// cleanupDecofiles deletes all Decofiles associated with the Service's deploymentId +func (d *ServiceCustomDefaulter) cleanupDecofiles(ctx context.Context, service *servingknativedevv1.Service) error { + // Get deploymentId from Service labels + if service.Labels == nil { + return nil // No labels, nothing to clean up } - service.Spec.Template.Labels[decofileLabel] = decofileName - servicelog.Info("Successfully injected Decofile into Service", "service", service.Name, "decofile", decofileName, "configmap", decofile.Status.ConfigMapName) + deploymentId, exists := service.Labels[deploymentIdLabel] + if !exists || deploymentId == "" { + return nil // No deploymentId, nothing to clean up + } + + // List all Decofiles in namespace + decofileList := &decositesv1alpha1.DecofileList{} + err := d.Client.List(ctx, decofileList, client.InNamespace(service.Namespace)) + if err != nil { + return fmt.Errorf("failed to list Decofiles: %w", err) + } + + // Delete Decofiles with matching deploymentId + for i := range decofileList.Items { + df := &decofileList.Items[i] + dfDeploymentId := df.Spec.DeploymentId + if dfDeploymentId == "" { + dfDeploymentId = df.Name + } + if dfDeploymentId == deploymentId { + servicelog.Info("Deleting Decofile", "decofile", df.Name, "deploymentId", deploymentId) + if err := d.Client.Delete(ctx, df); err != nil { + servicelog.Error(err, "Failed to delete Decofile", "decofile", df.Name) + // Continue with other Decofiles even if one fails + } + } + } return nil } From d82d1ec497496f1eef6d248a0a8e7665dd75ee2f Mon Sep 17 00:00:00 2001 From: Marcos Candeia Date: Wed, 26 Nov 2025 20:59:30 -0300 Subject: [PATCH 2/2] Fixes potential issues in production Signed-off-by: Marcos Candeia --- .../clusterrole-operator-manager-role.yaml | 10 +- ...ator-validating-webhook-configuration.yaml | 19 +++ cmd/main.go | 4 + config/rbac/role.yaml | 8 -- config/webhook/manifests.yaml | 19 +++ internal/webhook/v1/decofile_webhook.go | 122 ++++++++++++++++++ internal/webhook/v1/service_webhook.go | 91 ++----------- 7 files changed, 179 insertions(+), 94 deletions(-) create mode 100644 internal/webhook/v1/decofile_webhook.go diff --git a/chart/templates/clusterrole-operator-manager-role.yaml b/chart/templates/clusterrole-operator-manager-role.yaml index 1957081..993fb50 100644 --- a/chart/templates/clusterrole-operator-manager-role.yaml +++ b/chart/templates/clusterrole-operator-manager-role.yaml @@ -57,12 +57,4 @@ rules: verbs: - get - list - - patch - - update - - watch -- apiGroups: - - serving.knative.dev - resources: - - services/finalizers - verbs: - - update \ No newline at end of file + - watch \ No newline at end of file diff --git a/chart/templates/validatingwebhookconfiguration-operator-validating-webhook-configuration.yaml b/chart/templates/validatingwebhookconfiguration-operator-validating-webhook-configuration.yaml index 2e09a6c..533b0e8 100644 --- a/chart/templates/validatingwebhookconfiguration-operator-validating-webhook-configuration.yaml +++ b/chart/templates/validatingwebhookconfiguration-operator-validating-webhook-configuration.yaml @@ -6,6 +6,25 @@ metadata: cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Release.Name }}-serving-cert name: {{ .Release.Name }}-validating-webhook-configuration webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: {{ .Release.Name }}-webhook-service + namespace: {{ .Release.Namespace }} + path: /validate-deco-sites-v1alpha1-decofile + failurePolicy: Fail + name: vdecofile.kb.io + rules: + - apiGroups: + - deco.sites + apiVersions: + - v1alpha1 + operations: + - DELETE + resources: + - decofiles + sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/cmd/main.go b/cmd/main.go index 0bdbaf3..535cbc5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -219,6 +219,10 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "Service") os.Exit(1) } + if err := webhookv1.SetupDecofileWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Decofile") + os.Exit(1) + } } // +kubebuilder:scaffold:builder diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 474b174..d547341 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -58,12 +58,4 @@ rules: verbs: - get - list - - patch - - update - watch -- apiGroups: - - serving.knative.dev - resources: - - services/finalizers - verbs: - - update diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 1469f26..b0c8421 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -30,6 +30,25 @@ kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-deco-sites-v1alpha1-decofile + failurePolicy: Fail + name: vdecofile.kb.io + rules: + - apiGroups: + - deco.sites + apiVersions: + - v1alpha1 + operations: + - DELETE + resources: + - decofiles + sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/internal/webhook/v1/decofile_webhook.go b/internal/webhook/v1/decofile_webhook.go new file mode 100644 index 0000000..ad2aa49 --- /dev/null +++ b/internal/webhook/v1/decofile_webhook.go @@ -0,0 +1,122 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + servingknativedevv1 "knative.dev/serving/pkg/apis/serving/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + decositesv1alpha1 "github.com/deco-sites/decofile-operator/api/v1alpha1" +) + +// nolint:unused +var decofilelog = logf.Log.WithName("decofile-resource") + +// +kubebuilder:rbac:groups=serving.knative.dev,resources=services,verbs=get;list;watch + +// SetupDecofileWebhookWithManager registers the webhook for Decofile in the manager. +func SetupDecofileWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr).For(&decositesv1alpha1.Decofile{}). + WithValidator(&DecofileCustomValidator{Client: mgr.GetClient()}). + Complete() +} + +// +kubebuilder:webhook:path=/validate-deco-sites-v1alpha1-decofile,mutating=false,failurePolicy=fail,sideEffects=None,groups=deco.sites,resources=decofiles,verbs=delete,versions=v1alpha1,name=vdecofile.kb.io,admissionReviewVersions=v1 + +// DecofileCustomValidator struct is responsible for validating the Decofile resource +// when it is deleted. +// +// NOTE: The +kubebuilder:object:generate=false marker prevents controller-gen from generating DeepCopy methods, +// as this struct is used only for temporary operations and does not need to be deeply copied. +type DecofileCustomValidator struct { + Client client.Client +} + +var _ webhook.CustomValidator = &DecofileCustomValidator{} + +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type Decofile. +func (v *DecofileCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + // No validation on create + return nil, nil +} + +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type Decofile. +func (v *DecofileCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + // No validation on update + return nil, nil +} + +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type Decofile. +func (v *DecofileCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + decofile, ok := obj.(*decositesv1alpha1.Decofile) + if !ok { + return nil, fmt.Errorf("expected a Decofile object but got %T", obj) + } + + decofilelog.Info("Validating Decofile deletion", "name", decofile.Name, "namespace", decofile.Namespace) + + // Determine deploymentId for this Decofile + deploymentId := decofile.Spec.DeploymentId + if deploymentId == "" { + deploymentId = decofile.Name + } + + // Check if any Knative Services are using this Decofile + serviceList := &servingknativedevv1.ServiceList{} + err := v.Client.List(ctx, serviceList, client.InNamespace(decofile.Namespace)) + if err != nil { + // If we can't list services, allow deletion (fail-open to avoid blocking operations) + decofilelog.Error(err, "Failed to list Services during Decofile validation, allowing deletion") + return nil, nil + } + + // Check each Service for matching deploymentId and injection annotation + var usingServices []string + for i := range serviceList.Items { + svc := &serviceList.Items[i] + + // Check if Service has injection enabled + if svc.Annotations != nil && svc.Annotations[decofileInjectAnnot] == "true" { + // Check if Service's deploymentId matches this Decofile + if svc.Labels != nil { + svcDeploymentId := svc.Labels[deploymentIdLabel] + if svcDeploymentId == deploymentId { + usingServices = append(usingServices, svc.Name) + } + } + } + } + + if len(usingServices) > 0 { + return admission.Warnings{ + fmt.Sprintf("Decofile %s is currently in use by %d Service(s)", decofile.Name, len(usingServices)), + }, + fmt.Errorf("cannot delete Decofile %s: still in use by Service(s): %v. Remove deco.sites/decofile-inject annotation or delete the Service(s) first", + decofile.Name, usingServices) + } + + decofilelog.Info("Decofile deletion allowed - not in use", "name", decofile.Name) + return nil, nil +} diff --git a/internal/webhook/v1/service_webhook.go b/internal/webhook/v1/service_webhook.go index 013d71e..a5edc23 100644 --- a/internal/webhook/v1/service_webhook.go +++ b/internal/webhook/v1/service_webhook.go @@ -27,7 +27,6 @@ import ( servingknativedevv1 "knative.dev/serving/pkg/apis/serving/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -42,16 +41,13 @@ const ( decofileInjectAnnot = "deco.sites/decofile-inject" decofileMountPathAnnot = "deco.sites/decofile-mount-path" deploymentIdLabel = "app.deco/deploymentId" - decofileFinalizerName = "decofile.deco.sites/finalizer" ) // nolint:unused // log is for logging in this package. var servicelog = logf.Log.WithName("service-resource") -// +kubebuilder:rbac:groups=deco.sites,resources=decofiles,verbs=get;list;watch;delete -// +kubebuilder:rbac:groups=serving.knative.dev,resources=services,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups=serving.knative.dev,resources=services/finalizers,verbs=update +// +kubebuilder:rbac:groups=deco.sites,resources=decofiles,verbs=get;list;watch // SetupServiceWebhookWithManager registers the webhook for Service in the manager. func SetupServiceWebhookWithManager(mgr ctrl.Manager) error { @@ -76,27 +72,6 @@ type ServiceCustomDefaulter struct { var _ webhook.CustomDefaulter = &ServiceCustomDefaulter{} -// handleDeletion handles Service deletion and finalizer cleanup -func (d *ServiceCustomDefaulter) handleDeletion(ctx context.Context, service *servingknativedevv1.Service) error { - if !controllerutil.ContainsFinalizer(service, decofileFinalizerName) { - return nil - } - - // Cleanup: delete associated Decofile(s) - if err := d.cleanupDecofiles(ctx, service); err != nil { - servicelog.Error(err, "Failed to cleanup Decofiles", "service", service.Name) - return err - } - - // Remove finalizer - controllerutil.RemoveFinalizer(service, decofileFinalizerName) - if err := d.Client.Update(ctx, service); err != nil { - return fmt.Errorf("failed to remove finalizer: %w", err) - } - servicelog.Info("Removed finalizer from Service", "service", service.Name) - return nil -} - // getDeploymentId extracts deploymentId from Service labels func (d *ServiceCustomDefaulter) getDeploymentId(service *servingknativedevv1.Service) (string, error) { if service.Labels == nil { @@ -283,11 +258,6 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object } servicelog.Info("Mutating Service", "name", service.GetName()) - // Handle finalizer logic for deletion - if !service.DeletionTimestamp.IsZero() { - return d.handleDeletion(ctx, service) - } - // Check for deco.sites/decofile-inject annotation (boolean) if service.Annotations == nil { return nil @@ -304,15 +274,19 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object return err } - // Find matching Decofile + // Find matching Decofile (non-blocking - allow Service creation even if not found) decofile, err := d.findDecofileByDeploymentId(ctx, service.Namespace, deploymentId) if err != nil { - return err + servicelog.Info("Decofile not found, skipping injection (Service will be created without Decofile)", + "service", service.Name, "deploymentId", deploymentId, "reason", err.Error()) + return nil // Allow Service creation } - // Check if ConfigMap is ready + // Check if ConfigMap is ready (non-blocking) if decofile.Status.ConfigMapName == "" { - return fmt.Errorf("decofile %s does not have a ConfigMap created yet", decofile.Name) + servicelog.Info("Decofile ConfigMap not ready yet, skipping injection", + "service", service.Name, "decofile", decofile.Name) + return nil // Allow Service creation } // Get mount path from annotation or use default directory @@ -326,55 +300,18 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object return err } - // Add finalizer to Service for cleanup - if !controllerutil.ContainsFinalizer(service, decofileFinalizerName) { - controllerutil.AddFinalizer(service, decofileFinalizerName) - servicelog.Info("Added finalizer to Service", "service", service.Name) + // Explicitly add deploymentId label to pod template for notification + // (Don't rely on Knative label propagation) + if service.Spec.Template.Labels == nil { + service.Spec.Template.Labels = make(map[string]string) } + service.Spec.Template.Labels[deploymentIdLabel] = deploymentId servicelog.Info("Successfully injected Decofile into Service", "service", service.Name, "deploymentId", deploymentId, "configmap", decofile.Status.ConfigMapName) return nil } -// cleanupDecofiles deletes all Decofiles associated with the Service's deploymentId -func (d *ServiceCustomDefaulter) cleanupDecofiles(ctx context.Context, service *servingknativedevv1.Service) error { - // Get deploymentId from Service labels - if service.Labels == nil { - return nil // No labels, nothing to clean up - } - - deploymentId, exists := service.Labels[deploymentIdLabel] - if !exists || deploymentId == "" { - return nil // No deploymentId, nothing to clean up - } - - // List all Decofiles in namespace - decofileList := &decositesv1alpha1.DecofileList{} - err := d.Client.List(ctx, decofileList, client.InNamespace(service.Namespace)) - if err != nil { - return fmt.Errorf("failed to list Decofiles: %w", err) - } - - // Delete Decofiles with matching deploymentId - for i := range decofileList.Items { - df := &decofileList.Items[i] - dfDeploymentId := df.Spec.DeploymentId - if dfDeploymentId == "" { - dfDeploymentId = df.Name - } - if dfDeploymentId == deploymentId { - servicelog.Info("Deleting Decofile", "decofile", df.Name, "deploymentId", deploymentId) - if err := d.Client.Delete(ctx, df); err != nil { - servicelog.Error(err, "Failed to delete Decofile", "decofile", df.Name) - // Continue with other Decofiles even if one fails - } - } - } - - return nil -} - // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. // NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here. // Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.