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

Add nginx rockon #155

Merged
merged 5 commits into from
Feb 15, 2020
Merged

Add nginx rockon #155

merged 5 commits into from
Feb 15, 2020

Conversation

JasonTarka
Copy link
Contributor

@JasonTarka JasonTarka commented Aug 14, 2018

Use the official nginx image, but users will need to add a default.conf file to their nginx config share for the server to work. This is required as the mounted volume will shadow the config directory in the container that nginx tries to read the config file from.

Effectively the official nginx image, but uses `/config` to hold config files, and copies `default.conf` there.
@magicalyak
Copy link
Contributor

Jason, I don't know if this was an old one or one I did before but I had this in my testing folder. It's linuxserver based (which we prefer due to stability) and if you can verify it, feel free to take credit and submit it.

{
"nginx": {
"containers": {
"nginx": {
"image": "linuxserver/nginx",
"launch_order": 1,
"ports": {
"80": {
"description": "HTTP port. Suggested default: 80",
"host_default": 80,
"label": "HTTP port"
},
"443": {
"description": "HTTPS port. Suggested default: 443",
"host_default": 443,
"label": "HTTPS port"
}
},
"volumes": {
"/config": {
"description": "Choose a Share for configuration. Eg: create a Share called nginx-config for this purpose alone.",
"label": "Config Storage"
}
},
"environment": {
"PUID": {
"description": "Enter a valid UID to run with. It must have full permissions to all Shares mapped in the previous step.",
"label": "UID",
"index": 1
},
"PGID": {
"description": "Enter a valid GID to use along with the above UID. It(or the above UID) must have full permissions to all Shares mapped in the previous step.",
"label": "GID",
"index": 2
}
}
}
},
"description": "nginx by Linuxserver.io",
"more_info": "This Container is a simple nginx webserver configured with default and ssl, and all relevant config files moved out the user via the config directory for ultimate control. it contains some of the basic php-packages. and is built on our internal nginx baseimage.",
"volume_add_support": true,
"website": "https://hub.docker.com/r/linuxserver/nginx/",
"version": "1.0"
}
}

@nfriedly
Copy link
Contributor

nfriedly commented Oct 5, 2018

I just coppied @magicalyak's config and it seems to be working for me. I mapped a different share to /config/www to separate my config from my content.

I'll also mention that there is now an official docker nginx image: https://hub.docker.com/_/nginx/

The Linuxserver.io image includes php support, so it might make more sense to name it something like php-nginx and have both available as separate rock-ons.

@phillxnet
Copy link
Member

phillxnet commented Feb 2, 2019

@JasonTarka Thanks for the Rock-on submission and apologies for the significant delay.
I'm happy to merge this but I think @nfriedly has a good point in that if there is now an official nginx docker image then it should have the plain nginx name. Let me know if I've missed something obvious here however.

@magicalyak Thanks for yet another review, fancy submitting your json via another pr and giving it an appropriate linuxserver name with @nfriedly php suggestion added maybe?

I'm going to label this "help wanted" as we need a consensus on where to go with it.

My suggestion is we name this one after it's image at least ie:
"nginx-jtarka"
however if we have other Rock-ons with more popular images then maybe we should keep duplication to a minimum.

Suggestions welcome.

@JasonTarka
Copy link
Contributor Author

Do we care about having a default config available to the user?

The nginx image reads configs from /etc/nginx/conf.d, but mounting the user-specific config share there leads to it being an empty directory (assuming newly created share). This means the user needs to create a default site config from scratch to get even the most basic of functionality--serving static files--working.

This rockon is using jtarka/nginx, which is just a very slightly modified version of the official nginx image, purely to read from /config, and copy /etc/nginx/conf.d/* to /config on first launch.

Is there any functionality within Rockons to copy files within the container, run a customer Dockerfile, or anything else to present the user with a default config? If there's another way to do that, or you don't care about offering the user a default config, then I will change this to use the nginx container directly.

@magicalyak
Copy link
Contributor

magicalyak commented Feb 4, 2019

@phillxnet you want me to submit a new PR based on linuxserver.io or should we use this one?
@JasonTarka JsaonTarka, we try to avoid using self-managed repos when possible but it's not a hard rule. Also, we've organically adopted linuxserver.io repos when possible (this isn't official but is a trend). For copying files, I use Dockerfile and something like an entrypoint.sh if you want take a look at magicalyak/pocketmine (I'm honestly having some issues with the program but the dockerfile and entrypoint.sh work fine. You can see it copies server.properties and even downloads and unzips a file). The other container that does this is the letsencrypt one I think. I'd use these as examples. The json won't copy files but the dockerfile and scripts it calls certainly can.

@phillxnet
Copy link
Member

@JasonTarka Thanks for the clarification and chuffed you are still up for contributing. And yes I think we do care about providing a default config as I think many users would appreciate an 'up and running' variant to a blank / broken one. And if they can deal with a blank variant then they can also deal with a skeleton one, but the reverse is not true, ie greater target audience if we are up and running directly after install. Oh and re image selection: what @magicalyak said as they have greater experience than me in building docker images and authoring Rock-ons. However given @magicalyak offer above I think we can have both options if you are game. See later in this comment.

@magicalyak Thanks for you input on this one. Much appreciated.
Yes I think submitting an appropriately named linuxserver.io image based nginx Rock-on would be great as we know where we are with those and they have a well deserved reputation. So if you want to pop that in with an obvious name then that would be great. And for those that know less of what image (Rock-on in this case) to chose I again second @nfriedly's suggest that we emphasise either in name or more appropriately in description that it's PHP capable as that will be a key indicator of choice for some.

@JasonTarka So given @magicalyak looks to be up for rolling our 'friendly' variant of nginx based on a well know docker image, maybe there is also a place for an 'advanced' official variant but with a clear indication in the Rock-on description that it is 'broken' out of the door. I.e:

"This official nginx docker image based Rock-on has no default configuration and will therefore not work until one is provided within the given share (show-shares-root-mount-point-here ). Advanced nginx admin use only."

type thing; or what ever better wording you can come up with.

That way, if you are game, we can span the gamut. @magicalyak's friendly batteries included linuxserver.io variant and your vanilla 'no config' variant for those that wish to start from scratch using the official docker image only.

Are we all agreed that these two options can co-exist as long as we name and describe them for what they are?

@magicalyak
Copy link
Contributor

Good with me, I'll create a new nginx for the linuxserver.io

@JasonTarka
Copy link
Contributor Author

If you're okay with it not working out of the box, then I'm okay with it using the official nginx image. I can completely understand not wanting to use some random person's containers, if for no other reason than the potential security issues it could cause.

I'll update this PR to change the image name, and make the description tell the user what they need to do, including a link to a sample config file they can drop in.

@phillxnet
Copy link
Member

@JasonTarka Thanks for your efforts on this and for accommodating the 'advanced' end of our nginx user base. Would be great to put this one to bed.

- Reference the official image
- Mount config volume at `/etc/nginx/conf.d`
- Add notes about adding a default config
Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@JasonTarka I'm hoping you are still game to get this one sorted. It sort of lost steam some where along the line, most likely due to my negligence, so I'm trying to revive it.
I've popped in a couple of suggestions so we can take it form there.

nginx.json Outdated Show resolved Hide resolved
nginx.json Outdated Show resolved Hide resolved
@phillxnet
Copy link
Member

@magicalyak It's been a while but I find myself in this pr again and thought I'd drag up the following prior chat of our:

@magicalyak Thanks for you input on this one. Much appreciated.
Yes I think submitting an appropriately named linuxserver.io image based nginx Rock-on would be great as we know where we are with those and they have a well deserved reputation. So if you want to pop that in with an obvious name then that would be great. And for those that know less of what image (Rock-on in this case) to chose I again second @nfriedly's suggest that we emphasise either in name or more appropriately in description that it's PHP capable as that will be a key indicator of choice for some.

Might be nice to extend our 'developer' Rock-ons range.

@phillxnet
Copy link
Member

@JasonTarka we have had a potential renewed interest in this pull request from @mikey0000 in the following issue:
rockstor/rockstor-core#2129
Let us know if you are game to pick this up again with my recent suggestions otherwise we can start over in a new pr from where I unfortunately neglected this pull request from way back.

@JasonTarka
Copy link
Contributor Author

This is outside the scope of this PR, but is the standard now to manually include a link to the image in the description?

It feels like that's something that could (and probably should) be pulled dynamically from the containers config. Generating the links would improve consistency (especially visual), reduce manual effort, and lessen the chances of the links becoming stale if the selected image changes.

@FroggyFlox
Copy link
Member

This is outside the scope of this PR, but is the standard now to manually include a link to the image in the description?

You are correct; see Issue #31 for the history and origin for this.

It feels like that's something that could (and probably should) be pulled dynamically from the containers config. Generating the links would improve consistency (especially visual), reduce manual effort, and lessen the chances of the links becoming stale if the selected image changes.

That is an excellent idea and something that should be implemented. Would you mind opening an issue for that in the rockstor-core repository (as this is where the changes would need to be made)? This way we can keep track of it and make it available for somebody to start working on it if desired.

Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@JasonTarka Sorry but my last suggestion only got us part of the way, oh and I broke the json !!

If your happy with these proposed changes I think we might be getting there finally.

Description-nginx-rockon

And the Web-UI button:

test-content-pic

nginx.json Outdated Show resolved Hide resolved
nginx.json Outdated Show resolved Hide resolved
nginx.json Outdated Show resolved Hide resolved
JasonTarka and others added 2 commits February 12, 2020 22:22
- Remove superfluous comma
- Get rid of an extra period, and make less of the text a link.
@JasonTarka
Copy link
Contributor Author

It feels like that's something that could (and probably should) be pulled dynamically from the containers config. Generating the links would improve consistency (especially visual), reduce manual effort, and lessen the chances of the links becoming stale if the selected image changes.

That is an excellent idea and something that should be implemented. Would you mind opening an issue for that in the rockstor-core repository (as this is where the changes would need to be made)? This way we can keep track of it and make it available for somebody to start working on it if desired.

Created an issue to track this: rockstor/rockstor-core/issues/2131

@magicalyak
Copy link
Contributor

I just realized you had pinged my on this @phillxnet and it's been a while, is the nginx proxy rockon sufficient for our needs at this point or should i push forward with the linuxserver.io one?

@phillxnet
Copy link
Member

@magicalyak Hello again. Re:

... is the nginx proxy rockon sufficient for our needs at this point or should i push forward with the linuxserver.io one?

I think it would be great to have both this one, nearly done, and a beginner friendly linuxserver.io one as the latter would presumably run as selectable non root user with default config and PHP and what not. This one servers better those that want to use the official nginx docker image and their own or @JasonTarka example config.

So if you are still game then it would be great to also have a linuxserver.io nginx one, we just need to name it accordingly as discussed earlier in this thread.

Thanks for the response and being willing to submit yet another Rock-on.

Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@JasonTarka I'm pretty sure this is now sorted. Thanks for your perseverance on this and apologies for my neglecting it during our busy transition period.

Rock-on_entry

And your final fix / display improvements look to have got this one ready to merge.

@phillxnet phillxnet merged commit 79e43f4 into rockstor:master Feb 15, 2020
@phillxnet
Copy link
Member

@JasonTarka, @FroggyFlox, @magicalyak, and @nfriedly

I've now added commit bb584d2 to complement this pr's now merged json and am about to publish.

Thanks to all.

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.

5 participants