-
-
Notifications
You must be signed in to change notification settings - Fork 772
feat: add opaque mount options support for existing volumes #12585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add opaque mount options support for existing volumes #12585
Conversation
| MountReadOnly *bool `yaml:"readOnly,omitempty"` | ||
| // description: | | ||
| // Additional mount options to pass to the mount syscall. | ||
| // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve kept this mount option flexible to allow both boolean flags and key–value pairs (where key–value pairs are expressed as key=value), representing key–value pairs as plain strings feels a bit off—please let me know if there’s a better way to model this.
This will look like
mount:
readOnly: false
options:
- noatime # boolean flag
- nodiratime # boolean flag
- nosuid # boolean flag
- nodev # boolean flag
- uid=1000 # key=value
- gid=1000 # key=value
- size=100m # filesystem-specific key=value (e.g., tmpfs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had an internal discussion about this, and the consensus was that we are not happy with a free-form field.
We were considering the following (probably separate) fields, we don't have good names for them, but:
DisableAccessTime(defaults to false) -> triggers properno*atimebased on filesystem typeSecure(defaults to true, breaking change) -> triggersnosuid,nodev(if the filesystems supports it), and it should be explicitly set tofalse(breaking change for 1.13, but in the direction of better security)uid/gidmight be interesting, but if we do that, we need to do this as separate fieldssizedoesn't apply (yet), as there is no support fortmpfs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed mountOptions and used separate fields : DisableAccessTime and Secure.
- For
uid/gidwe should only apply them for filesystems that support those mount options
(examples:vfat, fat, ntfs, cifs, nfs, tmpfs, virtiofs) some of these might not be supported yet by talos but no harm while using validation. We cannot validate this at config time because the filesystem is discovered at runtime, the mount controller can check the filesystem type for this, I can add that logic if this is alright.
Also, there are several other mount options available, let me know if you want additional typed fields.
|
|
||
| | Field | Type | Description | Value(s) | | ||
| |-------|------|-------------|----------| | ||
| |`readOnly` |bool |Mount the volume read-only. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs page currently does not show mount options in the config here. I’m guessing this is because it’s an optional parameter, so I haven’t included those extra options here.
aa0af65 to
40d3df8
Compare
website/content/v1.13/reference/configuration/block/existingvolumeconfig.md
Outdated
Show resolved
Hide resolved
40d3df8 to
e7a7da7
Compare
|
I think we need E2E test for those options as well before we can merge it. Edit: |
2773555 to
2fb7b50
Compare
|
@shanduur , Added E2E test for both |
|
The tests are failing with: |
| // Build mount parameters based on configuration | ||
| var params []block.ParameterSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are FS_CONFIG parameters, you want Mount Parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give another try for integration test on github actions, I've updated mount.go to use mount params,
603f88f to
9d128f2
Compare
Add DisableAccessTime and Secure mount options for existing volumes. DisableAccessTime adds noatime parameter to disable access time updates. Secure adds nosuid and nodev parameters for security (defaults to true). Add integration tests for both options. Signed-off-by: Pranav Patil <[email protected]>
9d128f2 to
0c8ab63
Compare
| // Build mount parameters based on configuration | ||
| var params []block.ParameterSpec | ||
|
|
||
| if existingVolumeConfig.Mount().DisableAccessTime() { | ||
| params = append(params, block.NewBooleanParameter("noatime")) | ||
| } | ||
|
|
||
| // Add secure mount options (nosuid, nodev) by default | ||
| if existingVolumeConfig.Mount().Secure() { | ||
| params = append(params, block.NewBooleanParameter("nosuid")) | ||
| params = append(params, block.NewBooleanParameter("nodev")) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've did some more reading and this is not a correct way to propagate the parameters. You should follow the same path as ReadOnly is doing, so the values are properly read and if needed, the volume is remounted. The params are read and set only once on the disk provisioning, as they are referring to the filesystem, and can't be updated - you need to reprovision the Volume to do this.
Also, this should be captured in the VolumeMountRequest:
talos/internal/app/machined/pkg/controllers/block/internal/volumes/volumeconfig/user_volumes.go
Lines 339 to 347 in 4b274f7
| // HandleExistingVolumeMountRequest returns a MountTransformFunc for existing volumes. | |
| // It sets `VolumeMountRequestSpec.ReadOnly` based on the existing configuration. | |
| func HandleExistingVolumeMountRequest(existingVolumeConfig configconfig.ExistingVolumeConfig) func(m *block.VolumeMountRequest) error { | |
| return func(m *block.VolumeMountRequest) error { | |
| m.TypedSpec().ReadOnly = existingVolumeConfig.Mount().ReadOnly() | |
| return nil | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip @shanduur ,
After reading I've updated all 3 bits for mounts (mounts, mount_request and mount_status)
Add DisableAccessTime and Secure mount options for existing volumes. DisableAccessTime adds noatime parameter to disable access time updates. Secure adds nosuid and nodev parameters for security (defaults to true). Add integration tests for both options. Signed-off-by: Pranav Patil <[email protected]>
| if err := p.moveMount(p.target); err != nil { | ||
| return fmt.Errorf("error mounting %q to %q: %w", p.Source(), p.target, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried setting mount attributes directly using mount_setattr, but this failed with EINVAL when attempting to apply MOUNT_ATTR_NOATIME | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NODEV on mounts created via the new mount API.
Example error:
ERROR controller failed {"component":"controller-runtime","controller":"block.MountController","error":"failed to mount \"e-test-data\": failed to mount: 1 error(s) occurred:
error setting mount attributes on \"/var/mnt/test-data\": setattr failed for fd=70 target=\"/var/mnt/test-data\" flags=36864 attr=unix.MountAttr{Attr_set:0x16, Attr_clr:0x0, Propagation:0x0, Userns_fd:0x0}: invalid argument"}
I tested multiple variations:
- Calling mount_setattr both before and after move_mount
- Different flag combinations
- With and without AT_RECURSIVE
In all cases, setting these attributes via mount_setattr failed with EINVAL.
Using MS_REMOUNT works reliably for these flags, while mount_setattr does work correctly for MOUNT_ATTR_RDONLY.
|
Dropping some logs which I tried while testing with the changes: For Mount options: disableAccessTime & secure enabled |
This PR closes #12308
What? (description)
Why? (reasoning)
Acceptance
Please use the following checklist:
make conformance)make fmt)make lint)make docs)make unit-tests)