#49307 RFE: Friendlier configuration of ns-slapd listen addresses
Closed: wontfix 3 years ago by spichugi. Opened 6 years ago by merlinthp.

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.


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?

Metadata Update from @firstyear:
- Custom field type adjusted to defect
- Issue set to the milestone: 1.4 backlog

6 years ago

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

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

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?

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!

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 :)

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,

Metadata Update from @mreynolds:
- 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

6 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2366

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix
- Issue status updated to: Closed (was: Open)

3 years ago

Login to comment on this ticket.

Metadata