Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

Awesome! Thanks everyone.

Thayne McCombs
Senior Software Engineer
tha...@lucidchart.com



Lucid.co 


On Thu, Dec 31, 2020 at 2:10 AM Willy Tarreau  wrote:

> On Tue, Dec 29, 2020 at 10:52:56AM +0100, Frederic Lecaille wrote:
> > > +#REQUIRE_VERSION=2.0
> >
> > Ok for this test but I think we should use 2.4 which is the current
> > developemnt version.
>
> OK I applied this change. Tested here and it works.
>
> > Willy, I think we can import this patch after having merged the previous
> one
> > [1/2] patch about this feature implementation.
>
> Now done.
>
> Thanks!
> Willy
>

Re: [PR] Add srvkey option to stick-table

[Willy Tarreau]

On Tue, Dec 29, 2020 at 10:52:56AM +0100, Frederic Lecaille wrote:
> > +#REQUIRE_VERSION=2.0
> 
> Ok for this test but I think we should use 2.4 which is the current
> developemnt version.

OK I applied this change. Tested here and it works.

> Willy, I think we can import this patch after having merged the previous one
> [1/2] patch about this feature implementation.

Now done.

Thanks!
Willy

Re: [PR] Add srvkey option to stick-table

[Willy Tarreau]

Hi guys,

On Tue, Dec 29, 2020 at 10:45:17AM +0100, Frederic Lecaille wrote:
> You work sounds perfect to me.

FWIW I agree that th patch looks particularly clean. I've not studied it
though, I trust you :-)

> I am not sure we will import the second patch for the reg test: it makes
> usage of curl. We try to not use external programs for reg tests except if
> there is no choice. This is not the case here.
> 
> Willy, could you have a look at the first patch, especially sa2str()
> implementation. I have doubt about its portability:
> 
>const int max_length = sizeof(struct sockaddr_un) - offsetof(struct
> sockaddr_un, sun_path) - 1;
>return memprintf(&out, "abns@%.*s", max_length, path+1);
> 
> 
> As far as I see, everywhere that unix socket are used in haproxy code, we
> consider ->sun_path as NULL terminated.

I think it's OK since "%.*" fixes the maximum length anyway so it will
always work. But you're right, we normally enforce the trailing zero,
so I guess that "abns@%s" would equally work. But there's nothing wrong
here anyway, and we don't have an portability issues since abns only
exists on linux, which limits the scope.

I'm merging the patch then. Thayne, I'll extend a little bit your
commit message to mention that the peers support was also implemented
(the text only mentions stick-tables).

Thanks to you both!
Willy

Re: [PATCH] BUG/MEDIUM: cache: Fix hash collision in `accept-encoding` handling for `Vary`

[Willy Tarreau]

Hi Rémi,

(sorry for the lag, still on vacation, I don't look at haproxy every day :-))

On Tue, Dec 29, 2020 at 10:31:58AM +0100, Remi Tricot-Le Breton wrote:
> > I remain convinced that now that we have the bitmap, we're probably
> > deploying too many efforts to allow caching of unknown encodings with
> > all the consequences it implies, and that just like in the past we would
> > simply not cache when Vary was presented, not caching when an unknown
> > encoding is presented would seem perfectly acceptable to me, especially
> > given the already wide list of known encodings. And when I see the test
> > using "Accept-Encoding: br,gzip,xxx,jdcqiab", this comforts me in this
> > idea, because my feeling here is that the client supports two known
> > encodings and other ones, if we find an object in the cache using no
> > more than these encodings, it can be used, otherwise the object must
> > be fetched from the server, and the response should only be cached if
> > it uses known encodings. And if the server uses "jdcqiab" as one of
> > the encodings I don't care if it results in the object not being cached.
> 
> It would not be that hard to discard responses that have an unknown encoding
> but for now the content-encoding of the response is only parsed when the
> response has a vary header (it could actually have been limited to a
> response that varies on accept-encoding specifically). And considering that
> we probably don't want to parse the content-encoding all the time, responses
> with an unknown encoding would only be considered uncacheable when they have
> a vary, and would be cached when they don't. I'm totally fine with it if you
> are too, it would only require a few lines of explanation in the doc.

Well, if a content-encoding is specified in the response and there's no
vary header, it's the server's problem. I mean, we could be kind and helpful
and purposely decide not to cache in this case, but it remains the server's
responsibility to advertise accept-encoding in the vary header if an encoding
is used. And the server could very well decide that it knows its clients
always support the delivered encoding and purposely decide not to emit a
vary header for example.

Thus I'd suggest that we continue to inspect the content-encoding only
when vary is present, but just decide that when we find a token we don't
know, we simply refrain from caching. Based on the exhaustive list you've
already implemented, this will really affect no real-world use case at
the moment, and will allow us to totally get rid of the hash and the
collisions that come with it.

And as Tim mentionned, the tests you've implemented are easy to extend
to support new encodings in the future, so I think it's worth doing it
this way. But I'm also fine with alternate proposals, it's just that I
suspect that whatever other solution might become more complex for no
real-world benefit.

Thanks,
Willy

Re: [PATCH v2] BUG/MEDIUM: cache: Fix hash collision in `accept-encoding` handling for `Vary`

[Willy Tarreau]

Hi Tim,

On Tue, Dec 29, 2020 at 12:43:53PM +0100, Tim Duesterhus wrote:
> This patch fixes GitHub Issue #988. Commit 
> ce9e7b25217c46db1ac636b2c885a05bf91ae57e
> was not sufficient, because it fell back to a hash comparison if the bitmap
> of known encodings was not acceptable instead of directly returning the the
> cached response is not compatible.
> 
> This patch also extends the reg-test to test the hash collision that was
> mentioned in #988.
> 
> Vary handling is 2.4, no backport needed.

Many thanks, now applied!

Willy