-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
Attempt to catch cases of incorrect datadir ownership #74
base: master
Are you sure you want to change the base?
Conversation
b127173
to
fab9397
Compare
fab9397
to
0067fdb
Compare
cmd/elastic.sh
Outdated
function elastic_start(){ | ||
mkdir -p $DATA_DIR/elasticsearch | ||
# attemp to set proper permissions if running as root | ||
chown $DOCKER_USER $DATA_DIR/elasticsearch 2>/dev/null || true | ||
|
||
# record the owner of the Elasticsearch directory | ||
elasticsearch_owner_uid=$(stat --format '%u' $DATA_DIR/elasticsearch) |
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.
Still not working for me...
stat --format '%u' /data/pelias/docker/projects/portland-metro/elasticsearch
stat: illegal option -- -
usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]
I think you might need to use the g* version:
gstat --format '%u' /data/pelias/docker/projects/portland-metro/elasticsearch
501
There is already a process for handling GNU versions of utils under OSX in https://github.com/pelias/docker/blob/master/pelias#L10
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.
Good catch yet again. It turns out GNU coreutils and BSD coreutils do not have compatible flags here.
My .bashrc
adds the homebrew coreutils directory to my $PATH. Definitely convenient, but we can't expect everyone to do that.
I've added an environment variable for the stat
command with a similar pattern. It should really work this time.
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.
Hmm.. I still don't think this is working as expected...
I've never had a problem with permissions on my laptop and now I would get a failure when trying to start elasticsearch
, which would also probably be the case for all other Mac users too?
I don't have anything special set up for my environment, I decided to avoid aliases etc in order to have a 'stock standard' Mac dev environment which was closest to what our users would be running.
➜ portland-metro git:(check-data-dir-permissions) ✗ pelias elastic start
user 1000 cannot access elasticsearch directory at /data/pelias/docker/projects/portland-metro
please run 'sudo chown 1000 /data/pelias/docker/projects/portland-metro/elasticsearch'
➜ portland-metro git:(check-data-dir-permissions) ✗ ls -lah /data/pelias/docker/projects/portland-metro
total 0
drwxr-xr-x 14 peter staff 448B Mar 4 16:33 .
drwxr-xr-x 3 peter staff 96B Feb 8 08:14 ..
drwxr-xr-x 2 peter staff 64B Feb 8 08:18 blacklist
drwxr-xr-x 3 peter staff 96B Feb 8 08:18 csv
drwxr-xr-x 3 peter staff 96B Mar 4 16:33 elasticsearch
drwxr-xr-x 3 peter staff 96B Mar 4 16:32 es-bak
drwxr-xr-x 20 peter staff 640B Feb 8 08:41 interpolation
drwxr-xr-x 4 peter staff 128B Feb 8 08:18 openaddresses
drwxr-xr-x 3 peter staff 96B Feb 8 08:18 openstreetmap
drwxr-xr-x 4 peter staff 128B Feb 8 08:23 placeholder
drwxr-xr-x 3 peter staff 96B Feb 8 08:22 polylines
drwxr-xr-x 4 peter staff 128B Feb 8 08:18 tiger
drwxr-xr-x 16 peter staff 512B Feb 8 08:20 transit
drwxr-xr-x 5 peter staff 160B Feb 8 08:19 whosonfirst
➜ portland-metro git:(check-data-dir-permissions) ✗ ls -lah /data/pelias/docker/projects/portland-metro/elasticsearch
total 0
drwxr-xr-x 3 peter staff 96B Mar 4 16:33 .
drwxr-xr-x 14 peter staff 448B Mar 4 16:33 ..
drwxr-xr-x 3 peter staff 96B Mar 4 16:33 nodes
➜ portland-metro git:(check-data-dir-permissions) ✗ whoami
peter
➜ portland-metro git:(check-data-dir-permissions) ✗ groups
staff operator everyone localaccounts _appserverusr admin _appserveradm _lpadmin com.apple.sharepoint.group.1 _appstore _lpoperator _developer _analyticsusers com.apple.access_ftp com.apple.access_screensharing com.apple.access_ssh
➜ portland-metro git:(check-data-dir-permissions) ✗ gstat --format '%u' /data/pelias/docker/projects/portland-metro/elasticsearch
501
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.
One solution is to change the ID in https://github.com/pelias/docker/blob/master/projects/portland-metro/.env#L3 as per the readme.
This seems like an additional step for Mac users which will deter them from adopting Pelias, I'd really love it to 'just work first time'.
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.
It seems as though the chown
command is failing due to a permissions error, the messages are being silenced:
➜ portland-metro git:(check-data-dir-permissions) ✗ chown 1000 /data/pelias/docker/projects/portland-metro/elasticsearch
chown: /data/pelias/docker/projects/portland-metro/elasticsearch: Operation not permitted
➜ portland-metro git:(check-data-dir-permissions) ✗ echo $?
1
0067fdb
to
c752cf7
Compare
Heya, So backing up a bit, I'm trying to remember why we are doing it like this in the first place? It seems like an elegant solution is to put this at the top of the pelias executable? # Detect user and group ids for current user
export DOCKER_USER=$(id -u):$(id -g); So the In order to support export DOCKER_USER=$(id -u ${SUDO_USER-${USER}}):$(id -g ${SUDO_USER-${USER}}); This would mean that users couldn't easily override the value with a custom ID/GID value, there are two options for that:
The nice thing about the script above is, it's super simple, portable and leaves local file permissions set to the executing user by default. The thing I don't like about the existing solution is that it assumes the UID is 1000 and then tries to force the filesystem to 1000, which makes it hard to delete and edit files on a local filesystem when running as a non-1000 user. One final consideration is what will happen when someone uses edit: yep, it also emits a warning, which is nice |
I think you're on the right track with reworking the permissions. The important criteria we want to meet, roughly in priority order, are:
So I think we can achieve this by setting |
👍 sounds perfect, the only thing I worry about is existing projects with an Particularly for users who copied an existing project into a new directory and run OSX, in that case they would probably start experiencing an error, but this might be worth doing anyway since it would be correct for future users. |
If data_dir is owned by curent user and group is users (and DOCKER_USER is not set) then
This is with docker 17.05.0-ce and docker-compose 1.23.2 on debian jessie 8.8.
|
changing to user 'deploy' ( uid 1000 ) and chown of data_dir to deploy (group users) , and adding 'deploy' to docker group ( |
If the Elasticsearch datadir is not owned by the user specified in $DOCKER_USER, Elasticsearch will fail to start. Since #55 the helper scripts attempt to remedy this situation if the `pelias` script is run as root (which is not recommended but is often done). However, if the `pelias` script is not run as root and the permissions are incorrect, this situation cannot be automatically fixed. This code attempts to detect that case and recommend the proper command to run (with sudo) and set proper directory permissions. Connects #31 Connects #73
c752cf7
to
b9f4567
Compare
Okay, I've revived this PR with some minor changes. While the earlier comment about autodetecting $DOCKER_USER is a great idea, I believe we should solve that separately. With more time and clarity, here is the purpose of this PR: to detect (and fix) the case where a user has created the $DATA_DIR with I've changed the logic so that the script will now try to detect and fix permission issues, running a @missinglink can you review this and let me know what you think? |
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.
My preference would be to simplify the permissions as per my old comment, and I see you agree, so ignoring that for a second....
I'm happy to merge this, two considerations:
- need to check it works on mac too (ie all the commands are equivalent)
- this checks the ownership of the directory but doesn't consider group ownership, I understand that's more complicated to check but may be expected by some users?
I just tested this on my Mac where I had an existing project with pelias elastic start
user 1000 cannot access elasticsearch directory at /data/pelias/docker/projects/portland-metro
please run 'sudo chown 1000 /data/pelias/docker/projects/portland-metro/elasticsearch' |
Right, and just to clarify no matter how much we simplify the process of configuring/determining
I should be able to test it on my Mac but would love a double check :) As for group ownership, I don't believe that matters for Pelias and our Docker setup, but some people may care about group ownership. As it stands this PR will run We could either leave things like this, or filter out any group in |
If I switch back to gid
uid=501(peter) gid=20(staff) groups=20(staff),5(operator),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),33(_appstore),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae) cat .env
COMPOSE_PROJECT_NAME=pelias
DATA_DIR=/data/pelias/docker/projects/portland-metro
DOCKER_USER=1000 ls -lah /data/pelias/docker/projects/portland-metro/elasticsearch
total 0
drwxr-xr-x 3 peter staff 96B Jul 30 15:54 .
drwxr-xr-x 14 peter staff 448B Jul 29 16:53 ..
drwxrwxr-x 3 peter staff 96B Jul 30 15:54 nodes |
Try deleting the elasticsearch directory completely and then re-creating it with Alternatively, try running one of the importers, and I bet they will fail when they try to actually write to Elasticsearch. |
It's late for me here, so I'm maybe not explaining well. I'm 👍 for adding a check for the case where the local directory is not readable by the For instance, I can create the And my local user is |
Maybe time to revisit this, I was thinking we could pelias compose run elasticsearch test -w /usr/share/elasticsearch/data
echo $?
0 |
I'm actually hoping this won't be needed in practice any more. I suppose if people ran Maybe it's wishful thinking, and in that case we can do something like you suggest. |
If the Elasticsearch datadir is not owned by the user specified in $DOCKER_USER, Elasticsearch will fail to start.
Since #55 the helper scripts attempt to remedy this situation if the
pelias
script is run as root (which is not recommended but is often done).However, if the
pelias
script is not run as root and the permissions are incorrect, this situation cannot be automatically fixed.This code attempts to detect that case and recommend the proper command to run (with sudo) and set proper directory permissions.
Connects #31
Connects #73