Re: [PATCH] Extend tcp_outgoing_address for route selection

On 08/30/2013 09:56 AM, Amos Jeffries wrote:
> Looking into bug 3901 if we skip past the broken assumptions and drop
> half of the proposed patch...
>
> The root cause of this report is that we have no way to selectively
> per-IP determine that the outgoing connection should not be made.

It would help if squid.conf documentation explained what will happen
when a "none" rule matches (in addition or instead of saying what will
_not_ happen).

> + These directives are tested individually for each upstream
> + destination IP address.

> + none Do not use the destination IP being
> + checked for.

> Processing proceeds in the order specified, and stops at first fully
> matching line.

The combination of the new "none" action and the old squid.conf "first
match" principle quoted above may be [incorrectly] interpreted as if
after "none" matches, no outgoing address will be used, and Squid will
refuse to forward the request. The new "for each destination" text
attempts to clarify that, but it is too far and too isolated to make
enough of a difference IMO.

I suggest a clarifying rewrite along the following lines:

------
When forwarding a request to the next hop, Squid often has a choice of
multiple destination IP addresses. For each candidate destination
address, Squid starts to match all configured tcp_outgoing_address
rules. If a rule matches, Squid selects the corresponding source IP for
the candidate destination address and restarts checking all rules for
the next destination candidate. If no rules match, the candidate
destination address is selected as if an "autoselect" rule matched.

If a rule with "none" source IP matches, Squid does not use the
destination address candidate at all. This can be used to prohibit
connections to some destination IP addresses (even if doing so has
nothing to do with source IP selection).

If all destination address candidates match a none rule (or if there are
no candidates to start with), Squid [...]. Otherwise, Squid connects to
the first selected destination address using the source IP it matched.
------

> + autoselect Let the operating system decide.
...
> + none Do not use the destination IP being
> + checked for.

I recommend s/autoselect/auto/ because it is shorter and because you
should otherwise rename "none" to "noneselect" to keep all names
consistent. Or, if you prefer, use the more explicit "os" instead of
"auto" in case we later add a source IP auto-selection algorithm to
Squid itself.

> + :: Let the operating system decide, but only
> + amongst IPv6 addresses.

Can we use [::] instead? It would make it easier for the future parser
to identify that as a valid IP address rather than a column column
sequence. I hope the current parser can parse [::] OK.

> The interesting part of the proposed bug fix is to allow
> tcp_outgoing_address selection produce an invalid output which causes
> the connection to be discarded from the set of upstream sources.

The problem here is the tcp_outgoing_address name/scope. The "none"
trick is really unrelated to the source IP selection. If we knew the
direction this is going, we could use something like this instead:

    next_hop deny acl ...
    next_hop allow acl ...
    next_hop src=... acl ...

This is not your fault, of course, and there may be no good way to fix
this now. I am just clarifying the problem in case you can come up with
a better documentation (or solution).

> - NOTE: The use of this directive using client dependent ACLs is
> - incompatible with the use of server side persistent connections. To
> - ensure correct results it is best to set server_persistent_connections
> - to off when using this directive in such configurations.

The above text was removed. Is it no longer true?

> === modified file 'src/peer_select.cc'
> // check for a configured outgoing address for this destination...
> getOutgoingAddress(psstate->request, p);
> - psstate->paths->push_back(p);
> + if (!p->local.isNoAddr())
> + psstate->paths->push_back(p);

What about all other uses of getOutgoingAddress()? What would happen if
"none" matches in other getOutgoingAddress() callers? A misleading error
or timeout? Here are the ones I could find quickly:

> FwdState.cc: getOutgoingAddress(request, p);
> adaptation/icap/Xaction.cc: getOutgoingAddress(NULL, connection);
> neighbors.cc: getOutgoingAddress(NULL, conn);
> peer_select.cc: getOutgoingAddress(psstate->request, p);
> peer_select.cc: getOutgoingAddress(psstate->request, p);

It feels like you should

1) Change getOutgoingAddress() profile to return true/false depending on
whether "none" matched. A "false" result would mean "do not connect at
all because a none rule matched".

2) Change all callers to handle the new "false" case explicitly.

Thank you,

Alex.
Received on Fri Aug 30 2013 - 22:24:20 MDT