Centralize base path variable

[spotlesstofu]

During #776 I found the hardcoded /opt/confidential-containers paths to come in the way for running Trustee as a non-root user. Changing all the paths at runtime means traversing the whole KbsConfig struct, not optimal.

Here I propose a possible alternative, to set the default base path at runtime with a shared logic.

This approach can look heavy, in that it introduces a new package just to share a function. Over time the shared package can become more useful to share more project-wide config. For example if we decide to split the api server from the kbs package. I'd be willing to populate the shared config package a bit more but right now I don't know what else could be moved.

Please propose alternative approaches.

If we like this, I'll have to fix the tests, and possibly make the tests run in both the root and non-root scenario.

[fitzthum]

Nice. We definitely have a problem with the tests only working as root.

I am on the fence about creating a new module for this. curiuos what @Xynnn007 @mkulke @mythi think about it.

I think you may be able to use the concat! macro to keep the logic in the const at the top of the files. That might be a little more clear than joining the paths deeper in the code (although arguably that is more robust).

[Xynnn007]

I think the core idea of the PR is this part - to have a configurable parent config path.

TBO I don't think we need to introduce a new package for this. Let's re-visit the design:

If the configuration file doesn't specify a configuration directory, we move it to the ~/.trustee directory to ensure permissions are fit, and retain the existing subdirectory structure (we may need to reorganize the code that defines the configuration file path).

If config path(s) are specified, use the new one(s).

[spotlesstofu]

@Xynnn007 You say to change the default const to use ~/.trustee in place of /opt/confidential-containers, and keep everything else as-is. Did I get it?

The only issue I see with this design - backwards compatibility. Current deployments may misbehave if they don't have paths configured explicitly.

[spotlesstofu]

Another way to avoid the new package. Put the logic in the same file as the KbsConfig (kbs/config), and either:

• Child structs import the logic from kbs/config.
• KbsConfig passes the base path to child structs when initializing them.

[Xynnn007]

@Xynnn007 You say to change the default const to use ~/.trustee in place of /opt/confidential-containers, and keep everything else as-is. Did I get it?

The only issue I see with this design - backwards compatibility. Current deployments may misbehave if they don't have paths configured explicitly.

Oh sorry. I did not find that you havd used ~/.trustee at the first glance for the PR. So I am ok with the logic - root to opt and non-root to ~/.trustee, if no permission/no path is configured.

Another way to avoid the new package. Put the logic in the same file as the KbsConfig (kbs/config), and either:

• Child structs import the logic from kbs/config.
• KbsConfig passes the base path to child structs when initializing them.

Agreed.

[mythi]

While we're re-working this, I thought I'll ask if a unified base is still a good fit or should we look into separating things out into data/config/state functions. Similar to what XDG Base Spec is aiming to define.

[Xynnn007]

While we're re-working this, I thought I'll ask if a unified base is still a good fit or should we look into separating things out into data/config/state functions. Similar to what XDG Base Spec is aiming to define.

Now we have configs of path for

1. AS (policies, RVPS storage path)
2. KBS (resource, policies)

and the things are relative simple, so put them under one common parent dir can be easy. Maybe we can consider split data and state thing in future?

[spotlesstofu]

Recap of the options:

1. New package for shared configuration (current approach). Base path is calculated several times, using the same logic - default_base_path().
2. concat! macro. This doesn't work to change the path at runtime. Or does it?
3. Define the shared configuration in the KBS package, along struct KbsConfig. This is the same as the current approach, but saves us from a new package. Introduces dependency from kbs, thus erasing all the advantages of keeping packages separate. This could be an architectural flaw.
4. Constructor function - Config::new(base_path). Remove the impl Default and require to pass the base path when initializing a struct. This is more disruptive of the current workflow but it looks doable.
5. Use the same logic from 1 (current approach). Do not have a common package, but replicate it in each of the four packages that need it.

I'll be pushing option 5 shortly.

[mythi]

Maybe we can consider split data and state thing in future?

Sure.

On the PR itself, I feel /opt/confidential-containers vs home_dir() has limitations but it of course depends on what we want. For example, all container based deployments can have /opt/confidential-containers container path writable by a non-root process very easily.

[spotlesstofu]

@mythi do you advise to have a backwards compatibility mechanism? That is, if /opt/confidential-containers exists and is writable, use it.

[mythi]

@mythi do you advise to have a backwards compatibility mechanism? That is, if /opt/confidential-containers exists and is writable, use it.

Just saying that the assumption that /opt/confidential-containers is only root writable is wrong. Perhaps we could just use that as the default and only fix the the part that it's easy to change.