Skip to content

Conversation

FTS152
Copy link

@FTS152 FTS152 commented Apr 21, 2025

Add new fields to handle custom Samba configurations.

For [global] section, a new field customGlobalConfig is added under smbCommonConfig to handle custom settings. For individual share configurations, modifications can be made in customShareConfig field under smbShare. This design is chosen because, in group mode, a single server may correspond to several grouped shares.

For example, we can set customGlobalConfig in smbCommonConfig like:

apiVersion: samba-operator.samba.org/v1alpha1
kind: SmbCommonConfig
metadata:
  name: samba-service-mypublished
  namespace: samba-operator-system
spec:
  customGlobalConfig:
    configs:
      aio write size: "1"
      strict rename: "Yes"
  network:
    publish: external

During Update() process for configuration, the applyCustomGlobal() function is used to apply the keys and values from smbCommonConfig.spec.customGlobalConfig.configs into smb.conf, which will take effect on the deployed server. However, this method currently has some limitations. When samba server is already running, it is not possible to update smb.conf by modifying smbCommonconfig without interrupting the service. This is mainly due to the following constraints:

  1. Updates to the ConfigMap are not immediately reflected inside the pod’s volume (e.g., /etc/container-config/config.json)
  2. After modifying smbCommonConfig, you also need to import the changed config json to smbd manually (by execute samba-container import in the pod).
  3. The apply function currently only supports adding or modifying existing fields; it does not yet support field deletion.

These limitations are not insurmountable. The ideal solution would be to implement a mechanism that watches for changes to the config.json inside the pod, automatically imports the new configuration into smbd when changes are detected, and we should also improves the logic of the apply function. For now, this commit only provide the ability for modifying samba configurations when deploying a samba server.

Refs #281

@phlogistonjohn
Copy link
Collaborator

Thanks for the PR. FWIW this is what we implemented in ceph for something similar:
https://docs.ceph.com/en/latest/mgr/smb/ (search for custom_smb_global_options and/or custom_smb_share_options). It's similar but since the custom options really can break the automation I require a special key to check / hint that the user read the docs and understands the risks of setting a custom config option.

@FTS152
Copy link
Author

FTS152 commented Apr 22, 2025

OK, I will add a key to check if the user already read docs and know the risks that custom configs may lead to unexpected behaviors. Custom settings are ignored if acknowledgement flags are not set:

apiVersion: samba-operator.samba.org/v1alpha1
kind: SmbCommonConfig
metadata:
  name: samba-service-mypublished
  namespace: samba-operator-system
spec:
  customGlobalConfig:
    useUnsafeCustomConfig: true    <-- add this explicitly
    configs:
      aio write size: "1"
      strict rename: "Yes"
  network:
    publish: external

@phlogistonjohn phlogistonjohn added the enhancement New feature or request label Apr 23, 2025
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

the change generally looks ok to me. I would request that the fix commits get folded/squashed into the original patches to have a nice clean history. If that's ok but you want help doing so just ask.

@FTS152
Copy link
Author

FTS152 commented May 9, 2025

Sure, I'd appreciate it if you could help with the commit squash. Thanks!

@phlogistonjohn
Copy link
Collaborator

First execute git checkout master && git pull --ff-only && git checkout feat_enable_custom_conf && git rebase master to make sure your current branch is up to date with our base branch.

Then run git rebase -i master it will bring up an editor window that looks a bit like:

pick 6634d266f62 foo
pick 761c2c172aa bar
pick 0cdf01debf1 baz
pick 1d28814ffd6 quux

(these are made up ids and commits of course)

Edit this to move the fixup commits after the commit that added the comments with typos. I think in your case it's the first unique commit in the branch. Then change pick to squash for those two. It'll look something like:

pick 6634d266f62 foo
squash 0cdf01debf1 baz
squash 1d28814ffd6 quux
pick 761c2c172aa bar

Save and exit the editor.

When you squash the commits the tool will give you an opportunity to edit the commit message. Do so if you wish.
Look over your changes and if everything looks good do a force push. If you get stuck think you've messed up your commits let me know and I can take more direct measures but I prefer to try and coach you to do it yourself first :-)

@FTS152 FTS152 force-pushed the feat_enable_custom_conf branch 2 times, most recently from 32f7359 to 66ce58d Compare May 12, 2025 08:38
@FTS152
Copy link
Author

FTS152 commented May 12, 2025

Thanks for the clear and detailed explanation! Well...I think I've messed up something at the beginning (I included a commit in master branch) so I force-pushed twice. Please let me know if there are still any other problems.

@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

Frank Hsiao and others added 2 commits May 12, 2025 15:35
Add new fields to handle custom Samba configurations.
For [global] section, a new field customGlobalConfig is added
under smbCommonConfig to handle custom settings. For individual
share configurations, modifications can be made in customShareConfig
field under smbShare. This design is chosen because, in group mode,
a single server may correspond to several grouped shares.

Signed-off-by: FTS152 <[email protected]>
1. Add a key to check if the user already read docs and know the
risks that custom configs may lead to unexpected behaviors.
Custom settings are ignored if acknowledgement flags are not set
if useUnsafeCustomConfig not set to true explicitly.
2. fix comment typoes

Co-authored-by: Spencer B. <[email protected]>
Signed-off-by: FTS152 <[email protected]>
Copy link

mergify bot commented May 12, 2025

rebase

✅ Branch has been successfully rebased

@phlogistonjohn phlogistonjohn force-pushed the feat_enable_custom_conf branch from 66ce58d to b7d86f3 Compare May 12, 2025 15:35
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for working on this.

@phlogistonjohn
Copy link
Collaborator

2nd review requested: @obnoxxx @anoopcs9 @synarete

@anoopcs9
Copy link
Collaborator

/test centos-ci/sink-clustered/mini-k8s-latest

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@mergify mergify bot merged commit d810430 into samba-in-kubernetes:master May 13, 2025
13 checks passed
@mergify mergify bot added the priority-review This PR deserves a look label May 28, 2025
@HasseJohansen
Copy link

HasseJohansen commented Jun 29, 2025

@anoopcs9. Thanks for getting this feature into master. Would it be possible to cut release with this functionality? I think it is important for many usecases (my usecase is that newer versions of IOS needs to have vfs object = streams_xattr set or else shares are read only from IOS

@anoopcs9
Copy link
Collaborator

@anoopcs9. Thanks for getting this feature into master. Would it be possible to cut release with this functionality?

We are already due for a release. I hope we can get it out sooner rather than later. @phlogistonjohn any further comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-review This PR deserves a look
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants