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

Update readme about plugins/docker has special privileges #211

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kevinsimper
Copy link

kevinsimper commented Jan 2, 2019

I didn't know about this and I could not understand why I couldn't get other docker images to work the same way as plugins/docker worked. Wasted around 2 hours trying all kinds of stuff. Not mad, just want to spare other peoples time as I think it is quite an important undocumented feature, something quite unusual for Drone for what my impression has been.

As you can see it is not mentioned a single time in the docs, and being "container-native" you could assume that people would want to test building Docker images https://github.com/drone/docs/search?q=DRONE_ESCALATE&type=Code

README.md Outdated
@@ -2,6 +2,10 @@

Drone plugin to build and publish Docker images to a container registry.

### Special privilieges

This docker image is specified in Drone to run with Privilieged mode. If you fork this repository to run it on your own, you need to specify your docker image in the `DRONE_ESCALE`, as the container needs to run in privilieged mode, otherwise it will not work! https://github.com/drone/drone/blob/master/cmd/drone-server/server.go

This comment has been minimized.

@techknowlogick

techknowlogick Jan 2, 2019

TYPO, should be DRONE_ESCALATE

This comment has been minimized.

@kevinsimper

kevinsimper Jan 4, 2019

Thank you for noticing 👍

@kevinsimper kevinsimper force-pushed the kevinsimper:patch-1 branch from 2c98a92 to dcd0913 Jan 4, 2019

@@ -2,6 +2,10 @@

Drone plugin to build and publish Docker images to a container registry.

### Special privilieges

This docker image is specified in Drone to run with Privilieged mode. If you fork this repository to run it on your own, you need to specify your docker image in the `DRONE_ESCALATE`, as the container needs to run in privilieged mode, otherwise it will not work! https://github.com/drone/drone/blob/master/cmd/drone-server/server.go

This comment has been minimized.

@tboerger

tboerger Jan 6, 2019

Member

You got a typo in privileged multiple times, please drop the moving reference to the drone core source file, you should append "environment variable" to DRONE_ESCALATE.

Generally I'm not sure if the wording is that great.

This comment has been minimized.

@kevinsimper

kevinsimper Jan 6, 2019

I can agree the wording can be better @tboerger :) How would it be said better?

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Jan 6, 2019

I think the server configuration instructions should be decoupled from the plugin documentation, since the configuration instructions are specific to the version of Drone. For example Drone 0.8 uses DRONE_ESCALATE and Drone 1.0 uses DRONE_RUNNER_PRIVILAGED_IMAGES.

# Forking this Plugin

If you want to use a forked version of this plugin with your Drone server
instance it may require special configuration. For configuration instructions
please see [this thread](https://discourse.drone.io/...).

I think that directing users to a discourse thread is optimal since the instructions can be augmented with comments and discussion.

@kevinsimper

This comment has been minimized.

Copy link

kevinsimper commented Jan 6, 2019

@bradrydzewski I agree with you that hardcoding things is not so great idea, I am not sure that a Discourse thread is a better alternative since only the author can change the first post, unlike a Github Repo.

Since this plugin has special behavior which are undocumented, I think it is important to highlight it where most people would expect to find it. Either in the documentation or the plugin that has the special behavior.

Or maybe remove the special behavior so it doesn't have to be documented and treat all containers equal in a container native ecosystem where side-effects are unwanted :)

Or a mark in the UI showing that certain containers are treated differently than others because of a string match in their name :)

Just some ideas :)

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Jan 6, 2019

The reason I suggest discourse is that the instructions can be augmented with comments, as opposed to a pull request. I do not want version-specific information to leak into the plugin dogumentation, so it needs to be documented somewhere else.

Changing Drone behavior is not an option for security reasons. We whitelist specific plugins so that you do not need to enable trusted mode for your entire repository to use this plugin, which would otherwise reduce security for public github repositories and allow a malicious pull request to pwn the host machine.

@kevinsimper

This comment has been minimized.

Copy link

kevinsimper commented Jan 6, 2019

The reason I suggest discourse is that the instructions can be augmented with comments, as opposed to a pull request. I do not want version-specific information to leak into the plugin dogumentation, so it needs to be documented somewhere else.

Yes, that makes sense if they refer to some original documentation. Forums are seldom a good documentation resource to scavenge through.

Changing Drone behavior is not an option for security reasons. We whitelist specific plugins so that you do not need to enable trusted mode for your entire repository to use this plugin, which would otherwise reduce security for public github repositories and allow a malicious pull request to pwn the host machine.

Yeah I understand why, what do you think of my other suggestions that does not change the behavior? @bradrydzewski :)

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Jan 6, 2019

I think a link to documentation / thread is sufficient to solve this problem, as opposed to development changes to the user interface which are costly to develop and maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment