Skip to content

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

Merged
buchdag merged 3 commits intonginx-proxy:mainfrom
jessejoe:improve_skipping_dhparam
Apr 29, 2021
Merged

Change dhparam positional args to be inherited vars, standardize naming#1263
buchdag merged 3 commits intonginx-proxy:mainfrom
jessejoe:improve_skipping_dhparam

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 buchdag force-pushed the improve_skipping_dhparam branch from 7223710 to ab81ff8 Compare April 29, 2021 01:19
@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

Development

Successfully merging this pull request may close these issues.

DHPARAM_GENERATION env var ignored with false value docker-entrypoint.sh: Only setting DHPARAM_GENERATION fails

4 participants