Hi Ben,
On Tue, Apr 24, 2018 at 09:47:01PM -0600, Ben Draut wrote:
> This patch implements the 'parse-resolv-conf' directive for the resolvers
> section, it was previously discussed on the list in this thread:
> https://www.mail-archive.com/[email protected]/msg29600.html
>
> (Is it preferred to use the discussion thread to present the patch?)
Not needed, that's fine this way, thanks.
> It's a little inefficient, in that it will re-parse /etc/resolv.conf every
> time the directive is encountered, but that didn't seem too bad to me since
> that file is (usually?) tiny. I'm happy to do an alternate approach if we
> think it's necessary.
I don't think it's a problem, at least for now. If someone starts to complain
maybe we can change this in the future. There would be a benefit in doing it
differently, which would be the ability to change it from the CLI. But this
would also require changes to the resolvers code itself so that it can rely
on a shared resolver between all sections. So I think that your approach is
fine for now.
> I also opted to just use the nameserver's address as its name in the
> resolvers section, as I thought that would have the highest probability of
> avoiding name conflicts with other configured nameservers. Again I'm open
> to feedback though.
I think you're right at this point. Please just check what happens when the
same IP address happens multiple times in the same file, as I've seen this
quite a few times in field. As long as it doesn't prevent the conf from
starting, it's OK.
I'm just having a few comments on the patch itself :
> @@ -2288,6 +2288,100 @@ int cfg_parse_resolvers(const char *file, int
> linenum, char **args, int kwm)
>
> newnameserver->addr = *sk;
> }
> + else if (strcmp(args[0], "parse-resolv-conf") == 0) {
I think you should register a config keyword and parse this in its own
function if at all possible, but I don't know if resolvers can use
registered config keywords, so if it's not possible, please ignore this
comment.
> + const char *whitespace = "\r\n\t ";
> + char *resolv_line = NULL;
> + int resolv_linenum = 0;
> + FILE *f;
> + char *address = NULL;
> + struct sockaddr_storage *sk;
> + struct protocol *proto;
> +
> + if ((resolv_line = malloc(sizeof(*resolv_line) * LINESIZE)) ==
> NULL) {
> + ha_alert("parsing [%s:%d] : out of memory.\n",
> + file, linenum);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> +
> + if ((f = fopen("/etc/resolv.conf", "r")) == NULL) {
> + free(resolv_line);
> + ha_alert("parsing [%s:%d] : failed to open
> /etc/resolv.conf.\n",
> + file, linenum);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> +
> +
> + sk = calloc(1, sizeof(*sk));
> + if (sk == NULL) {
> + ha_alert("parsing [/etc/resolv.conf:%d] : out of
> memory.\n", resolv_linenum);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
You have a memory leak above, resolv_line is not freed, and f is not closed.
In fact it's the same in each and every other error block below. You should
use a label in your code block for error processing which would systematically
close the file and release memory.
> + while (fgets(resolv_line, LINESIZE, f) != NULL) {
> + resolv_linenum++;
> + if (strncmp(resolv_line, "nameserver", 10) != 0)
> + continue;
> +
> + address = strtok(resolv_line + 10, whitespace);
> + if (address == NULL) {
> + ha_warning("parsing [/etc/resolv.conf:%d] :
> nameserver line is missing address.\n",
> + resolv_linenum);
> + err_code |= ERR_WARN;
> + continue;
> + }
Here you can accidently parse something else. For example if it's written
"nameservers", the address will be "s". I think you should add a test before
this one :
if (address == resolv_line + 10)
continue;
> + list_for_each_entry(newnameserver,
> &curr_resolvers->nameservers, list) {
> + if (strcmp(newnameserver->id, address) == 0) {
> + ha_alert("Parsing [/etc/resolv.conf:%d]
> : generated name for /etc/resolv.conf nameserver '%s' conflicts with another
> nameserver (declared at %s:%d).\n",
> + resolv_linenum, address,
> newnameserver->conf.file, newnameserver->conf.line);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + }
> + }
> +
> + if ((newnameserver = calloc(1, sizeof(*newnameserver)))
> == NULL) {
> + ha_alert("parsing [/etc/resolv.conf:%d] : out
> of memory.\n", resolv_linenum);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> +
> + LIST_ADDQ(&curr_resolvers->nameservers,
> &newnameserver->list);
> + newnameserver->resolvers = curr_resolvers;
> + newnameserver->conf.file = strdup("/etc/resolv.conf");
> + newnameserver->conf.line = resolv_linenum;
> + newnameserver->id = strdup(address);
> +
> + memset(sk, 0, sizeof(*sk));
> + sk = str2ip2(address, sk, 1);
> + if (!sk) {
> + ha_alert("parsing [/etc/resolv.conf:%d] :
> failed to populate sockaddr_storage for address '%s'\n",
> + resolv_linenum, address);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
Just out of curiosity, what could cause this error to happen ? I'm asking
because the alert doesn't make it easy for the end user to figure how to
fix the problem, thus we should adjust the message in this direction.
> + proto = protocol_by_family(sk->ss_family);
> + if (!proto || !proto->connect) {
> + ha_alert("parsing [/etc/resolv.conf:%d] : '%s'
> : connect() not supported for this address family.\n",
> + resolv_linenum, address);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
At this point, I'm realizing that we should probably just ignore such entries
and emit a warning : usually the admin will not have permissions to edit the
resolv.conf file, or sometimes will not want to take risks. And just due to
an issue in resolv.conf, we could prevent haproxy from starting at all.
> + set_host_port(sk, 53);
> + newnameserver->addr = *sk;
> + }
> +
> + free(sk);
> + free(resolv_line);
> + if (fclose(f) != 0) {
> + ha_warning("parsing [%s:%d] : failed to close handle to
> /etc/resolv.conf.\n",
> + file, linenum);
> + err_code |= ERR_WARN;
> + }
In practice you don't need to run this check on a read-only file, as it
cannot fail, and if it really did, the user couldn't do anything about
it anyway.
Thanks,
Willy
