feat(stats): refactor global stats sockets

[tacerus]

PR progress checklist (to be filled in by reviewers)

• Changes to documentation are appropriate (or tick if not required)
• Changes to tests are appropriate (or tick if not required)
• Reviews completed

---

What type of PR is this?

Primary type

• [build] Changes related to the build system
• [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ci] Changes to the continuous integration configuration
• [feat] A new feature
• [fix] A bug fix
• [perf] A code change that improves performance
• [refactor] A code change that neither fixes a bug nor adds a feature
• [revert] A change used to revert a previous commit
• [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [docs] Documentation changes
• [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No. Yes, as discussed in the comments, it was desired to change the existing pillar instead of introducing a new one.

Related issues and/or pull requests

n/a

Describe the changes you're proposing

This is useful to implement sockets with different access levels. The existing stats pillar is left in tact.

Pillar / config required to test the proposed changes

See changes in pillar.example.

Debug log showing how the proposed changes work

Documentation checklist

• Updated the README (e.g. Available states).
• Updated pillar.example.

Testing checklist

• Included in Kitchen (i.e. under state_top).
• Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• Updated the relevant test pillar.

Additional context

[n-rodriguez]

LGTM

[daks]

At the moment I can't test your code (some issue with Ruby install with rvm) but looking at it, I see there are already a number of stats dicts available, you can see them in pillar.example, can't those' be extended to support multiple ones?
I think it would be better than adding a new configuration pillar. The previous code is quite old so it may need to be changed.

[tacerus]

Hi @daks,

I'm sorry, but I do not see how the existing stats dictionary allows me to configure multiple stats sockets in the global section.

[daks]

Hi @tacerus no the current implementation does not allow it in fact :)
My proposal was to refactor it to allow them, to keep only one stats pillar, instead of adding a new stats_sockets one.

It's a little bit more work I know but I think it's worth it.

Ideally, default behaviour would keep the current behaviour. If it's really not possible, a BREAKING CHANGE would need to be introducted (in commit msg).

Don't hesitate to ask questions if something is not clear

[tacerus]

I think it's less intrusive to keep both, but to mark the existing stats one as deprecated. Then, in a future formula release, one can drop the stats one as a breaking change. This gives users time to switch.

[n-rodriguez]

My proposal was to refactor it to allow them, to keep only one stats pillar, instead of adding a new stats_sockets one.

It's a little bit more work I know but I think it's worth it.

Who's gonna do this work? you? I'm pretty sure you won't, so let's merge this as is.

[daks]

I think it's less intrusive to keep both, but to mark the existing stats one as deprecated. Then, in a future formula release, one can drop the stats one as a breaking change. This gives users time to switch.

It could in fact be a viable solution, but I have no idea how to 'communicate' this deprecation with the current tools for formulas. I think it must be at least indicated in pillar.example and with a dedicated commit so it appears clearly in the CHANGELOG.

I also fear the second modification will never be done (there is no real maintainers on most of the formulas) and the formula will stay with the two configuration options, but if you plan to do it yourself, I'm OK with it :)

[tacerus]

OK, I amended the patch to instead refactor the existing haproxy:global:stats pillar.

[sylvainfaivre]

LGTM

[daks]

@tacerus thanks for the change. Two things though:

• I managed to run Kitchen and there is a problem running them on the default-debian-11-master-py3 image: the service does not start with your code (the problem does not exist on the master branch)

Mar 07 14:25:21 1e2a676970e6 haproxy[556]: [ALERT] 066/142521 (556) : parsing [/etc/haproxy/haproxy.cfg:20] : 'stats socket' : ''group' : unknown group name 'wheel''
Mar 07 14:25:21 1e2a676970e6 haproxy[556]: [ALERT] 066/142521 (556) : Error(s) found in configuration file : /etc/haproxy/haproxy.cfg
Mar 07 14:25:21 1e2a676970e6 haproxy[556]: [ALERT] 066/142521 (556) : Fatal errors found in configuration.
Mar 07 14:25:21 1e2a676970e6 systemd[1]: haproxy.service: Control process exited, code=exited, status=1/FAILURE

• because the implementation is now changed, you will need to indicate the BREAKING CHANGE to generate the appropriate changelog and number version (based on semver). It used to exist a doc indicating how to do it but I just don't find it right now

[tacerus]

• If "wheel" does not exist on Debian, which similar group does exist in all the distribution containers which can be used in the testing pillar?

• I did indicate the breaking change in the commit message. I don't know about such documentation either.

[tacerus]

Inspecting the groups in

saltimages/salt-master-py3:debian-11

$ podman run --rm -it saltimages/salt-master-py3:debian-11
root@7e9c5151bd1c:/# more /etc/group
root:x:0:
daemon:x:1:
bin:x:2:
sys:x:3:
adm:x:4:
tty:x:5:
disk:x:6:
lp:x:7:
mail:x:8:
news:x:9:
uucp:x:10:
man:x:12:
proxy:x:13:
kmem:x:15:
dialout:x:20:
fax:x:21:
voice:x:22:
cdrom:x:24:
floppy:x:25:
tape:x:26:
sudo:x:27:
audio:x:29:
dip:x:30:
www-data:x:33:
backup:x:34:
operator:x:37:
list:x:38:
irc:x:39:
src:x:40:
gnats:x:41:
shadow:x:42:
utmp:x:43:
video:x:44:
sasl:x:45:
plugdev:x:46:
staff:x:50:
games:x:60:
users:x:100:
nogroup:x:65534:
systemd-journal:x:101:
systemd-network:x:102:
systemd-resolve:x:103:
input:x:104:
kvm:x:105:
render:x:106:
ssh:x:107:

it seems "users" is the next best thing. I amended it respectively.

[tacerus]

I can manually write a changelog line, but why are there both CHANGELOG.md and docs/CHANGELOG.rst?

[dafyddj]

No manual changelog edits are needed:
https://github.com/saltstack-formulas/.github/blob/master/CONTRIBUTING.rst

[tacerus]

Thanks for the link, @dafyddj! I adjusted the commit message to match the described format.

[daks]

• If "wheel" does not exist on Debian, which similar group does exist in all the distribution containers which can be used in the testing pillar?

  • I did indicate the breaking change in the commit message. I don't know about such documentation either.

You could look at what was the current one in master. I can't help you on that before a few days.
If this is only a problem of test data (because it must be specified in the pillar) then it's not really a problem [*] because each user will make it appropriate, but if it's added by your code, it must be right.

[*] actually, ideally it should also be right even for testing, and for all platforms but I'm not sure the CI/tests are functional right now

[daks]

No manual changelog edits are needed: https://github.com/saltstack-formulas/.github/blob/master/CONTRIBUTING.rst

That is the link I was searching before, thanks :)

[tacerus]

As stated I already replaced the group.

[n-rodriguez]

@daks

because the implementation is now changed,

it should have not changed. it should have been done in a separate PR.

that's not fair/nice :

• to put the burden of technical debt on contributors (the purpose of this PR was to fix something, not to refactor code, thank you)
• to put the burden of technical debt on formula users (now they will have to cope with a breaking change, thank you)
• to ask people to do something you wouldn't have done yourself

[daks]

@tacerus thanks. A basic kitchen test on default-debian-11-master-py3 passes now. I won't do any further tests/evaluation of the code which is OK for me like this.

I'll merge in a few days.

[daks]

@n-rodriguez

it should have not changed. it should have been done in a separate PR.

This is your opinion. Did you plan to do it yourself? (as you asked me before)

that's not fair/nice :

* to put the burden of technical debt on contributors (the purpose of this PR was to fix something, not to refactor code, thank you)

This is not a fix, it's a feature implementation.
The conversation has been cordial and constructive with the contributor.

And I don't see any technical debt here. (apart the test/CI one)

* to put the burden of technical debt on formula users (now they will have to cope with a breaking change, thank you)

Formula management tooling is not sufficient, this problem exists since I got involved in using and developing formulas (and I several times raised this problem in the community) several years ago. It has since improved quite a bit with the versioning and breaking change tooling. Even if it's not sufficient this is the one we have right now so I use it. (If tools and ways of functioning have changed recently, I'm not aware and docs don't seem to reflect that)

Another identified problem is the lack of maintainers (and what I could call 'ownership').

If you want to contribute, don't hesitate to get in touch with the Working Group (for example on slack).

* to ask people to do something you wouldn't have done yourself

You don't know what I would have done myself so please avoid imagining it.

To conclude: I don't understand why you are so aggressively (with your comments and multiple thumbs down) intervening here but please stop.

[n-rodriguez]

To conclude: I don't understand why you are so aggressively (with your comments and multiple thumbs down) intervening here but please stop.

Do you mean formula developers must absolutely agree with all your opinions?

[n-rodriguez]

This is your opinion. Did you plan to do it yourself? (as you asked me before)

Nope, I no longer use this formula, and you?

[daks]

To conclude: I don't understand why you are so aggressively (with your comments and multiple thumbs down) intervening here but please stop.

Do you mean formula developers must absolutely agree with all your opinions?

No, but look at the way you express your disagreement.

[daks]

Thanks @tacerus for your contribution

[tacerus]

Thanks as well! :)

[SuperTux88]

@tacerus: This now doesn't allow to set extra bind parameter anymore, as the new structure dropped the extra field.

[tacerus]

Hi @SuperTux88, that is true - I only facilitated what is described here:

https://docs.haproxy.org/2.4/management.html#9.3

Which options are you missing? Are you trying to specify additional bind parameters for stats listeners which are not Unix sockets?

[SuperTux88]

For the -x <unix_socket> parameter to work, you need to add expose-fd listeners to the stats socket:

The capability must be enable on the stats socket using "expose-fd listeners" in your configuration.

https://docs.haproxy.org/2.4/management.html#3-where (at the bottom of the options list)

[tacerus]

Interesting, thanks for the pointer! Since this is just one option which doesn't take any parameters (https://docs.haproxy.org/2.4/configuration.html#5.1-expose-fd%20listeners) I suggest to expose it using a boolean toggle in the pillar under the stats sockets. What do you think?

[SuperTux88]

I liked the extra field, it's most flexible and it's also more future proof in case some new options show up in a future version or maybe there is even already another option which we both don't know yet. And extra is also already used in various other places in this formula.

But a boolean works too for my use-case.

[tacerus]

I understand. I think it's good to have options natively exposed where possible as it makes the pillar easily parseable for other uses. In this case it should simple enough to introduce additional options if needed in the future. But open to other feedback.

#92

[SuperTux88]

I see the advantage of specific keys in the case of for example user: haproxy instead of extra: user haproxy. But in the case of a boolean, I personally like extra: expose-fd listeners more over expose-fd listeners: true and I also find it less confusing with the extra (but that might be a personal thing).

[tacerus]

I get what you mean, maybe it makes sense for this. Let's see what others think.