RFE: Friendlier configuration of ns-slapd listen addresses

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49307

• Created at 2017-06-29 21:11:02 by merlinthp
• Assigned to nobody

---

ns-slapd will listen on all interfaces by default, or whatever is specified in nsslapd-listenhost and nsslapd-securelistenhost. These attributes can be used to configure ns-slapd to listen on a subset of interfaces if the value is a name that getaddrinfo resolves to multiple addresses (e.g. by having multiple entries in /etc/hosts for the same name). I'd say that that's a bit of a non-obvious and unusual way of doing things these days.

It would be nice if there was a different way to configure this, maybe by making the nsslapd-*listenhost attributes multi-value, or allowing them to take a comma separated list of values.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-06-30 01:51:48

I think this is a really good idea. I think the only risk I see is that we have to maintain the configurations of existing configs. So here is how I would this approached.

First is that we make this a multivalue attribute like you say, and we can show that it works correctly.

The next is that if any of the value lines have "host, host", we do an internal replace to make it two lines. This way we update the configuration on upgrade.

And finally, we need test cases that shows this works correctly.

How does that sound?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-06-30 01:51:49

Metadata Update from @Firstyear:

• Custom field type adjusted to defect
• Issue set to the milestone: 1.4 backlog

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-06-30 10:36:12

Indeed it is a good idea. Now I do not like that much the idea of comma separated interfaces.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-06-30 11:07:08

Indeed it is a good idea. Now I do not like that much the idea of comma separated interfaces.

I agree. And I don't see the need of the step with a list. At the point we support multiple values, we can do it with multivalued attributes. And if a deployment wants to make use of it, the config has to be changed anyway

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-07-01 04:05:09

If we wanted to be particularly strict on this, we could make a syntax error to have a , in the list and prevent start up along with a message explaining the reason?

[389-ds-bot]

Comment from merlinthp at 2017-07-01 13:27:23

OK then, as a starter for discusson, how about this?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-07-03 05:20:59

Great start. Some comments:

• I don't think we can assume we have access to change localhost on the test env. I often run these test in containers, so this may not work there. I'm wondering if there is a better way ....

• You use check_connect, but from the topo.standalone object you can call standalone.openConnection(connOnly=True, ....). Look at lib389/init.py line 1000 and 523.

• Don't hardcode the port numbers. You can use PORT_STANDALONE1 and HOST_STANDALONE1 for these.

• We should test more scenarios like
  ** No listen host
  ** multiple list hosts in multi valued attr
  ** comma seperated. (Do we want to continue to support this? )
  ** ipv6 and ipv4? Maybe LL addrs.

• daemon.c line 2910, don't use bare int. which leads to your loop around 2932, you should do:

for (size_t i = 0; ....)

Arrays in C are always indexed by size_t, and you should try and use the counters in the forloop init if possible to prevent shadow / value reuse.

• Same about int addrcnt, and int k - never use int in a C program :) it's 2017, we have inttypes.h. In this case k should be size_t and init in the forloop the same way, addrcnt should be uint64_t (or uint_fast32_t)
• Rather that memsetting netaddr each iteration, just call calloc each iteration. This can show us memory leaks, use after free etc with address sanitiser. Re-using memory like this breaks those assertions. Additionally, calloc of a new memory region may be faster than memset(0). It may also prevent the need to realloc.

I think it's a really good start, thank you!

I think that if I was to do one thing it would be to leave the parsing code for the comma separated values in place. Then when you set these in the config, and dse.ldif is written, it'll be turned into a multivalue list anyway. So that would be really good to test as well. This way we have a seamless upgrade path for users.

Thanks again for your work!

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-07-03 05:24:53

As some light side reading, https://matt.sh/howto-c and http://www.port389.org/docs/389ds/development/coding-style.html

Thanks so much!

[389-ds-bot]

Comment from merlinthp at 2017-07-03 10:16:36

Looking at the current code, I don't see anything that handles a comma separated list of names. A quick test with master shows that it fails.

Thanks for the pointers for the C coding style stuff. I automatically try to match the style of the code I'm making changes to, which isn't what we want in this case :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-07-04 02:52:02

It's hard to follow our style because there are "so many styles" over time. I am trying to ask for a higher "baseline" from new code, and touching up old locations, so that future work becomes easier. Thanks for accepting my feedback on this,

Okay, if the current code doesn't even use a comma sep list, then I think we have very few hurdles to this change. It seems pretty painless.

Thanks again,

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-10-18 21:35:07

Metadata Update from @mreynolds389:

• Custom field component adjusted to None
• Custom field origin adjusted to None
• Custom field reviewstatus adjusted to None
• Custom field version adjusted to None
• Issue tagged with: RFE