reverseproxy: use pointer when loading modules and remove LazyCertPool

[WeidiDeng]

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.

panic: reflect: call of reflect.Value.Elem on struct Value                                                                                                                                  
                                                                                                                                                                                            
goroutine 1 [running]:                                                                                                                                                                      
reflect.Value.Elem({0x1b23140?, 0xc0008c9260?, 0x457e3e?})                                                                                                                                  
        reflect/value.go:1277 +0x195                                                                                                                                                        
github.com/caddyserver/caddy/v2.Context.LoadModule({{0x20f9bf0, 0xc0002fd9b0}, 0xc0002fc3f0, 0xc0000de600, {0xc0002d6b00, 0x4, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)                            
        github.com/caddyserver/caddy/[email protected]/context.go:158 +0xac                                                                                                                         
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.TLSConfig.MakeTLSClientConfig({{0xc0000e6ac0, 0x39, 0x40}, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, {0x0, ...}, ...}, ...)          
        github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/reverseproxy/httptransport.go:614 +0x634                                                                                   
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*HTTPTransport).NewTransport(0xc0001746c0, {{0x20f9bf0, 0xc0002fd9b0}, 0xc0002fc3f0, 0xc0000de600, {0xc0002d6b00, 0x4, 0x4},
 {0x0, 0x0, ...}, ...})

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?

[mohammed90]

Good catch! Thank you. I checked, and those are the only ones.

[mohammed90]

How were you testing it? What did we miss in our tests?

[WeidiDeng]

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.

[mholt]

I think these should be using pointer receivers on the methods instead, shouldn't they? Otherwise the module will just get loaded onto a copy of the struct? (You're passing in a pointer to a copy of the struct)

Good catch though!!

[WeidiDeng]

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.

[mholt]

Not sure I follow 🤔

[mohammed90]

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.

[mholt]

Why is decoding the RawMessage needed more than once though? It's a static value.

[mohammed90]

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.

[mholt]

Very nice. Thank you so much. We can revisit the LazyCertPool later if needed.