-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
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 |
There was a problem hiding this comment.
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 })) |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
& here
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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
@julesterrien updated |
src/nginx.js
Outdated
'http {', | ||
'ssl_certificate /etc/ssl/fullchain1.pem;', | ||
'ssl_certificate_key /etc/ssl/privkey1.pem;', | ||
]; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
@julesterrien updated |
@theQuazz running into this err when running locally: That's because |
@julesterrien tested locally and it appears to be working! (at least to the point where it provisions the task) |
sweet approved! |
This will allow people to customize their own NGINX files how they see fit