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

Do not to use --stdout for relay credential generate #1267

Closed
wants to merge 7 commits into from

Conversation

aminvakil
Copy link
Collaborator

First brought up in #1251 (comment)

@aminvakil aminvakil changed the title First attempt not to use --stdout Do not to use --stdout for relay credential generate Jan 23, 2022
@aminvakil
Copy link
Collaborator Author

relay tries to create credentials.json with 10001:10001 owner/group and gets permission denied.

   2022-01-23T12:25:00Z [relay_log::utils] ERROR: could not write config file (file /tmp/relay/credentials.json)
    caused by: Permission denied (os error 13)

@aminvakil
Copy link
Collaborator Author

So we can see what uid/gid is running docker, try to change uid/gid of relay_directory to 10001:10001 to let relay write on it, then change it back to uid/gid of user running docker, but this has a couple of disadvantages.

  1. It's hacky!
  2. If something happens during these commands or relay docker command breaks, this folder will remain in unwanted condition (10001:10001 uid/gid).
  3. Changing ownership needs sudo privilege which some user may not have (although the workaround would be something like docker run --rm --volume "$(pwd)/relay:/tmp/relay" alpine chown -R 10001:10001 /tmp/relay)

But this is what I have thought of, @BYK @untitaker @chadwhitacre What do you think about this?

Is it something obvious I'm missing which can handle this situation easily? Or redirecting and using --stdout is better than this?

@aminvakil
Copy link
Collaborator Author

Created credentials.json is different now:

{
  "secret_key": "JNGDJB-lmrJskamELp9Ge67ljccSKHKFLcRb5cBeHrA",
  "public_key": "vu58jZocizKvK6v2b6Q85aOFZny3mzdCvXBr1IUiiMg",
  "id": "cd11c1cc-8b5c-4adc-89dc-c1c20beb2e1e"
}

It used to be like this:

{"secret_key":"JNGDJB-lmrJskamELp9Ge67ljccSKHKFLcRb5cBeHrA","public_key":"vu58jZocizKvK6v2b6Q85aOFZny3mzdCvXBr1IUiiMg","id":"cd11c1cc-8b5c-4adc-89dc-c1c20beb2e1e"}

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Seems like relay is still complaining about not being able to read credentials: https://github.com/getsentry/self-hosted/runs/4913039350?check_suite_focus=true#step:5:1026

@@ -13,11 +14,12 @@ if [[ ! -f "$RELAY_CREDENTIALS_JSON" ]]; then
# JSON. We hit this case as we redirect output to the same config folder,
# creating an empty credentials file before relay runs.

docker run --rm --volume "$(pwd)/$RELAY_DIRECTORY:/tmp/relay" busybox chown -R 10001 /tmp/relay
Copy link
Member

Choose a reason for hiding this comment

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

Why not put these in a custom entrypoint script and use $dcr instead?

Copy link
Collaborator Author

@aminvakil aminvakil Jan 24, 2022

Choose a reason for hiding this comment

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

Does this (1d7c7a7) look good?

@aminvakil
Copy link
Collaborator Author

aminvakil commented Jan 24, 2022

Seems like relay is still complaining about not being able to read credentials: https://github.com/getsentry/self-hosted/runs/4913039350?check_suite_focus=true#step:5:1026

I've made a change trying to read it, and it seems that user running docker which is runner can read it, the only difference this PR is making is the structure which I don't know if it should be important or not.

https://github.com/getsentry/self-hosted/runs/4919429511?check_suite_focus=true#step:4:526

@BYK
Copy link
Member

BYK commented Jan 24, 2022

is making is the structure which I don't know if it should be important or not.

Both are JSON blobs, just one is better formatted. Should be fine.

$dcr \
--no-deps \
--volume "$(pwd)/$RELAY_DIRECTORY:/tmp/relay" \
relay --config /tmp/relay credentials generate
Copy link
Member

Choose a reason for hiding this comment

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

I'd say, in the final version, we can just use the mounter relay config dir for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate please?

Copy link
Member

Choose a reason for hiding this comment

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

We already mount the relay directory for the relay service and set that as the config folder so you should not need the --volume "$(pwd)/$RELAY_DIRECTORY:/tmp/relay" part and --config /tmp/relay part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have this:

relay:
<<: *restart_policy
image: "$RELAY_IMAGE"
volumes:
- type: bind
read_only: true
source: ./relay
target: /work/.relay
- type: bind
read_only: true
source: ./geoip
target: /geoip
depends_on:
kafka:
<<: *depends_on-healthy
redis:
<<: *depends_on-healthy
web:
<<: *depends_on-healthy

which mounts relay directory read-only and AFAICS there is no --config.

Copy link
Member

Choose a reason for hiding this comment

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

We can mount it as writable for that run but yeah, I get your point :)

@aminvakil aminvakil closed this Jan 24, 2022
@aminvakil aminvakil reopened this Jan 24, 2022
@aminvakil aminvakil closed this Jan 24, 2022
@aminvakil aminvakil reopened this Jan 24, 2022
@chadwhitacre
Copy link
Member

@aminvakil Can we switch this to a draft PR if it's still under active development? 😶

@aminvakil aminvakil force-pushed the relay_credential_stdout branch 2 times, most recently from 939fb44 to 61024e3 Compare January 24, 2022 17:59
@aminvakil
Copy link
Collaborator Author

@aminvakil Can we switch this to a draft PR if it's still under active development? no_mouth

Sure, sorry for the noise these past days.

@aminvakil aminvakil marked this pull request as draft January 24, 2022 18:01
@aminvakil aminvakil force-pushed the relay_credential_stdout branch 2 times, most recently from 5805d92 to 9c4b533 Compare January 26, 2022 16:49
@aminvakil aminvakil mentioned this pull request Feb 1, 2022
@aminvakil
Copy link
Collaborator Author

Considering current changes in relay generation, this chowning and back is too hacky, I would say let's keep at is it for now, we can take a look at this later if we wanted to.

@aminvakil aminvakil closed this Feb 4, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2022
@aminvakil aminvakil deleted the relay_credential_stdout branch March 12, 2022 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants