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

Change dhparam positional args to be inherited vars, standardize naming #1263

Merged
merged 3 commits into from
Apr 29, 2021

Conversation

jessejoe
Copy link
Contributor

@jessejoe jessejoe commented Apr 9, 2019

PR #913 added DHPARAM_GENERATION as a positional argument to generate-dhparam.sh. However, since it was the second positional argument,DHPARAM_BITS would also have to be defined or DHPARAM_GENERATION would be read into DHPARAM_BITS. This changes the arguments to be inherited variables which do not depend on order, just declaration.

Also change instances of GENERATE_DHPARAM to DHPARAM_GENERATION since it's unnecessary to have another variable. I think GENERATE_DHPARAM is actually a better name (verb vs. noun), but DHPARAM_GENERATION is already defined and may break someone if changed.

Addresses #913 (comment)


Behavior before:

$ docker run -it --rm -e "DHPARAM_GENERATION=false" testing /bin/bash
WARNING: /etc/nginx/dhparam/dhparam.pem was not found. A pre-generated dhparam.pem will be used for now while a new one
is being generated in the background.  Once the new dhparam.pem is in place, nginx will be reloaded.
dhparam: Can't parse "false" as a number

Behavior after:

$ docker run --rm -e "DHPARAM_GENERATION=false" testing /bin/bash
Skipping Diffie-Hellman parameters generation and Ignoring pre-generated dhparam.pem

@kamermans
Copy link
Contributor

[CC @jwilder] this looks good to me, thanks @jessejoe!

@buchdag
Copy link
Member

buchdag commented May 20, 2019

@jwilder this is a required fix, could you merge please ?

Copy link

@joren485 joren485 left a comment

Choose a reason for hiding this comment

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

I believe with this change

# Generate dhparam file if required
# Note: if $DHPARAM_BITS is not defined, generate-dhparam.sh will use 2048 as a default
# Note2: if $DHPARAM_GENERATION is set to false in environment variable, dh param generator will skip completely
/app/generate-dhparam.sh $DHPARAM_BITS $DHPARAM_GENERATION

should be changed to

# Generate dhparam file if required
/app/generate-dhparam.sh

in entrypoint.sh.

@buchdag buchdag added the type/fix PR for a bug fix label Apr 29, 2021
@buchdag buchdag self-assigned this Apr 29, 2021
jessejoe and others added 3 commits April 29, 2021 03:11
PR nginx-proxy#913 added `DHPARAM_GENERATION` as a positional argument to
generate-dhparam.sh. However, since it was the second positional argument,
`DHPARAM_BITS` would also have to be defined or `DHPARAM_GENERATION` would be
read into `DHPARAM_BITS`. This changes the arguments to be inherited variables
which do not depend on order, just declaration.

Also change instances of `GENERATE_DHPARAM` to `DHPARAM_GENERATION` since it's
unnecessary to have another variable. I think `GENERATE_DHPARAM` is actually a
better name (verb vs. noun), but `DHPARAM_GENERATION` is already defined and
may break someone if changed.

Addresses nginx-proxy#913 (comment)
@buchdag
Copy link
Member

buchdag commented Apr 29, 2021

  • rebased on main
  • fixed merge conflict
  • included @joren485 suggestion
  • did a bit of linting on the shell scripts while we're at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix PR for a bug fix
Projects
None yet
4 participants