terraform-controller icon indicating copy to clipboard operation
terraform-controller copied to clipboard

If a backend is specified, an error in the `preCheck` during deletion will result in the backend becoming null

Open wujiuye opened this issue 2 years ago • 4 comments

Due to the fact that the errors returned by preCheck during deletion are ignored, the processing logic for deletion is executed directly.

// pre-check Configuration
if err := r.preCheck(ctx, &configuration, meta); err != nil && !isDeleting {
return ctrl.Result{}, err
}

The initialization of the backend occurs in the preCheck method. If an error occurs in preCheck before calling the RenderConfiguration method to initialize the backend, the backend will not be initialized, resulting in the deletion processing logic using the default backend.

During the CheckProvider step, calling the meta.getCredentials method triggers the tfcfg.SetRegion method. If the configuration.Spec.Region is not specified, the tfcfg.SetRegion method will update the configuration. At this point, an error may occur with the message "the object has been modified."

I don't understand the code either, and it's unclear why configuration.Spec.Region is being updated here, as there are no other references to it elsewhere.

wujiuye avatar Aug 17 '23 10:08 wujiuye

Is there a plan to fix this bug?

wujiuye avatar Aug 31 '23 08:08 wujiuye

The reason of "the object has been modified" may be k8sClient got outdated Configuration in controller's cache. That is to say, someone update the object but somehow the controller didn't watch the event to update the cache.

I think we can refine the errors in preCheck. When chekcking preCheck errors, use a function say IsErrBlockDeletion to examine the error.

	if err := r.preCheck(ctx, &configuration, meta); err != nil {
		if !isDeleting {
			return ctrl.Result{}, err
		}
		// deleting
		if IsErrBlockDeletion(err) { 
			return ctrl.Result{}, err
		}
	}

chivalryq avatar Aug 31 '23 09:08 chivalryq

The reason of "the object has been modified" may be k8sClient got outdated Configuration in controller's cache. That is to say, someone update the object but somehow the controller didn't watch the event to update the cache.

I think we can refine the errors in preCheck. When chekcking preCheck errors, use a function say IsErrBlockDeletion to examine the error.

	if err := r.preCheck(ctx, &configuration, meta); err != nil {
		if !isDeleting {
			return ctrl.Result{}, err
		}
		// deleting
		if IsErrBlockDeletion(err) { 
			return ctrl.Result{}, err
		}
	}

After IsErrBlockDeletion, what should we do next?

Should we retry by putting it back in the queue? return ctrl.Result{Requeue: true}, nil

wujiuye avatar Aug 31 '23 12:08 wujiuye

After IsErrBlockDeletion, what should we do next?

Should we retry by putting it back in the queue? return ctrl.Result{Requeue: true}, nil

If return err is not nil, object will be reequeued automatically.

chivalryq avatar Sep 01 '23 08:09 chivalryq