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

Custom NGINX configuration file option #2

Merged
merged 8 commits into from
Aug 10, 2018
Merged

Custom NGINX configuration file option #2

merged 8 commits into from
Aug 10, 2018

Conversation

theQuazz
Copy link
Contributor

@theQuazz theQuazz commented Aug 8, 2018

This will allow people to customize their own NGINX files how they see fit

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@theQuazz theQuazz requested a review from julesterrien August 8, 2018 21:33
README.md Outdated
@@ -265,3 +268,51 @@ If you use AWS's ECR service to host your images, you may want to consider addin
If you run `init()` more than once, you'll see errors as wonqa tries to create resources that have already been created by a previous call. You'll need to log into the AWS console to delete these resources and try again.

If the `wonqa.create()` runs but you are unable to accessing your environment causes a timeout (for eg., if it hangs on `Waiting for your QA env to return a 200 OK`), this probably means that the security group that you are using does not have ingress rules for HTTP and HTTPS as well as the rules required by your app to work.


## NGINX Config
Copy link
Contributor

Choose a reason for hiding this comment

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

there's an /example dir here which we could use to put this

@@ -46,7 +47,7 @@ const create = ({
}) => {
const scope = {};
return configure({ WONQA_DIR, awsRegion })
.then(() => writeConfFile({ WONQA_DIR, servers }))
.then(() => writeConfFile({ WONQA_DIR, servers, configurationPath }))
Copy link
Contributor

Choose a reason for hiding this comment

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

have a basic integration test at create.test.js which we may want to edit to make sure this value is always passed if it exists

let config;

if (configurationPath) {
config = fs.readFileSync(configurationPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add some basic validation on there. maybe something like:

const required = [
	“http {”
	“ssl_certificate     /etc/ssl/fullchain1.pem;”
	“ssl_certificate_key /etc/ssl/privkey1.pem;”
	“map $http_upgrade $connection_upgrade {”
    ...all the ones we need
];
required.every(el => config.includes(el));


const s = servers.map(({ port, serverName }) => serverConfig(port, serverName));
const s = servers.map(({ port, serverName }) => serverConfig(port, serverName));
Copy link
Contributor

Choose a reason for hiding this comment

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

meant to add the space here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to elevate it to the same level as the block in the if statement


const config = `
config = `
Copy link
Contributor

Choose a reason for hiding this comment

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

& here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as other spacing

src/validate.js Outdated
@@ -59,7 +60,14 @@ const validateHTTPSOptions = ({
} else {
throw new Error('cachePath is undefined');
}
validateNginxConf(servers);
if (configurationPath) {
const stats = fs.lstatSync(cachePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

meant configurationPath here?

src/validate.js Outdated
if (configurationPath) {
const stats = fs.lstatSync(cachePath);
if (!stats.isFile()) {
throw new Error('nginx.configurationPath is not a path to a directory');
Copy link
Contributor

Choose a reason for hiding this comment

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

error should say it's not a file

@theQuazz
Copy link
Contributor Author

theQuazz commented Aug 8, 2018

@julesterrien updated

src/nginx.js Outdated
'http {',
'ssl_certificate /etc/ssl/fullchain1.pem;',
'ssl_certificate_key /etc/ssl/privkey1.pem;',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put all validations in validate.js

Also, should we not validate for others as well? for eg.:
// at least 1 server block
server {

// default server
listen 443 ssl default_server;

// the default server blocks stuff:
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header Host $http_host;
proxy_redirect off;
proxy_http_version 1.1;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection "upgrade";

// events
events {
accept_mutex on;
worker_connections 1024;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it here so we only need to read the file once. And in terms of validation, I'd rather be permissive with what they do in their nginx file, what works for us might not work for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok yea being permissive makes sense to me too. i just want to make sure the nginx conf has at least what it needs to run, for eg. a default server with the basic configs for that server. then again, maybe it's safe to assume that if the user uses a custom config she knows how to set them up

will look into the events config - not sure if this is essential or custom to us. do you know?

src/validate.js Outdated
@@ -59,7 +60,14 @@ const validateHTTPSOptions = ({
} else {
throw new Error('cachePath is undefined');
}
validateNginxConf(servers);
if (configurationPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on just calling validateNginxConf({ servers, configurationPath }); here and then doing all nginx validations within that fn?

@theQuazz
Copy link
Contributor Author

theQuazz commented Aug 9, 2018

@julesterrien updated

@julesterrien
Copy link
Contributor

@theQuazz running into this err when running locally:
screen shot 2018-08-09 at 12 24 14 pm

That's because ssl.sh uses the serverName values from nginx.servers to create the domains value used to request SSL certs. To fix, we either need to parse the nginx conf file to find the server names and create the domains string (error prone imo) so have the user also provide the servers array so we can create this.
What do you think?

@theQuazz
Copy link
Contributor Author

@julesterrien tested locally and it appears to be working! (at least to the point where it provisions the task)

@julesterrien
Copy link
Contributor

sweet approved!

@theQuazz theQuazz merged commit ceb2516 into master Aug 10, 2018
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.

None yet

2 participants