diff --git a/internal/controller/clusteraddon_controller.go b/internal/controller/clusteraddon_controller.go index 8f8412282..fcdea7bbd 100644 --- a/internal/controller/clusteraddon_controller.go +++ b/internal/controller/clusteraddon_controller.go @@ -238,7 +238,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re if clusterAddon.Spec.Version != metadata.Versions.Components.ClusterAddon { clusterAddon.Status.Ready = false - shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in, true) + shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in) if err != nil { conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply: %s", err.Error()) return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err) @@ -273,7 +273,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re // if condition is false we have not yet successfully applied the helm chart if conditions.IsFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition) { - shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in, true) + shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in) if err != nil { conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply: %s", err.Error()) return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err) @@ -718,7 +718,7 @@ func (r *ClusterAddonReconciler) getAddonStagesInput(restConfig *rest.Config, cl return addonStages, nil } -func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx context.Context, in *templateAndApplyClusterAddonInput, shouldDelete bool) (bool, error) { +func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx context.Context, in *templateAndApplyClusterAddonInput) (bool, error) { clusterAddonChart := in.clusterAddonChartPath var shouldRequeue bool @@ -734,7 +734,7 @@ func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx conte kubeClient := r.KubeClientFactory.NewClient(clusterAddonNamespace, in.restConfig) - newResources, shouldRequeue, err := kubeClient.Apply(ctx, helmTemplate, in.clusterAddon.Status.Resources, shouldDelete) + newResources, shouldRequeue, err := kubeClient.Apply(ctx, helmTemplate, in.clusterAddon.Status.Resources) if err != nil { return false, fmt.Errorf("failed to apply objects from cluster addon Helm chart: %w", err) } diff --git a/internal/controller/clusteraddon_controller_test.go b/internal/controller/clusteraddon_controller_test.go index b73c3a9a4..9525194cf 100644 --- a/internal/controller/clusteraddon_controller_test.go +++ b/internal/controller/clusteraddon_controller_test.go @@ -61,7 +61,7 @@ var _ = Describe("ClusterAddonReconciler", func() { }, } - testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) + testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) }) AfterEach(func() { @@ -240,7 +240,7 @@ var _ = Describe("ClusterAddonReconciler", func() { }, timeout, interval).Should(BeTrue()) By("checking Update method was called") - Expect(testEnv.KubeClient.AssertCalled(GinkgoT(), "Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything)).To(BeTrue()) + Expect(testEnv.KubeClient.AssertCalled(GinkgoT(), "Apply", mock.Anything, mock.Anything, mock.Anything)).To(BeTrue()) }) It("should not call update if the ClusterAddon version does not change in the ClusterClass update", func() { diff --git a/internal/controller/clusteraddoncreate_controller_test.go b/internal/controller/clusteraddoncreate_controller_test.go index 537f0a5e0..614998caf 100644 --- a/internal/controller/clusteraddoncreate_controller_test.go +++ b/internal/controller/clusteraddoncreate_controller_test.go @@ -69,7 +69,7 @@ var _ = Describe("ClusterAddonCreateReconciler", func() { key = types.NamespacedName{Name: fmt.Sprintf("cluster-addon-%s", cluster.Name), Namespace: testNs.Name} - testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) + testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) }) AfterEach(func() { diff --git a/internal/controller/clusterstackrelease_controller.go b/internal/controller/clusterstackrelease_controller.go index 44ee08d20..e3fd87280 100644 --- a/internal/controller/clusterstackrelease_controller.go +++ b/internal/controller/clusterstackrelease_controller.go @@ -288,7 +288,7 @@ func (r *ClusterStackReleaseReconciler) templateAndApply(ctx context.Context, re return false, fmt.Errorf("template is empty") } - newResources, shouldRequeue, err := kubeClient.Apply(ctx, template, clusterStackRelease.Status.Resources, true) + newResources, shouldRequeue, err := kubeClient.Apply(ctx, template, clusterStackRelease.Status.Resources) if err != nil { conditions.MarkFalse(clusterStackRelease, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply") return false, fmt.Errorf("failed to apply cluster class helm chart: %w", err) diff --git a/internal/controller/clusterstackrelease_controller_test.go b/internal/controller/clusterstackrelease_controller_test.go index 1e04e8101..70c5ab06e 100644 --- a/internal/controller/clusterstackrelease_controller_test.go +++ b/internal/controller/clusterstackrelease_controller_test.go @@ -311,7 +311,7 @@ var _ = Describe("ClusterStackRelease validation", func() { }, } - testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) + testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil) }) AfterEach(func() { diff --git a/pkg/kube/fake/kube.go b/pkg/kube/fake/kube.go index a6e216320..4e3254218 100644 --- a/pkg/kube/fake/kube.go +++ b/pkg/kube/fake/kube.go @@ -41,7 +41,7 @@ func NewFactory() kubeclient.Factory { } // Apply applies the given yaml object. -func (*kube) Apply(_ context.Context, _ []byte, _ []*csov1alpha1.Resource, _ bool) (_ []*csov1alpha1.Resource, _ bool, _ error) { +func (*kube) Apply(_ context.Context, _ []byte, _ []*csov1alpha1.Resource) (_ []*csov1alpha1.Resource, _ bool, _ error) { return nil, false, nil } diff --git a/pkg/kube/kube.go b/pkg/kube/kube.go index dc70d4043..e891785e3 100644 --- a/pkg/kube/kube.go +++ b/pkg/kube/kube.go @@ -37,7 +37,7 @@ type kube struct { // Client has all the meathod for helm chart kube operation. type Client interface { - Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource, shouldDelete bool) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) + Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) Delete(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) ApplyNewClusterStack(ctx context.Context, oldTemplate, newTemplate []byte) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) @@ -163,7 +163,7 @@ func (k *kube) DeleteNewClusterStack(ctx context.Context, template []byte) (newR return newResources, shouldRequeue, nil } -func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource, shouldDelete bool) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) { +func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) { logger := log.FromContext(ctx) objs, err := parseK8sYaml(template) @@ -212,32 +212,29 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1 newResources = append(newResources, resource) } - // TODO: cleanup shouldDelete - if shouldDelete { - // make a diff between new objs and oldResources to find out - // a) if an object is in oldResources and synced and not in new objs, then delete should be attempted - // then, all objs should be applied by create or update - // at the end, we should delete objects that are supposed to be deleted - for _, resource := range resourcesToBeDeleted(oldResources, objs) { - // call the function and get dynamic.ResourceInterface - // getDynamicResourceInterface - logger.Info("resource are being deleted", "kind", resource.Kind, "name", resource.Name, "namespace", resource.Namespace) - - dr, err := GetDynamicResourceInterface(k.Namespace, k.RestConfig, resource.GroupVersionKind()) - if err != nil { - return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err) - } - - if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { - reterr := fmt.Errorf("failed to delete object: %w", err) - logger.Error(reterr, "failed to delete object", "obj", resource.GroupVersionKind(), "namespacedName", resource.NamespacedName()) - - // append resource to status and requeue again to be able to retry deletion - resource.Status = csov1alpha1.ResourceStatusNotSynced - resource.Error = reterr.Error() - newResources = append(newResources, resource) - shouldRequeue = true - } + // make a diff between new objs and oldResources to find out + // a) if an object is in oldResources and synced and not in new objs, then delete should be attempted + // then, all objs should be applied by create or update + // at the end, we should delete objects that are supposed to be deleted + for _, resource := range resourcesToBeDeleted(oldResources, objs) { + // call the function and get dynamic.ResourceInterface + // getDynamicResourceInterface + logger.Info("resource are being deleted", "kind", resource.Kind, "name", resource.Name, "namespace", resource.Namespace) + + dr, err := GetDynamicResourceInterface(k.Namespace, k.RestConfig, resource.GroupVersionKind()) + if err != nil { + return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err) + } + + if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + reterr := fmt.Errorf("failed to delete object: %w", err) + logger.Error(reterr, "failed to delete object", "obj", resource.GroupVersionKind(), "namespacedName", resource.NamespacedName()) + + // append resource to status and requeue again to be able to retry deletion + resource.Status = csov1alpha1.ResourceStatusNotSynced + resource.Error = reterr.Error() + newResources = append(newResources, resource) + shouldRequeue = true } } diff --git a/pkg/kube/mocks/Client.go b/pkg/kube/mocks/Client.go index a4cd02192..29d0a9713 100644 --- a/pkg/kube/mocks/Client.go +++ b/pkg/kube/mocks/Client.go @@ -15,32 +15,32 @@ type Client struct { mock.Mock } -// Apply provides a mock function with given fields: ctx, template, oldResources, shouldDelete -func (_m *Client) Apply(ctx context.Context, template []byte, oldResources []*v1alpha1.Resource, shouldDelete bool) ([]*v1alpha1.Resource, bool, error) { - ret := _m.Called(ctx, template, oldResources, shouldDelete) +// Apply provides a mock function with given fields: ctx, template, oldResources +func (_m *Client) Apply(ctx context.Context, template []byte, oldResources []*v1alpha1.Resource) ([]*v1alpha1.Resource, bool, error) { + ret := _m.Called(ctx, template, oldResources) var r0 []*v1alpha1.Resource var r1 bool var r2 error - if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource, bool) ([]*v1alpha1.Resource, bool, error)); ok { - return rf(ctx, template, oldResources, shouldDelete) + if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource) ([]*v1alpha1.Resource, bool, error)); ok { + return rf(ctx, template, oldResources) } - if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource, bool) []*v1alpha1.Resource); ok { - r0 = rf(ctx, template, oldResources, shouldDelete) + if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource) []*v1alpha1.Resource); ok { + r0 = rf(ctx, template, oldResources) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*v1alpha1.Resource) } } - if rf, ok := ret.Get(1).(func(context.Context, []byte, []*v1alpha1.Resource, bool) bool); ok { - r1 = rf(ctx, template, oldResources, shouldDelete) + if rf, ok := ret.Get(1).(func(context.Context, []byte, []*v1alpha1.Resource) bool); ok { + r1 = rf(ctx, template, oldResources) } else { r1 = ret.Get(1).(bool) } - if rf, ok := ret.Get(2).(func(context.Context, []byte, []*v1alpha1.Resource, bool) error); ok { - r2 = rf(ctx, template, oldResources, shouldDelete) + if rf, ok := ret.Get(2).(func(context.Context, []byte, []*v1alpha1.Resource) error); ok { + r2 = rf(ctx, template, oldResources) } else { r2 = ret.Error(2) }