Skip to content

Add mode, uid, and gid options for mount plugs#461

Merged
jonathan-conder merged 6 commits into
mainfrom
feature/configure-mount-plugs
Aug 27, 2025
Merged

Add mode, uid, and gid options for mount plugs#461
jonathan-conder merged 6 commits into
mainfrom
feature/configure-mount-plugs

Conversation

@jonathan-conder
Copy link
Copy Markdown
Contributor

@jonathan-conder jonathan-conder commented Aug 20, 2025

Description

Ties in with canonical/sdkcraft#170. The main issue addressed is for mount plugs like ~/go/pkg/mod - previously the intermediate directories (go and pkg) were owned by root:root. Now they will be owned by workshop:workshop. This is configurable using the interface attributes uid, gid and mode.

The mount point itself should also be affected, but LXD has some issues (canonical/lxd#12648 and canonical/lxd#16264) which prevent this for now. Not a big deal since the mount point is only visible when the interface is disconnected.

Also fixes a bug where workshop-source: $SDK requires a trailing slash (which I thought I'd fixed already but I must have overlooked something).

Fixes a recurring issue with interface attributes being different types in workshop and SDK definitions. This is potentially a breaking change (if a launch or refresh is in progress). Removes the need to work around this in the tests.

Makes .Xauthority and workshopctl mounts read-only, because why not?

Adds some common directories to the cloud-config:

  • ~/.cache, ~/.config and ~/.local so SDKs don't need to worry about 0700 permissions
  • ~/bin and ~/.local/bin so SDKs don't need to source .profile after installing something there

Fixes several correctness issues with sftp. I tried to reuse as much functionality as possible, but after fixing a few things the remaining sftpfs functions were just directly calling sftp, so I dropped sftpfs. Summary of issues:

  • Mkdir and OpenFile aren't transactional, they ignore the umask,
    and don't return ErrExist (required for afero.TempDir and
    afero.TempFile). The rewrites are transactional, but not atomic.
    They respect a hardcoded umask of 0022, which is the Ubuntu default
    for root.
  • Paths are stripped from certain error types. The wrappers add these back in.
  • On error, functions can return non-nil file handles
  • Sync and WriteAt aren't implemented at all. Fixed by removing from the
    interface.
  • Readdir and Readdirnames don't use the open file handle. Fixed by adding
    ReadDir to the Fs interface.

After looking at the sftp package, I think SFTP is ultimately the right protocol
for us, but both the client and server side need some work to implement the os
filesystem functions correctly. Some custom extensions might be needed for
something like os.Root.

Self-review quick check

  • Make decisions that cost a lot to reverse explicit in the PR description.
  • Avoid nested conditions.
  • Delete dead code and redundant comments.
  • Normalise symmetries by sticking to doing identical things identically.
// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...
  • Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
  • Put variable declaration and initialisation together.
  • Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
  • Put a blank line between two logically different chunks of code.
  • Follow the style guide for new error messages.

Docs

  • I have checked and added or updated relevant documentation.
  • I have checked and added or updated relevant release notes.
  • I have included the technical author in the review.

Or:

  • I confirm the PR has no implications for documentation.

@jonathan-conder jonathan-conder self-assigned this Aug 20, 2025
@jonathan-conder jonathan-conder changed the title Add mkdir, mode, uid, and gid options for mount plugs Add mode, uid, and gid options for mount plugs Aug 21, 2025
Since the state is stored as JSON, weakly typed plug attributes can be
converted from int64 to float64 when the file is attached to a task.
Adding another layer of marshalling works around this.

Attribute types can be important, for example plug binding uses
reflect.DeepEqual to check if two plugs can be bound. Previously plugs
defined in SDKs could look different from plugs defined in a workshop.

A previous workaround, "Fix type mismatch when comparing workshop
definitions," is no longer needed.

This reverts commit 92260d7.
@jonathan-conder jonathan-conder force-pushed the feature/configure-mount-plugs branch from 8ad60ec to 7add6ea Compare August 21, 2025 06:29
@jonathan-conder jonathan-conder force-pushed the feature/configure-mount-plugs branch 2 times, most recently from 0097286 to 702cb16 Compare August 24, 2025 23:08
@jonathan-conder jonathan-conder marked this pull request as ready for review August 24, 2025 23:41
@jonathan-conder jonathan-conder requested review from akcano and dmitry-lyfar and removed request for akcano August 24, 2025 23:42
@jonathan-conder jonathan-conder mentioned this pull request Aug 25, 2025
13 tasks
Copy link
Copy Markdown
Collaborator

@dmitry-lyfar dmitry-lyfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed everything except fsutil which I'm going to review separately now. There was just one comment so far.

Also, smoke-tested it on both types of mounts, works as expected.

Comment thread internal/workshop/lxd/lxd_backend.go
@jonathan-conder jonathan-conder force-pushed the feature/configure-mount-plugs branch from 8d24252 to 4ab7732 Compare August 26, 2025 02:44
@dmitry-lyfar dmitry-lyfar requested a review from Copilot August 26, 2025 06:00
Comment thread internal/fsutil/sftp.go
Comment thread internal/fsutil/basepath.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds mount interface attributes mode, uid, and gid for configuring directory ownership and permissions in mount plugs. The main purpose is to address ownership issues where intermediate directories were previously owned by root:root, making them inaccessible to the workshop user. Now mount plugs can specify proper ownership via these new attributes.

Key changes include:

  • Added mode, uid, and gid attributes to mount interface plugs for directory creation permissions
  • Replaced the custom workshop.WorkshopFs interface with a new fsutil.Fs abstraction for better filesystem operations
  • Made .Xauthority and workshopctl mounts read-only for security

Reviewed Changes

Copilot reviewed 51 out of 52 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/interfaces/builtin/mount.go Adds new mode, uid, gid attributes with validation and fallback logic for mount plugs
internal/fsutil/fs.go New filesystem abstraction with common operations like MkdirAll, AtomicWriteTo, temp file creation
internal/fsutil/sftp.go SFTP filesystem implementation with proper error handling and umask support
internal/workshop/device.go Adds Mode, Owner, Group fields to Mount struct and reorders fields
internal/interfaces/lxd_device/backend.go Enhanced mount preparation with proper directory ownership/permissions handling
docs/reference/sdks.rst Documents new mount plug attributes for permissions and ownership

Comment thread internal/workshop/device.go
Comment thread internal/interfaces/builtin/mount.go Outdated
Comment thread internal/fsutil/sftp.go Outdated
Comment thread internal/interfaces/lxd_device/backend.go
Comment thread internal/interfaces/builtin/mount.go Outdated
Comment thread internal/interfaces/builtin/mount.go Outdated
Comment thread docs/explanation/interfaces/mount-interface.rst Outdated
Comment thread docs/reference/definition-files/workshop-definition.rst Outdated
Comment thread docs/reference/definition-files/workshop-definition.rst Outdated
@jonathan-conder jonathan-conder force-pushed the feature/configure-mount-plugs branch 2 times, most recently from fb71109 to 5ecca31 Compare August 27, 2025 01:15
jonathan-conder and others added 2 commits August 28, 2025 09:42
The new sftpFs works around a number of issues with afero/sftpfs:
- Mkdir and OpenFile aren't transactional, they ignore the umask,
  and don't return ErrExist (required for afero.TempDir and
  afero.TempFile).
- Paths are stripped from certain error types
- On error, functions can return non-nil file handles
- Sync and WriteAt aren't implemented at all
- Readdir and Readdirnames don't use the open file handle

The Fs wrapper also brings MkdirAll, MkdirTemp, CreateTemp, WriteFile
and ReadFile in line with the standard library implementations.

It turns out the only functions that sftpfs implements correctly are
those which directly call the corresponding function on the client, so
there's no need for sftpfs any more. But we'll continue using
BasePathFs. All of these have moved to a standalone fsutil package.

Co-authored-by: Dmitry Lyfar <69887876+dmitry-lyfar@users.noreply.github.com>
Co-authored-by: Artem Konev <141050460+akcano@users.noreply.github.com>
@jonathan-conder jonathan-conder force-pushed the feature/configure-mount-plugs branch from 5ecca31 to 9345e20 Compare August 27, 2025 21:43
@github-actions
Copy link
Copy Markdown

TICS Quality Gate

✔️ Passed

workshop

All conditions passed

See the results in the TICS Viewer

The following files have been checked for this project
  • internal/fsutil/basepath.go
  • internal/fsutil/fs.go
  • internal/fsutil/sftp.go
  • internal/interfaces/builtin/desktop.go
  • internal/interfaces/builtin/mount.go
  • internal/interfaces/lxd_device/backend.go
  • internal/interfaces/lxd_device/spec.go
  • internal/overlord/cmdstate/handlers.go
  • internal/overlord/hookstate/handlers.go
  • internal/overlord/ifacestate/ifacemgr.go
  • internal/overlord/sdkstate/handlers.go
  • internal/overlord/workshopstate/handlers.go
  • internal/overlord/workshopstate/request.go
  • internal/testutil/dircontentchecker.go
  • internal/workshop/backend.go
  • internal/workshop/device.go
  • internal/workshop/fakebackend/backend.go
  • internal/workshop/lxd/lxd_backend_profile.go
  • internal/workshop/lxd/lxd_backend_project.go
  • internal/workshop/lxd/lxd_backend_volume.go
  • internal/workshop/lxd/lxd_backend.go

Automatic-tests / code-coverage / TICS GitHub Action

@jonathan-conder jonathan-conder merged commit 4377769 into main Aug 27, 2025
13 checks passed
@jonathan-conder jonathan-conder deleted the feature/configure-mount-plugs branch August 27, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants