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

honor pkg_svc_user/pkg_svc_group #1012

Closed
wants to merge 1 commit into from
Closed

Conversation

bookshelfdave
Copy link
Contributor

@bookshelfdave bookshelfdave commented Jun 29, 2016

Plans that specify pkg_svc_user and pkg_svc_group will have SVC_USER and
SVC_GROUP artifact metafiles created during build. The supervisor will
attempt to run the child service process as the given user/group.

  • if SVC_USER and SVC_GROUP metafiles exist, the supervisor will verify the user
    and group specified within exists. If we are running as the specified
    user:group, the process will start without changing user:group. If we
    aren't running as the specified user:group, but are running as root,
    we exec the child process as SVC_USER:SVC_GROUP. If a pkg_svc_user and
    pkg_svc_group aren't specified, try running the service as hab:hab if
    that user:group exists, otherwise, fall back to the current uid:gid.
  • we no longer symlink to hooks, they are copied to /hab/svc/foo/*. This
    is so we can chmod the run script as the user/group running the service.
  • friendlier chmod/chown/mkdir errors that bubble up to the CLI
  • Tracing added for chmod/chown/mkdir functions
  • a RuntimeConfig struct is passed into Supervisor that specifies
    additional params used during child process exec.
  • Supervisor provides a platform-dependent start_platform() function
    to allow for future expansion.
  • hooks are executed as SVC_USER:SVC_GROUP if they are specified, otherwise, they fall back to hab:hab, current_uid:current_gid, and finally panic.
  • Platform dependent users module, currently supports Linux (osx
    untested but might just work)
  • postgres plan updated to run as hab:hab. initdb (in the init hook) requires a non-root user to run.
  • permissions are tricky, please kick the tires with this branch on a few packages before considering a merge for this PR.

Staring without user dparfitt, which is specified in my local test core/redis package:

root@a4b6b938bc68:/src/components/sup# ./target/debug/hab-sup start core/redis
hab-sup(MN): Starting core/redis
hab-sup(USERS)[src/util/users.rs:41:27]: Package requires user dparfitt to exist, but it doesn't

Staring with user dparfitt, which is specified in my local test core/redis package:

root@a4b6b938bc68:/src/components/sup# ./target/debug/hab-sup start core/redis
hab-sup(MN): Starting core/redis
hab-sup(TP): Child process will run as user=dparfitt, group=dparfitt

Staring w/ dparfitt:dparfitt, but without write access to /hab/svc/redis:

hab-sup(PK): Can't create directory /hab/svc/redis
hab-sup(PK): If this service is running as non-root, you'll need to create /hab/svc/redis and give the current user write access to it
hab-sup(PK)[src/package/mod.rs:154:16]: Can't create "/hab/svc/redis", Permission denied (os error 13)

/hab/svc/ file permissions for a running service that uses hab:hab:

root@3e8333673d2f:/src/components/sup# ls -latr /hab/svc/postgresql/
total 84
drwxr-xr-x  4 root root  4096 Jun 29 21:17 ..
drwx------  2 hab  hab   4096 Jun 29 21:17 files
drwx------  2 root root  4096 Jun 29 21:17 toml
-rw-r--r--  1 root root 46036 Jun 29 21:17 config.toml
-rwxr-xr-x  1 root root   195 Jun 29 21:17 run
drwxr-xr-x  2 root root  4096 Jun 29 21:17 hooks
drwx------  3 hab  hab   4096 Jun 29 21:17 config
drwx------  3 hab  hab   4096 Jun 29 21:17 var
drwx------ 19 hab  hab   4096 Jun 29 21:17 data

TODO:

  • plans with chpst will fail if they don't specify pkg_svc_user/pkg_svc_group as root
    • postgresql updated
    • ruby-rails-sample won't build, needs chpst removed
  • should hab-plan-build ensure that pkg_svc_user/pkg_svc_group must be specified together or not at all?
    • hab-plan-build defaults them to hab:hab
  • Test hab file upload and check permissions

Plans that specify pkg_svc_user and pkg_svc_group will have SVC_USER and
SVC_GROUP artifact metafiles created during build. The supervisor will
attempt to run the child service process as the given user/group.

additionally:
- if SVC_USER and SVC_GROUP exist, the supervisor will verify the user
  and group specified within exists. If we are running as the specified
  user:group, the process will start without changing user:group. If we
  aren't running as the specified user:group, but are running as root,
  we exec the child process as SVC_USER:SVC_GROUP. If a pkg_svc_user and
  pkg_svc_group aren't specified, try running the service as hab:hab if
  that user:group exists, otherwise, fall back to the current uid:gid.
- we no longer symlink to hooks, they are copied to /hab/svc/foo/*. This
  is so we can chmod the run script as the user/group running the service.
- friendlier chmod/chown/mkdir errors that bubble up to the CLI
- Tracing added for chmod/chown/mkdir functions
- a RuntimeConfig struct is passed into Supervisor that specifies
  additional params used during child process exec.
- Supervisor provides a platform-dependent start_platform() function
  to allow for future expansion.
- hooks are executed as SVC_USER:SVC_GROUP
- Platform dependent users module, currently support Linux (osx
untested but might just work)

Signed-off-by: Dave Parfitt <[email protected]>
@bookshelfdave bookshelfdave changed the title [WIP] honor pkg_svc_user/pkg_svc_group honor pkg_svc_user/pkg_svc_group Jun 29, 2016
use error::{Result, Error};
use hcore::package::PackageInstall;

static LOGKEY: &'static str = "USERS";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep with a two-letter convention for LOGKEY, or expand it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bumped it up because I can't ever tell what the two-letter key is. I don't mind either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are those keys documented anywhere? I've never known what any of them mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure we just make them up as we go along. This could be a useful question for @ryankeairns as to whether this output format is even helpful to people

Copy link
Contributor

Choose a reason for hiding this comment

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

By reading the original comment, I presume the LOGKEY in the output is hab-sup(XX): ... if so, I too have always wondered and never understood it. As Nathan asked, is this documented? The definitions for the abbreviations ought to be readily available but even so, you're likely still getting that info after the fact (if you can locate it).

What would the extended version look like? e.g. what does hab-sup(PK): translate/expand to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 👍 👍 👍 on changing the LOGKEYs and/or behavior, but I feel like this should be discussed in a separate Github issue as to not distract the original intent of the PR.

🇺🇸 🇺🇸 🇺🇸 🇺🇸

Copy link
Contributor

Choose a reason for hiding this comment

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

@metadave created #1036 to track this future change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The purpose of the two character log key isn't for informing users or about their readability, but developer access and debugging. We are supposed to be using these to identify which module a log even is coming from without needing to specify the full path plus crate name of the module in the log output. This is much like the way we log error messages in the servers with a very similar format.

I would stick with two or three character log prefixes, expanding them is going to clutter the log and I don't think it makes things any easier to track down.

@thesentinels
Copy link
Contributor

☔ The latest upstream changes (presumably #1024) made this pull request unmergeable. Please resolve the merge conflicts.

@smith
Copy link
Contributor

smith commented Jul 6, 2016

Does this make {{pkg.svc_user}} and {{pkg_svc_group}} available in templates? It would be cool if it did, because it would be useful in things like init hooks where files get created and need to be owned by the service user.

@bookshelfdave
Copy link
Contributor Author

Closed in favor of #1050

@bookshelfdave
Copy link
Contributor Author

@smith great idea regarding pkg.svc_user + pkg.svc_group, I'm implementing these on PR #1050.

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.

6 participants