Fix arm paths for liblua.so and lua_package_cpath

[toolboc]

What this PR does / why we need it:

This PR sets proper paths in ARM builds of ingress-nginx for liblua.so and lua_package_cpath.

Without this fix, nginx.tmpl is always invalid for ARM devices due to assumption of /usr/lib/x86_64-linux-gnu/lua/5.1/?.so; as a valid path regardless of platform architecture. Similarly, build.sh assumes that /usr/lib/x86_64-linux-gnu/liblua5.1.so always exists, but this is not true for all platforms.

Explanation:
nginx.tmpl is used to create the default nginx.conf for the nginx-ingress-controller. As-is, this file references lua packages in a hard-coded platform-specific path usr/lib/x86_64-linux-gnu/lua/5.1/?.so;. As a result, lua packages installed via clean-install were not being found on ARM builds as this path does not exist. On ARM, the search path for lua packages installed via clean-install should be usr/lib/rm-linux-gnueabihf/lua/5.1/?.so; .

This explains why cjson was already being installed via clean-install, but not found on ARM per commentary by @ElvinEfendi on #2588. The fix that was applied in #2588 installs lua-cjson in /usr/local/lib/lua/5.1 (via luarocks) instead of /usr/lib/linux-gnueabihf/lua/5.1 (via clean-install) which gets picked up in nginx.tmpl due to inclusion of the search path /usr/local/lib/lua/?.so;. The clean-install method was already sufficient but nginx.tmpl was improperly referencing one of the search paths. In summary, nginx.tmpl was the real culprit of #2547.

Which issue this PR fixes
Addresses ##2547

[codecov-io]

Codecov Report

Merging #2597 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2597      +/-   ##
==========================================
+ Coverage   40.66%   40.68%   +0.01%     
==========================================
  Files          75       75              
  Lines        5108     5108              
==========================================
+ Hits         2077     2078       +1     
+ Misses       2753     2751       -2     
- Partials      278      279       +1

Impacted Files	Coverage Δ
internal/watch/file_watcher.go	84.61% <0%> (+3.84%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54500d5...3159384. Read the comment docs.

[toolboc]

/assign @aledbf

[toolboc]

/assign @ElvinEfendi

[aledbf]

The nginx image does not provide any template. Please remove

[toolboc]

@aledbf , a template (nginx.tmpl) is provided into the nginx-ingress-controller image as part of this Makefile on line 122 during the build of the rootfs Dockerfile. You can verify the file exists by heading to /etc/nginx/template/nginx.tmpl within a container instance.

This template file is responsible for the default configuration of the nginx-ingress-controller and causes failures on ARM due to lua_package_cpath "/usr/local/lib/lua/?.so;/usr/lib/x86_64-linux-gnu/lua/5.1/?.so;;";; as the second path in this setting is not available on ARM images.

That said, this seems like the proper place to make the necessary modification as this is where the source is copied to a temp directory for building. There is a similar strategy already being employed for BASEIMAGE, QEMUARCH, and DUMB_ARCH in this Makefile.

Thoughts?

[antoineco]

@toolboc that's not what @aledbf meant (he wrote that code btw), he was talking about the base nginx image. Just a little confusion I guess.
That said, nginx.tmpl does indeed contain hardcoded paths to *.so files. Maybe creating symlinks after the installation of Lua would be more elegant than replacing paths at build time using sed?

[toolboc]

@antoineco , we could fix this with symlinks via something like:
ln -s /usr/lib/arm-linux-gnueabihf /usr/lib/x86_64-linux-gnu

or

we could symlink both of these paths to a new commonpath and adjust the template accordingly.

That said, I feel like this is just getting around the issue being caused by the template itself. Open to any suggestions and happy to follow up with an appropriate update.

/Please advise

[antoineco]

we could symlink both of these paths to a new commonpath and adjust the template accordingly

I like that one. Each container image is built for a specific architecture so using a specific path is not necessary.

Setting LUA_PATH and LUA_CPATH in the base nginx image is also something that would allow us to to skip the lua_package_path and lua_package_cpath directives (cf. docs). edit: @ElvinEfendi do we even need to configure anything? Aren't Lua's defaults sane enough?

[toolboc]

@antoineco , I have added a change which symlinks the appropriate paths for all platforms to a common lua-platform-path in ede1094

[toolboc]

@antoineco , It looks like the CI build is getting stuck on changeset ede1094 as it requires a republish of the base nginx image to account for the changes to build.sh which are now coupled to the modified nginx.tmpl. Making the fix in the Makefile would bypass this issue. This PR will not be able to move forward with the symlink strategy without an updated nginx image, bit of a chicken / egg problem.

[aledbf]

Making the fix in the Makefile would bypass this issue. This PR will not be able to move forward with the symlink strategy without an updated nginx image, bit of a chicken / egg problem.

That is why we always make this change in two steps (PRs):

• change something nginx image (after the PR is merged I trigger the build for new image)
• change the main Makefile

[toolboc]

@aledbf ,

Thank you for the explanation, I will make a new PR for the changes to the nginx image, then we can make the change to nginx.tmpl here after the new image is published.

Let me know if there are any issues with this approach.

[aledbf]

We need to find a way to fix this for all the platforms

[toolboc]

Do you have any suggestions which would avoid a cascade of conditional statements for each platform?

[aledbf]

We are not building armv71 https://github.com/toolboc/ingress-nginx/blob/ede1094f6d212b5f58d409eeed4d3c9b5747afdb/images/nginx/Makefile#L21

[toolboc]

@aledbf

build.sh is obtaining the platform using ARCH=$(uname -m) which returns 'armv71' in nginx-arm

Which reminds me, the value I supplied for arm64 is incorrect ;)

[aledbf]

@toolboc please rebase, remove the changes to the nginx image, squash the commits and update https://github.com/kubernetes/ingress-nginx/blob/master/Makefile#L62 to 0.49
After that your change in the template should pass travis-ci

[ElvinEfendi]

/lgtm

[aledbf]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, toolboc

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:

• OWNERS [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aledbf]

@toolboc thanks!