Skip to content

confd: Add mount constraint for container config #1046

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

axkar
Copy link
Collaborator

@axkar axkar commented May 15, 2025

This PR adds a validation rule to prevent incomplete container mount configurations. Ensures that either "source" or "content" is present when "target" is specified, avoiding runtime errors caused by undefined mount sources.

Fixes #1040

Checklist

Tick relevant boxes, this PR is-a or has-a:

  • Bugfix
    • Regression tests
    • ChangeLog updates (for next release)
    • YANG model change => revision updated?
  • Feature
    • YANG model change => revision updated?
    • Regression tests added?
    • ChangeLog updates (for next release)
    • Documentation added?
  • Test changes
    • Checked in changed Readme.adoc (make test-spec)
    • Added new test to group Readme.adoc and yaml file
  • Code style update (formatting, renaming)
  • Refactoring (please detail in commit messages)
  • Build related changes
  • Documentation content changes
    • ChangeLog updated (for major changes)
  • Other (please describe):

@axkar axkar self-assigned this May 15, 2025
@axkar axkar force-pushed the resctrict-mount-in-cont branch from 59def71 to cceb5c8 Compare May 15, 2025 13:58
@axkar axkar marked this pull request as ready for review May 15, 2025 13:59
@axkar axkar requested a review from troglobit May 15, 2025 13:59
@wkz
Copy link
Contributor

wkz commented May 15, 2025

Did you verify that adding mandatory true; to the data and content leafs did not solve this issue without an explicit must expression?

@troglobit
Copy link
Contributor

Did you verify that adding mandatory true; to the data and content leafs did not solve this issue without an explicit must expression?

My question too.

@axkar
Copy link
Collaborator Author

axkar commented May 16, 2025

I have just checked.
Unfortunately it doesn't seem to work to have it this way:

choice data {
          case source {
            leaf source {
              mandatory true;
              description "...";
              type string {
                pattern '/.*';
              }
            }
          }
          case content {
            leaf content {
              mandatory true;
              description "...";
              type binary;
            }
          }
        }

This looked like the ideal solution, but it seems that libyang might not be handling this correctly.
Possibly a bug or a limitation in how it treats choice nodes with mandatory leaves.

@axkar
Copy link
Collaborator Author

axkar commented May 16, 2025

It actually works by placing mandatory true; at the top level of choice data.

choice data {
          mandatory true;
          case source {
            leaf source {
              description "...";
              type string {
                pattern '/.*';
              }
            }
          }
          case content {
            leaf content {
              description "...";
              type binary;
            }
          }
        }

Error messages are not perfect though for CLI users who do not interact with YANG models:

admin@test-00-01-00:/config/> leave
Error: Mandatory choice "data" data do not exist. (path "/infix-containers:containers/container[name='xyz']/mount[name='abc']")
Error: Invalid candidate configuration
admin@test-00-01-00:/config/>

There is no mention of source or content.
However, this seems to be the way forward. It is minimal and correct :)

@axkar axkar force-pushed the resctrict-mount-in-cont branch 4 times, most recently from 79a3832 to 34456e9 Compare May 19, 2025 06:48
axkar added 2 commits May 19, 2025 09:35
  This change ensures configuration correctness by enforcing that each
  container mount has either a source or content set. Without this, the
  system may generate invalid runtime arguments (src=(null)), leading
  to container startup failures.
  If an invalid mount source path is specified, the container logs a
  "file not found" error and subsequently crashes.

  Adding a check inside the SR_EV_CHANGE event handler performs an
  early validation of the mount source path, allowing to reject
  configurations that reference non-existent or unreadable files before
  they are committed to the datastore.
@axkar axkar force-pushed the resctrict-mount-in-cont branch from d9c49ff to dfc350a Compare May 19, 2025 07:35
@mattiaswal
Copy link
Contributor

mattiaswal commented May 19, 2025

It actually works by placing mandatory true; at the top level of choice data.

choice data {
          mandatory true;
          case source {
            leaf source {
              description "...";
              type string {
                pattern '/.*';
              }
            }
          }
          case content {
            leaf content {
              description "...";
              type binary;
            }
          }
        }

Error messages are not perfect though for CLI users who do not interact with YANG models:

admin@test-00-01-00:/config/> leave
Error: Mandatory choice "data" data do not exist. (path "/infix-containers:containers/container[name='xyz']/mount[name='abc']")
Error: Invalid candidate configuration
admin@test-00-01-00:/config/>

There is no mention of source or content. However, this seems to be the way forward. It is minimal and correct :)

data is a very general name, i really think the (choice) node should be renamed to source to more indicate what it is. But this is a separate issue IMHO.

But for now, the user may need to look at the model to know whats happening.

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.

Container: mount allows target without source or content
4 participants