Commit Graph

119 Commits

Author SHA1 Message Date
Christopher Faulet
d83a6df5cd BUG/MEDIUM: resolvers: Skip DNS resolution at startup if SRV resolution is set
At startup, if a SRV resolution is set for a server, no DNS resolution is
created. We must wait the first SRV resolution to know if it must be
triggered. It is important to do so for two reasons.

First, during a "classical" startup, a server based on a SRV resolution has
no hostname. Thus the created DNS resolution is useless. Best waiting the
first SRV resolution. It is not really a bug at this stage, it is just
useless.

Second, in the same situation, if the server state is loaded from a file,
its hosname will be set a bit later. Thus, if there is no additionnal record
for this server, because there is already a DNS resolution, it inhibits any
new DNS resolution. But there is no hostname attached to the existing DNS
resolution. So no resolution is performed at all for this server.

To avoid any problem, it is fairly easier to handle this special case during
startup. But this means we must be prepared to have no "resolv_requester"
field for a server at runtime.

This patch must be backported as far as 2.2.
2021-03-12 17:41:28 +01:00
Christopher Faulet
0efc0993ec BUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks
Another way to say it: "Safely unlink requester from a requester callbacks".

Requester callbacks must never try to unlink a requester from a resolution, for
the current requester or another one. First, these callback functions are called
in a loop on a request list, not necessarily safe. Thus unlink resolution at
this place, may be unsafe. And it is useless to try to make these loops safe
because, all this stuff is placed in a loop on a resolution list. Unlink a
requester may lead to release a resolution if it is the last requester.

However, the unkink is necessary because we cannot reset the server state
(hostname and IP) with some pending DNS resolution on it. So, to workaround
this issue, we introduce the "safe" unlink. It is only performed from a
requester callback. In this case, the unlink function never releases the
resolution, it only reset it if necessary. And when a resolution is found
with an empty requester list, it is released.

This patch depends on the following commits :

 * MINOR: resolvers: Purge answer items when a SRV resolution triggers an error
 * MINOR: resolvers: Use a function to remove answers attached to a resolution
 * MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
 * MINOR: resolvers: Add function to change the srv status based on SRV resolution

All the series must be backported as far as 2.2. It fixes a regression
introduced by the commit b4badf720 ("BUG/MINOR: resolvers: new callback to
properly handle SRV record errors").

don't release resolution from requester cb
2021-03-12 17:41:28 +01:00
Christopher Faulet
6b117aed49 MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
When the server status must be updated from the result of a SRV resolution,
we can directly call srvrq_update_srv_state(). It is simpler and this avoid
a test on the server DNS resolution.

This patch is mandatory for the next commit. It also rely on "MINOR:
resolvers: Directly call srvrq_update_srv_state() when possible".
2021-03-12 17:41:28 +01:00
Christopher Faulet
1dec5c7934 MINOR: resolvers: Use a function to remove answers attached to a resolution
resolv_purge_resolution_answer_records() must be used to removed all answers
attached to a resolution. For now, it is only used when a resolution is
released.
2021-03-12 17:41:28 +01:00
Christopher Faulet
3e0600fbbf BUG/MEDIUM: resolvers: Trigger a DNS resolution if an ADD item is obsolete
When a ADD item attached to a SRV item is removed because it is obsolete, we
must trigger a DNS resolution to be sure the hostname still resolves or
not. There is no other way to be the entry is still valid. And we cannot set
the server in RMAINT immediatly, because a DNS server may be inconsitent and
may stop to add some additionnal records.

The opposite is also true. If a valid ADD item is still attached to a SRV
item, any DNS resolution must be stopped. There is no reason to perform
extra resolution in this case.

This patch must be backported as far as 2.2.
2021-03-12 17:41:28 +01:00
Baptiste Assmann
6a8d11dc80 MINOR: resolvers: new function find_srvrq_answer_record()
This function search for a SRV answer item associated to a requester
whose type is server.
This is mainly useful to "link" a server to its SRV record when no
additional record were found to configure the IP address.

This patch is required by a bug fix.
2021-03-12 17:41:28 +01:00
Christopher Faulet
77f860699c BUG/MEDIUM: resolvers: Fix the loop looking for an existing ADD item
For each ADD item found in a SRV response, we try to find a corresponding
ADD item already attached to an existing SRV item. If found, the ADD
last_seen time is updated, otherwise we try to find a SRV item with no ADD
to attached the new one.

However, the loop is buggy. Instead of comparing 2 ADD items, it compares
the new ADD item with the SRV item. Because of this bug, we are unable to
renew last_seen time of existing ADD.

This patch must be backported as far as 2.2.
2021-03-12 17:41:24 +01:00
Christopher Faulet
ab177ac1f3 BUG/MEDIUM: resolvers: Don't set an address-less server as UP
when a server status is updated based on a SRV item, it is always set to UP,
regardless it has an IP address defined or not. For instance, if only a SRV
item is received, with no additional record, only the server hostname is
defined. We must wait to have an IP address to set the server as UP.

This patch must be backported as far as 2.2.
2021-03-12 16:43:37 +01:00
Christopher Faulet
bca680ba90 BUG/MINOR: resolvers: Unlink DNS resolution to set RMAINT on SRV resolution
When a server is set in RMAINT becaues of a SRV resolution failure, the
server DNS resolution, if any, must be unlink first. It is mandatory to
handle the change in the context of a SRV resolution.

This patch must be backported as far as 2.2.
2021-03-12 16:43:37 +01:00
Christopher Faulet
5037c06d91 Revert "BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record"
This reverts commit a331a1e8eb.

This commit fixes a real bug, but it also reveals some hidden bugs, mostly
because of some design issues. Thus, in itself, it create more problem than
it solves. So revert it for now. All known bugs will be addressed in next
commits.

This patch should be backported as far as 2.2.
2021-03-12 16:43:37 +01:00
Willy Tarreau
144f84a09d MEDIUM: task: extend the state field to 32 bits
It's been too short for quite a while now and is now full. It's still
time to extend it to 32-bits since we have room for this without
wasting any space, so we now gained 16 new bits for future flags.

The values were not reassigned just in case there would be a few
hidden u16 or short somewhere in which these flags are placed (as
it used to be the case with stream->pending_events).

The patch is tagged MEDIUM because this required to update the task's
process() prototype to use an int instead of a short, that's quite a
bunch of places.
2021-03-05 08:30:08 +01:00
Willy Tarreau
61cfdf4fd8 CLEANUP: tree-wide: replace free(x);x=NULL with ha_free(&x)
This makes the code more readable and less prone to copy-paste errors.
In addition, it allows to place some __builtin_constant_p() predicates
to trigger a link-time error in case the compiler knows that the freed
area is constant. It will also produce compile-time error if trying to
free something that is not a regular pointer (e.g. a function).

The DEBUG_MEM_STATS macro now also defines an instance for ha_free()
so that all these calls can be checked.

178 occurrences were converted. The vast majority of them were handled
by the following Coccinelle script, some slightly refined to better deal
with "&*x" or with long lines:

  @ rule @
  expression E;
  @@
  - free(E);
  - E = NULL;
  + ha_free(&E);

It was verified that the resulting code is the same, more or less a
handful of cases where the compiler optimized slightly differently
the temporary variable that holds the copy of the pointer.

A non-negligible amount of {free(str);str=NULL;str_len=0;} are still
present in the config part (mostly header names in proxies). These
ones should also be cleaned for the same reasons, and probably be
turned into ist strings.
2021-02-26 21:21:09 +01:00
Christopher Faulet
69beaa91d5 REORG: server: Export and rename some functions updating server info
Some static functions are now exported and renamed to follow the same
pattern of other exported functions. Here is the list :

 * update_server_fqdn: Renamed to srv_update_fqdn and exported
 * update_server_check_addr_port: renamed to srv_update_check_addr_port and exported
 * update_server_agent_addr_port: renamed to srv_update_agent_addr_port and exported
 * update_server_addr: renamed to srv_update_addr
 * update_server_addr_potr: renamed to srv_update_addr_port
 * srv_prepare_for_resolution: exported

This change is mandatory to move all functions dealing with the server-state
files in a separate file.
2021-02-25 10:02:39 +01:00
Christopher Faulet
52d4d30109 BUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records
When a SRV record expires, the ip/port assigned to the associated server are
now removed. Otherwise, the server is stopped but keeps its ip/port while
the server hostname is removed. It is confusing when the servers state are
retrieve on the CLI and may be a problem if saved in a server-state
file. Because the reload may fail because of this inconsistency.

Here is an example:

 * Declare a server template in a backend, using the resolver <dns>

server-template test 2 _http._tcp.example.com resolvers dns check

 * 2 SRV records are announced with the corresponding additional
   records. Thus, 2 servers are filled. Here is the "show servers state"
   output :

2 frt 1 test1 192.168.1.1 2 64 0 1 2 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 2 64 0 1 1 15 3 4 6 0 0 0 http2.example.com 8002 _http._tcp.example.com 0 0 - - 0

 * Then, one additional record is removed (or a SRV record is removed, the
   result is the same). Here is the new "show servers state" output :

2 frt 1 test1 192.168.1.1 2 64 0 1 38 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 0 96 0 1 19 15 3 0 14 0 0 0 - 8002 _http._tcp.example.com 0 0 - - 0

On reload, if a server-state file is used, this leads to undefined behaviors
depending on the configuration.

This patch should be backported as far as 2.0.
2021-02-24 21:58:45 +01:00
Baptiste Assmann
b4badf720c BUG/MINOR: resolvers: new callback to properly handle SRV record errors
When a SRV record was created, it used to register the regular server name
resolution callbacks. That said, SRV records and regular server name
resolution don't work the same way, furthermore on error management.

This patch introduces a new call back to manage DNS errors related to
the SRV queries.

this fixes github issue #50.

Backport status: 2.3, 2.2, 2.1, 2.0
2021-02-24 21:58:45 +01:00
Christopher Faulet
a331a1e8eb BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record
If no additional record is associated to a SRV record, its TTL must not be
renewed. Otherwise the entry never expires. Thus once announced a first
time, the entry remains blocked on the same IP/port except if a new announce
replaces the old one.

Now, the TTL is updated if a SRV record is received while a matching
existing one is found with an additional record or when an new additional
record is assigned to an existing SRV record.

This patch should be backported as far as 2.2.
2021-02-24 21:58:45 +01:00
Christopher Faulet
9c246a4b6c BUG/MINOR: resolvers: Fix condition to release received ARs if not assigned
At the end of resolv_validate_dns_response(), if a received additionnal
record is not assigned to an existing server record, it is released. But the
condition to do so is buggy. If "answer_record" (the received AR) is not
assigned, "tmp_record" is not a valid record object. It is just a dummy
record "representing" the head of the record list.

Now, the condition is far cleaner. This patch must be backported as far as
2.2.
2021-02-24 21:58:45 +01:00
Emeric Brun
56fc5d9ebc MEDIUM: resolvers: add supports of TCP nameservers in resolvers.
This patch introduce the new line "server" to set a TCP
nameserver in a "resolvers" section:

server <name> <address> [param*]
  Used to configure a DNS TCP or stream server. This supports for all
  "server" parameters found in 5.2 paragraph. Some of these parameters
  are irrelevant for DNS resolving. Note: currently 4 queries are pipelined
  on the same connections. A batch of idle connections are removed every
  5 seconds. "maxconn" can be configured to limit the amount of those
  concurrent connections and TLS should also usable if the server supports
. The current implementation limits to 4 pipelined

The name of the line in configuration is open to discussion
and could be changed before the next release.
2021-02-13 10:03:46 +01:00
Emeric Brun
c943799c86 MEDIUM: resolvers/dns: split dns.c into dns.c and resolvers.c
This patch splits current dns.c into two files:

The first dns.c contains code related to DNS message exchange over UDP
and in future other TCP. We try to remove depencies to resolving
to make it usable by other stuff as DNS load balancing.

The new resolvers.c inherit of the code specific to the actual
resolvers.

Note:
It was really difficult to obtain a clean diff dur to the amount
of moved code.

Note2:
Counters and stuff related to stats is not cleany separated because
currently counters for both layers are merged and hard to separate
for now.
2021-02-13 10:03:46 +01:00