Add setting to prevent burrito from destroying resources

[LucasMrqes]

We should add a setting to prevent burrito from running apply operations when the number of resources to destroy is > 0 in the plan output. This would facilitate the adoption of Burrito on critical terraform code and is a good way to prevent unwanted disasters while having drift detection.

[florianspk]

Hi! 👋
I'd be happy to take on this issue. I think it's a great safety feature to add, especially for critical Terraform codebases. Let me know if there are any implementation details or constraints you'd like me to follow before

[LucasMrqes]

Hi @florianspk !
Awesome, thank you for taking this on! Here's what I have in mind:

Introduce a new field: spec.remediationStrategy.nonDestructiveApply in both the TerraformLayer and TerraformRepository CRDs.

It should follow the exact same behavior as spec.remediationStrategy.autoApply: the value defined at the layer level overrides the repository-level value, and the default should be false when unset.

In the TerraformLayer controller’s state machine (internal/controllers/terraformlayer/states.go), update the ApplyNeeded state handler to prevent the TerraformRun object from being created if nonDestructiveApply is enabled and the layer’s status.lastResult contains a destructive action. (check this with a clever regex?)

So overall, it’s essentially the same implementation pattern as autoApply.

---

What’s suggested here is a straightforward change that I think we should go ahead and implement.
That said, I’m noticing that we now have multiple ways to prevent a run from being created (autoApply, sync windows, and now this setting). Maybe it would make sense to introduce a dedicated "ApplyBlocked" state in the state machine to handle these cases more explicitly. WDYT @corrieriluca

[corrieriluca]

Yes I think an ApplyBlocked state would be more explicit for Burrito users on why a Layer is stuck in ApplyNeeded (events that can be read with kubectl describe layer are only emitted when layer is blocked due to sync windows, in the other cases the information is in the controller logs).

It would require to add new condition(s) in the state machines (e.g. IsAllowedToApply) that check sync windows, auto apply setting, non-destructive apply setting, etc.

And when a Layer is in the ApplyBlocked state it would just wait for drift detection (the same as the Idle state).

However this is a more complex implementation for the non-destructive apply setting, it will require a lot of tests. If we want a quick win, the straightforward implementation mentioned above is a good candidate 😉

[florianspk]

I'll start working on the integration of the new nonDestructiveApply field with the proposed behavior (similar to autoApply).

The second proposal regarding a dedicated ApplyBlocked state is definitely relevant, especially to improve clarity for users. I think it would be best to handle that in a separate issue.