feat(kumacp) Delete related mesh resources #312
Conversation
| @@ -40,6 +43,9 @@ func (r *MeshReconciler) Reconcile(req kube_ctrl.Request) (kube_ctrl.Result, err | |||
| mesh := &mesh_k8s.Mesh{} | |||
| if err := r.Get(ctx, req.NamespacedName, mesh); err != nil { | |||
| if kube_apierrs.IsNotFound(err) { | |||
| if err := r.ResourceManager.Delete(ctx, &mesh_core.MeshResource{}, store.DeleteByKey(req.NamespacedName.Namespace, req.Name, req.Name)); err != nil { | |||
| @@ -40,6 +43,9 @@ func (r *MeshReconciler) Reconcile(req kube_ctrl.Request) (kube_ctrl.Result, err | |||
| mesh := &mesh_k8s.Mesh{} | |||
| if err := r.Get(ctx, req.NamespacedName, mesh); err != nil { | |||
| if kube_apierrs.IsNotFound(err) { | |||
| if err := r.ResourceManager.Delete(ctx, &mesh_core.MeshResource{}, store.DeleteByKey(req.NamespacedName.Namespace, req.Name, req.Name)); err != nil { | |||
There was a problem hiding this comment.
should we also check for if kube_apierrs.IsNotFound(err) ?
| @@ -30,7 +30,8 @@ import ( | |||
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | |||
|
|
|||
| // +kubebuilder:scaffold:imports | |||
| sample_v1alpha1 "github.com/Kong/kuma/pkg/plugins/resources/k8s/native/test/api/sample/v1alpha1" | |||
| mesh_proto "github.com/Kong/kuma/pkg/plugins/resources/k8s/native/api/v1alpha1" | |||
There was a problem hiding this comment.
usage of _proto suffix is misleading in this case (it's used to refer to Protobuf definitions)
| @@ -13,6 +13,7 @@ type ResourceStore interface { | |||
| Create(context.Context, model.Resource, ...CreateOptionsFunc) error | |||
| Update(context.Context, model.Resource, ...UpdateOptionsFunc) error | |||
| Delete(context.Context, model.Resource, ...DeleteOptionsFunc) error | |||
| DeleteMany(context.Context, ...DeleteManyOptionsFunc) error | |||
There was a problem hiding this comment.
I think, DeleteAll would be a more representative name
| @@ -88,6 +89,9 @@ func (s *strictResourceStore) Delete(ctx context.Context, r model.Resource, fs . | |||
| } | |||
| return s.delegate.Delete(ctx, r, fs...) | |||
| } | |||
| func (s *strictResourceStore) DeleteMany(ctx context.Context, fs ...DeleteManyOptionsFunc) error { | |||
| return s.delegate.DeleteMany(ctx, fs...) | |||
There was a problem hiding this comment.
DeleteMany with opts.Mesh == "" is too dangerous.
I would suggest to forbid it inside strictResourceStore
| opts := store.NewDeleteManyOptions(fs...) | ||
| statement := `DELETE FROM resources` | ||
| var statementArgs []interface{} | ||
| if opts.Mesh != "" { |
There was a problem hiding this comment.
Support for opts.Mesh == "" is too dangerous (I don't think somebody will ever need this feature)
| return errors.Wrapf(err, "failed to list resources of type %s", list.GetObjectKind().GroupVersionKind().String()) | ||
| } | ||
| for _, obj := range list.GetItems() { | ||
| if opts.Mesh == "" || opts.Mesh == obj.GetMesh() { |
There was a problem hiding this comment.
Support for opts.Mesh == "" is too dangerous (I don't think somebody will ever need this feature)
| @@ -93,6 +94,23 @@ func (s *KubernetesStore) Delete(ctx context.Context, r core_model.Resource, fs | |||
| } | |||
| return nil | |||
| } | |||
| func (s *KubernetesStore) DeleteMany(ctx context.Context, fs ...store.DeleteManyOptionsFunc) error { | |||
There was a problem hiding this comment.
I don't think it's a good idea to remove resources without taking into account their type
| @@ -89,6 +91,10 @@ func (m *meshManager) Delete(ctx context.Context, resource core_model.Resource, | |||
| if err := m.builtinCaManager.Delete(ctx, name); err != nil { | |||
| return errors.Wrapf(err, "failed to delete Builtin CA for a given mesh") | |||
| } | |||
| // delete associated resources | |||
| if err := m.store.DeleteMany(ctx, core_store.DeleteManyByMesh(opts.Name)); err != nil { | |||
There was a problem hiding this comment.
I would expect the logic that iterates over all supported resource types in here (rather than in k8s store)
| @@ -13,6 +13,7 @@ type ResourceStore interface { | |||
| Create(context.Context, model.Resource, ...CreateOptionsFunc) error | |||
| Update(context.Context, model.Resource, ...UpdateOptionsFunc) error | |||
| Delete(context.Context, model.Resource, ...DeleteOptionsFunc) error | |||
| DeleteMany(context.Context, ...DeleteManyOptionsFunc) error | |||
There was a problem hiding this comment.
I think, DeleteMany operation should be implemented at ResourceManager level rather than at ResourceStore level. Resources should be deleted one-by-one by a ResourceManager.
For example, consider an audit feature where we would be required to maintain a log of all operations that modify state of resources. Another example would be complex delete logic per resource type (like in the case of Mesh)
|
Closing in favour of #332 |
Summary
Delete related mesh resources when mesh entity is deleted.
On K8S there is no option to delete many entities with given field, that's why we retrieve all of them and filter in memory.
Full changelog
Issues resolved
Fix #191