Closed WeidiDeng closed 1 month ago
How were you testing it? What did we miss in our tests?
I was just fixing one of the deprecation warnings and then this one happened, you know, basicauth
and such.
I think there are no tests for these.
From the comment,
// Loaded modules have already been provisioned and validated. Upon returning
// successfully, this method clears the json.RawMessage(s) in the field since
// the raw JSON is no longer needed, and this allows the GC to free up memory.
I think some of the method require more drastic changes actually, since some of validation called LoadModule
which will probably make the loading process fail anyway. Check module lifecycle.
Not sure I follow :thinking:
This brings my attention to a critical point
... this method clears the json.RawMessage(s) in the field since // the raw JSON
If the method CertPool()
is changed to have a pointer received for the LazyCertpool
, then later calls to CertPool
will fail. In this case, I think it's fine to pass the pointer to the copy so the later calls still succeed.
Why is decoding the RawMessage needed more than once though? It's a static value.
Why is decoding the RawMessage needed more than once though? It's a static value.
The module in question here is the LazyCertPool
, which is meant to delay the loading until the method CertPool
is called and doesn't cache (yet... per the TODO). If CertPool
were to be called multiple times, it will try load the child module from CARaw
multiple times.
However, I took a look at the code paths now. The lazy certpool doesn't work as I imagined. The method CertPool
is always called during provisioning, not at client authentication time. I was wrong to conceive of it and include it. The LazyCertPool type may be removed.
While testing the latest beta, I encountered a deprecation
The 'tls_trusted_ca_certs' field is deprecated. Use the 'tls_trust_pool' field instead.
When I made the necessary changes and reloaded, caddy panicked.This is introduced in 6065.
When fixing this issue, I found it's the same problem the the CA provider modules, introduced in 5784.
@mohammed90 @armadi1809 Can you check if there are more instances where value is used instead of pointer when loading modules in your commits?