docker-library / haproxy

Docker Official Image packaging for HAProxy
http://www.haproxy.org/
GNU General Public License v2.0
347 stars 158 forks source link

Use Lua5.4 as default version when building haproxy 2.9+ #215

Closed Darlelet closed 7 months ago

Darlelet commented 1 year ago

Following @TimWolla suggestion in https://github.com/docker-library/haproxy/commit/f389054a408dd4a1cb6f314fd66a851f565190d3#r119087804:

Encouraging Lua 5.4 adoption by building newer haproxy versions with the lua5.4-dev package.

Versions prior to 2.9 will still be built with the lua5.3-dev package to prevent any breakage.

(only Dockerfile.template was modified, and then ./update.sh was used to update Dockerfiles)

tianon commented 7 months ago

I don't think I'm worried too much about "minor incompatibilities" in general here (usage of deprecated/EOL software concerns me a lot, on the other hand).

I do worry that I don't understand enough about how users typically use the Lua functionality of HAProxy and whether they'd be very likely to experience breakage (I'm guessing they wouldn't, but again, I don't actually know at all). Ideally, we'd have something from the HAProxy upstream project saying "we recommend version X.Y of Lua" but I don't think they have any such specific recommendation. :disappointed:

Darlelet commented 7 months ago

Hmm yes I understand your point of view. Sorry to confuse you, let me try to clarify mine so that you can take the proper decision :smile:

how users typically use the Lua functionality of HAProxy and whether they'd be very likely to experience breakage

Lua support within haproxy allows users to extend the core functionality by loading custom-made lua scripts defining custom processing/helper functions that haproxy can use during runtime according to haproxy configuration.

Ideally, we'd have something from the HAProxy upstream project saying "we recommend version X.Y of Lua

Indeed, there is no official position on that specific point. But Lua 5.4 is officially supported within haproxy so it should be considered as the recommended version for new setups indeed.

What I was worried about existing docker images: since Lua scripts are custom made, they are not provided by haproxy, but by third-parties or users themselves. As such, due to minor incompatibilities that can be encountered when upgrading from 5.3 to 5.4 I was worried that some old existing scripts might not be working as before, without the user being able to explain it or find support for it (considering the user picked up the script from an article or a github repo, written by someone else at the time 5.3 was the default, but doesn't maintain it anymore).

To sum up: haproxy 2.4+ integration with lua 5.4 itself is fine and should be the default for new setups. But I was a bit concerned about older versions where users might still be deploying haproxy with unmaintained Lua scripts that could stop working properly if enforcing Lua interpereter to 5.4.

Perhaps we could start by enforcing Lua 5.4 for haproxy 2.9+, and then try to slowly introduce 5.4 for older versions as well (up to haproxy 2.4), while considering revert as an option (for 2.4 .. 2.8) if too many users complain that their existing Lua scripts suddenly stopped working and they are not able to fix the script themselves? Or provide 2 separate haproxy binaries within the image? One for Lua 5.3 support, and the other one for 5.4?

With that said, Lua 5.4 is mainly backward-compatible with 5.3 so hopefully this should not be an issue for the vast majority of existing scripts in the wild, I'm just being extra cautious (maybe too much). FYI, haproxytech docker images already enforce Lua 5.4 for haproxy 2.4+ (https://github.com/haproxytech/haproxy-docker-ubuntu/commit/392c51b48c600d94449d63b5c26d9e67c34c6e20)

Darlelet commented 7 months ago

I updated the PR so that it now makes use of version.sh to define the lua variable used in the template, instead of hard-coding the selection logic within the template file (taking this opportunity to expose the lua version in versions.json file as well)

Feel free to suggest or performs modifications to it (ie: apply the 5.4 default lua version to more haproxy versions)

Darlelet commented 7 months ago

Well, not sure committing version.json was a good move since every time the bot bumps versions.json there will be merge conflicts :smiling_face_with_tear:

Apart from that, do you think we are good this time @tianon? Or do you need an official position from haproxy? Thanks for your help

tianon commented 7 months ago

FYI, haproxytech docker images already enforce Lua 5.4 for haproxy 2.4+ (https://github.com/haproxytech/haproxy-docker-ubuntu/commit/392c51b48c600d94449d63b5c26d9e67c34c6e20)

This is really interesting IMO and a pretty good justification for us to do the same, but I'm OK with staying on 5.3 for now if we want to start conservative. :bow: :heart:

I think I'd rather not put this in versions.json since it's not actually something that we're scraping externally (or that will factor into the tags we generate in generate-stackbrew-library.sh, for example), so I think I'd rather just have a def lua or similar inside Dockerfile.template that defines it, especially since it might have to be different in Debian vs Alpine in the future (given we're relying on their Lua packages). :sweat_smile:

Maybe something like this, for now (and we can get more complex / opt more versions into our 5.4 default over time)?

def lua:
    if [ "2.0", "2.2", "2.4", "2.6", "2.7", "2.8" ] | index(env.version) then
        "5.3"
    else
        "5.4"
    end
;

(and then {{ .lua }} becomes {{ lua }})

I really appreciate you staying patient with me and helping me understand! Feel free to let me know if at any point here you'd like to punt and have me take over making the actual changes. :sweat_smile: :heart:

Darlelet commented 7 months ago

Ok great!

Oh yes that makes sense indeed, I reverted back to previous implementation as you suggested :smile:, we're all set!