Skip to content
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

confd: ensure admin users have full access to vtysh #946

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

troglobit
Copy link
Contributor

Description

Future Infix defconfigs for smaller system may exclude Frr and set all routes directly in the kernel. So failing to add a user to the frrvty group should not cause an error, hence the new group_exists() function.

Note, the new *_groups() functions could also be used for guest and operator level users. We've talked opening up guest access to sysrepo, relying fully on NACM, in which case we could create a new group sysrepo for /dev/shm/* files.

Checklist

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

  • Bugfix
    • Regression tests
    • ChangeLog updates (for next release)
  • 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): convenience

Future Infix defconfigs for smaller system may exclude Frr and set all
routes directly in the kernel.  So failing to add a user to the frrvty
group should not cause an error, hence the new group_exists().

Signed-off-by: Joachim Wiberg <[email protected]>
@troglobit troglobit requested review from wkz and mattiaswal February 16, 2025 11:58
Copy link
Contributor

@wkz wkz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! 🥇

@@ -45,6 +45,12 @@ static char *os = NULL;
static char *nm = NULL;
static char *id = NULL;

static const char *admin_groups[] = {
"wheel",
"frrvty",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"frrvty",
#ifdef HAVE_FRR
"frrvty",
#endif

As this list grows, having this type of structure might make it easier to keep track of which deps pull in which groups. It would also avoid the need for group_exists(). Just a thought.

Now that I think about it: Is there a podman group that we should add for builds with container support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add that to my commit 😀👍

Not that I've seen, but I'll have a look again at podman. I'm still at it with the other related issues anyway.

Copy link
Contributor

@mattiaswal mattiaswal left a comment

Choose a reason for hiding this comment

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

Great 👌

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.

3 participants