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

Allow custom webserver port #21

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

Conversation

jtackaberry
Copy link

This PR adds a port value to the Helm chart. The primary use case is to enable binding to high ports to avoid running Drone server as root.

@jtackaberry
Copy link
Author

Note that the CI failure does not appear to be a consequence of this PR.

@ashwilliams1
Copy link
Contributor

ashwilliams1 commented Aug 28, 2020

can you provide more details why you would change the port inside the container? usually one would change the host port mapping. changing the DRONE_SERVER_PORT is generally not required or recommended. Also, this probably won't work as intended because DRONE_SERVER_PORT must be prefixed by a colon (i.e. :8080).

@jtackaberry
Copy link
Author

can you provide more details why you would change the port running inside the container?

So the Drone server pod can be run as non-root. At the moment, it does look like the server and the kube runner work fine non-root, although the build container spawned by the runner currently assumes root.

Also, this probably won't work as intended because DRONE_SERVER_PORT must be prefixed by a colon.

That's no problem, the PR takes this into account.

@@ -12,3 +12,6 @@ data:
{{- range $envKey, $envVal := .Values.env }}
{{ $envKey | upper }}: {{ $envVal | quote }}
{{- end }}
{{- if .Values.port }}
DRONE_SERVER_PORT: :{{ .Values.port }}
Copy link
Author

Choose a reason for hiding this comment

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

Note the : prefix.

Copy link

Choose a reason for hiding this comment

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

+1 can we merge that?

@bakito
Copy link

bakito commented Dec 28, 2020

Hi @jtackaberry I run into the same problem, when running the drone server in OpenShift.
Looking for a work around I found a more generic way to override the port.
https://github.com/bakito/drone-charts/commit/6822525f1752835f3010e96ed8f314553f2c1adf

Your solution also may lead into having the DRONE_SERVER_PORT variable twice in the configmap if it is also defined as env variable.

@james-mcgoodwin
Copy link

Putting a plus-one here as well. Getting drone to run with out root is a goal for us, and it requires the chart to switch to a different port.

At the moment we have to use a very permissive PSP to get drone to run, and we'd prefer not to.

@jimsheldon
Copy link
Contributor

Apologies for the extreme delay on this.

Since this PR, the server chart has changed a bit. containerPort is now hard-coded to port 80 https://github.com/drone/charts/blob/master/charts/drone/templates/deployment.yaml#L41

I think this is a mistake, we should support changing the port.

I am a little hesitant to automatically set DRONE_SERVER_PORT in the configmap, we don't set any other variables this way. Could we instead add a comment in the valyes.yaml file where the port is documented, mentioning that DRONE_SERVER_PORT must be set to the same value in the env section?

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