-
Notifications
You must be signed in to change notification settings - Fork 73
Cleanup orphaned WasmPlugin objects #2053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ import ( | |
| istioapinetworkingv1alpha3 "istio.io/api/networking/v1alpha3" | ||
| istiov1beta1 "istio.io/api/type/v1beta1" | ||
| istioclientgonetworkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| k8stypes "k8s.io/apimachinery/pkg/types" | ||
|
|
@@ -35,6 +36,7 @@ import ( | |
| "github.com/kuadrant/kuadrant-operator/internal/wasm" | ||
| ) | ||
|
|
||
| //+kubebuilder:rbac:groups=extensions.istio.io,resources=wasmplugins,verbs=delete | ||
| //+kubebuilder:rbac:groups=networking.istio.io,resources=envoyfilters,verbs=get;list;watch;create;update;patch;delete | ||
|
|
||
| // IstioExtensionReconciler reconciles Istio EnvoyFilter custom resources for wasm plugin injection | ||
|
|
@@ -95,6 +97,7 @@ func (r *IstioExtensionReconciler) Reconcile(ctx context.Context, _ []controller | |
| } | ||
|
|
||
| modifiedGateways := make([]string, 0, len(gateways)) | ||
| var reconcileErr error | ||
|
|
||
| for _, gateway := range gateways { | ||
| gatewayKey := k8stypes.NamespacedName{Name: gateway.GetName(), Namespace: gateway.GetNamespace()} | ||
|
|
@@ -122,22 +125,30 @@ func (r *IstioExtensionReconciler) Reconcile(ctx context.Context, _ []controller | |
| desiredEnvoyFilterUnstructured, err := controller.Destruct(desiredEnvoyFilter) | ||
| if err != nil { | ||
| logger.Error(err, "failed to destruct envoyfilter object", "gateway", gatewayKey.String(), "envoyfilter", desiredEnvoyFilter) | ||
| reconcileErr = errors.Join(reconcileErr, fmt.Errorf("failed to destruct envoyfilter %s/%s: %w", gateway.GetNamespace(), desiredEnvoyFilter.GetName(), err)) | ||
| continue | ||
| } | ||
| if _, err = resource.Create(ctx, desiredEnvoyFilterUnstructured, metav1.CreateOptions{}); err != nil { | ||
| logger.Error(err, "failed to create envoyfilter object", "gateway", gatewayKey.String(), "envoyfilter", desiredEnvoyFilterUnstructured.Object) | ||
| // TODO: handle error | ||
| reconcileErr = errors.Join(reconcileErr, fmt.Errorf("failed to create envoyfilter %s/%s: %w", gateway.GetNamespace(), desiredEnvoyFilter.GetName(), err)) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Clean up old WasmPlugin for this specific gateway - temporary to be removed | ||
| wasmPluginName := wasm.ExtensionName(gateway.GetName()) | ||
| if err := r.client.Resource(kuadrantistio.WasmPluginsResource).Namespace(gateway.GetNamespace()).Delete(ctx, wasmPluginName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { | ||
| logger.Error(err, "failed to delete wasmplugin", "gateway", gatewayKey.String(), "wasmplugin", wasmPluginName) | ||
| reconcileErr = errors.Join(reconcileErr, fmt.Errorf("failed to delete wasmplugin %s/%s: %w", gateway.GetNamespace(), wasmPluginName, err)) | ||
| } | ||
|
|
||
| existingEnvoyFilter := existingEnvoyFilterObj.(*controller.RuntimeObject).Object.(*istioclientgonetworkingv1alpha3.EnvoyFilter) | ||
|
|
||
| // delete | ||
| if utils.IsObjectTaggedToDelete(desiredEnvoyFilter) && !utils.IsObjectTaggedToDelete(existingEnvoyFilter) { | ||
| if err := resource.Delete(ctx, existingEnvoyFilter.GetName(), metav1.DeleteOptions{}); err != nil { | ||
| logger.Error(err, "failed to delete envoyfilter object", "gateway", gatewayKey.String(), "envoyfilter", fmt.Sprintf("%s/%s", existingEnvoyFilter.GetNamespace(), existingEnvoyFilter.GetName())) | ||
| // TODO: handle error | ||
| reconcileErr = errors.Join(reconcileErr, fmt.Errorf("failed to delete envoyfilter %s/%s: %w", existingEnvoyFilter.GetNamespace(), existingEnvoyFilter.GetName(), err)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slightly worried about the proliferation of this pattern in the STOW reconciliation workflow. Failing the entire workflow because one task returned an error won't automatically trigger a retry as one coming from a controller-runtime background would expect. I sincerely believe we should give #2043 some attention.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recalled seeing that PR but that's my mistake for not investigating what it was doing before returning the error here. What would you advise to proceed here - a follow up removing the errors or leaving as is for the imminent patch release?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the error handling is not required for the patch which is more about deleting the WasmPlugin leftovers, right? I would rollback ed89ee9.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right, luckily I kept the changes separate here so here's a follow up to revert that commit only #2067 |
||
| } | ||
| continue | ||
| } | ||
|
|
@@ -159,13 +170,13 @@ func (r *IstioExtensionReconciler) Reconcile(ctx context.Context, _ []controller | |
| } | ||
| if _, err = resource.Update(ctx, existingEnvoyFilterUnstructured, metav1.UpdateOptions{}); err != nil { | ||
| logger.Error(err, "failed to update envoyfilter object", "gateway", gatewayKey.String(), "envoyfilter", existingEnvoyFilterUnstructured.Object) | ||
| // TODO: handle error | ||
| reconcileErr = errors.Join(reconcileErr, fmt.Errorf("failed to update envoyfilter %s/%s: %w", existingEnvoyFilter.GetNamespace(), existingEnvoyFilter.GetName(), err)) | ||
| } | ||
| } | ||
|
|
||
| state.Store(StateIstioExtensionsModified, modifiedGateways) | ||
|
|
||
| return nil | ||
| return reconcileErr | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| func (r *IstioExtensionReconciler) reconcileUpstreamClusters(ctx context.Context, topology *machinery.Topology, gateways []*machinery.Gateway) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.