[DPE-7908] Separation of storage (take 2)

[astrojuanlu]

Issue

(Depends on #256)

I went back and forth over several days about this. There's a series of unfortunate circumstances that make it difficult to find an elegant, simple implementation.

• One cannot execute the backup scripts as snap_daemon. This user has the home directory set to /nonexistent, and any attempt to trigger it (sudo -u, subprocess.Popen(user=), runuser -u) will trigger a snap confinement error.
• As such, a different approach is needed to avoid running the backup scripts as root. Specifically, [MISC] Add wrappers for backup apps charmed-mysql-snap#104. This uses setpriv as recommended in the official Snap documentation and as done for other apps already.
• However, only root can run setpriv, which means that the wrapped scripts must all run as root to drop privileges properly.
• And yet everything else that the wrapped scripts themselves need must be readable by snap_daemon. This includes
  • The CA file, and
  • Any temporary directories
• Initially I thought that creating the CA file as snap_daemon and let everything else use user and group would suffice. But this was wrong. The backup methods require creating some temporary directories in writable locations.
• As such, everything must run as snap_daemon, except the scripts themselves, which must be run as root.
• Similarly to my initial attempt, these were the options I came up with:
  1. Copy-paste all the logic in the machines subclass, tweaking what I need. Felt excessive for changing the argument of 1 function.
  2. Introduce some extra parameter in the lib methods that must be passed around for the child class to detect when to force root. Seemed similar to what I initially tried in [MISC] Remove permissions hack from backups using wrapped apps (VM) #247.
  3. Special-case the wrapped scripts in _execute_commands in the child subclass. Ugly, but effective. This is what's implemented here.
  4. Refactor the backup methods to introduce private methods that would run the inner script which could be overridden by the child classes. Something like

    def execute_backup_commands(self, ...):
        # pre-backup checks
        self._do_execute_backup_commands(self, ...)  # Overridden by the child classes

However, I think this might introduce a lot of indirection, some duplicity, and little benefit.

---

Ultimately, the tension comes because the user parameter clashes with the need to execute the wrappers as root. The solution is clear, but the architecture of the code makes it hard to actually implement.

Solution

Checklist

• I have added or updated any relevant documentation.
• I have cleaned any remaining cloud resources from my accounts.

[paulomach]

Thanks for the context. I had happily forgot all these details.

[sinclert-canonical]

I reviewed commits 52e4a2c, 8e50a82 and 5dc6357, which I assumed are the ones built on top of PR #256.

They are exactly the same as the ones in the previously approved separation of storage PR, aside from the encapsulation of the required root user switch in a single conditional (which I find highly convenient).

Fantastic investigation 💯

[paulomach]

Left some questions - otherwise is good given the constraints

[astrojuanlu]

Some stuff failing on K8s, looking into it.

[astrojuanlu]

After closer inspection, I figured that the failing tests are probably not related to this PR. So I'm merging as-is.

[astrojuanlu]

Proposal for self-healing test that was failing here #264

[astrojuanlu]

There are some test failures from the usual suspects. I'm going ahead but I'll keep an eye on these tests for the coming days.