Cleanup of nginx image #3214
Cleanup of nginx image #3214
Conversation
aledbf
added
the
do-not-merge/hold
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
size/M
images/nginx/rootfs/build.sh
Outdated
| @@ -95,6 +93,7 @@ clean-install \ | |||
| dumb-init \ | |||
| gdb \ | |||
| valgrind \ | |||
| luajit \ | |||
There was a problem hiding this comment.
why is this? we build luajit from source
There was a problem hiding this comment.
Because of this https://packages.debian.org/buster/luajit. I rollbacked this change
images/nginx/rootfs/build.sh
Outdated
| if [[ (${ARCH} != "ppc64le") && (${ARCH} != "s390x") ]]; then | ||
| cd "$BUILD_PATH/luajit2-2.1-20180420" | ||
| make CCDEBUG=-g | ||
| make install |
There was a problem hiding this comment.
related to https://github.com/kubernetes/ingress-nginx/pull/3214/files#r223915810, why are you removing this?
| REGISTRY ?= quay.io/kubernetes-ingress-controller | ||
| ARCH ?= $(shell go env GOARCH) | ||
| DOCKER ?= docker | ||
|
|
||
| ALL_ARCH = amd64 arm arm64 ppc64le s390x | ||
| ALL_ARCH = amd64 arm arm64 ppc64le |
There was a problem hiding this comment.
I saw openresty/lua-nginx-module#1379 today - that tells me ingress-nginx isn't going to work on ARM64 because lua-nginx-module does not support it yet.
There was a problem hiding this comment.
there's also a user reported issue about this #2802
k8s-ci-robot
added
size/L
|
/lgtm for the sake of documentation: this PR is also upgrading openresty/luajit to |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
aledbf
removed
the
do-not-merge/hold
|
Merging. |
| --add-module=$BUILD_PATH/nginx-http-auth-digest-$NGINX_DIGEST_AUTH \ | ||
| --add-module=$BUILD_PATH/ngx_http_substitutions_filter_module-$NGINX_SUBSTITUTIONS \ | ||
| --add-module=$BUILD_PATH/lua-nginx-module-$LUA_NGX_VERSION \ | ||
| --add-module=$BUILD_PATH/lua-upstream-nginx-module-$LUA_UPSTREAM_VERSION \ | ||
| --add-module=$BUILD_PATH/nginx_cookie_flag_module-$COOKIE_FLAG_VERSION \ |
There was a problem hiding this comment.
@aledbf why was this module removed? is there an alternative other than build and maintain a new image myself?
There was a problem hiding this comment.
Since dynamic mode only, this module was not used by the ingress controller
There was a problem hiding this comment.
Not by the controller itself but it is useful to have this module and use it under nginx.ingress.kubernetes.io/configuration-snippet: to add secure and httponly flags to certain cookies from 3rd party apps.
Any chance to see this back in the image?
There was a problem hiding this comment.
Any chance to see this back in the image?
No.
We already have the foundation to add custom plugins to the controller without the need to change the base nginx image. Please check #3807 and as an example #3967
Once this is available, you could build a plugin using https://github.com/cloudflare/lua-resty-cookie
I am sorry if this is not the answer you are looking for, but we are trying to allow custom features without the need of a custom docker image or the overhead we have maintaining a base image with all the nginx modules.
There was a problem hiding this comment.
I am also exploring alternatives to the current base image using multi-stage builds, like https://github.com/aledbf/horus-proxy/blob/master/Dockerfile to produce dynamic modules using openresty as the base image.
No description provided.