NGINX Plus R33 related fixes #3
Conversation
NGINX Plus now requires license.jwt to start.
It is useful to test this script with preview repos.
| if [ $TARGETVER -ge 31 ]; then | ||
| if [ $TARGETVER -ge 33 ]; then | ||
| mv $TMPDIR/license.jwt $ABSPATH/etc/nginx/license.jwt | ||
| echo "mgmt { license_token $ABSPATH/etc/nginx/license.jwt; }" >> $ABSPATH/etc/nginx/nginx.conf |
There was a problem hiding this comment.
In r33, the uuid_file directive was obsolete and replaced by state_path directive. The path can remain the same as for uuid_file, I think, however, the state file will be created automatically.
There was a problem hiding this comment.
Hmmm, do you mean we also need a state_path set here for R33+?
There was a problem hiding this comment.
Unfortunately, yes, this is another change that breaks similar scenarios. Actually, we could keep the uuid_file directive, but nginx will log a warning that this directive is deprecated. Starting from 33, state_path will indicate the path to the state file.
| @@ -83,6 +86,11 @@ if [ "$NGXPATH" = '' ] && ( [ "$ACTION" = 'install' ] || [ "$ACTION" = 'upgrade' | |||
| fi | |||
| fi | |||
|
|
|||
| if [ "$NGXLICENSE" = '' ] && ( [ "$ACTION" = 'install' ] || [ "$ACTION" = 'upgrade' ] ) ; then | |||
There was a problem hiding this comment.
Hmm, I’m not sure it’s worth overcomplicating the logic, but this approach lacks backward compatibility. Let’s say I want to install r32, but I still have to provide the license file…
There was a problem hiding this comment.
Right. The problem is that when we parse the CLI opts we don't really know which version we're going to install.
Dropping the requirement for the -j option to be specified means R33 will not be installed/launched easily by default, which this script is kinda supposed to do. I'm not sure on how to deal with this one, to be honest.
There was a problem hiding this comment.
I sent you a pr where I implemented this approach:
JWTREQ="NO"
if [ "$ACTION" = 'install' ] || [ "$ACTION" = 'upgrade' ]; then
for pkg in $FILES; do
pkgname=$(basename "$pkg")
version=$(echo "$pkgname" | sed -n 's/^nginx-plus[_-]\([0-9]\+\).*$/\1/p')
if [ -n "$version" ]; then
if [ "$version" -ge 33 ]; then
JWTREQ="YES"
fi
break
fi
done
fi
if [ "$NGXLICENSE" = '' ] && ( [ "$ACTION" = 'install' ] || [ "$ACTION" = 'upgrade' ] ) && [ "$JWTREQ" = 'YES' ]; then
echo "-j option is mandatory for install/upgrade with NGINX Plus version >=33"
exit 1
fi
Packages in the repository are named based on a template, so conclusions can be drawn from the filename: nginx-plus_<major>-<minor>~.... This won’t work in all cases, but in the vast majority of instances users will be obtaining files via fetch param I think.
| @@ -367,7 +382,13 @@ upgrade() { | |||
| [ -d $TMPDIR/usr/lib64/ ] && cp -a $TMPDIR/usr/lib64/* $ABSPATH/usr/lib64/ | |||
| check_modules_deps | |||
| TARGETVER=$($ABSPATH/usr/sbin/nginx -v 2>&1 | cut -d '(' -f 2 | cut -d ')' -f 1 | cut -d'-' -f 3 | tr -d 'r') | |||
| if [ $TARGETVER -ge 31 ]; then | |||
| if [ $TARGETVER -ge 33 ]; then | |||
| if ! $ABSPATH/usr/sbin/nginx -p $ABSPATH/etc/nginx -c nginx.conf -T 2>&1 | grep 'license_token' | grep -vE '^(.*)#.*license_token' >/dev/null; then | |||
There was a problem hiding this comment.
As I understand it, with this kind of check, I’ll end up with a broken conf if I already have mgmt{} block, say, from a previous ver? And will get somthing like:
mgmt {uuid_file foo;}
mgmt {license_token bar;}
There was a problem hiding this comment.
Hmm, yes, upgrade from R31/R32 to R33 will be broken if the configuration already has an mgmt block. I'll fix it.
There was a problem hiding this comment.
I can’t commit directly to this PR, so I sent you these changes for review in your fork.
NGINX Plus R33 introduces a need to place license_token on a filesystem. Those changes make sure the file is propagated to the installation directory.