Agent Auto Configuration: Refactor RPC endpoint to use interface delegate

[mkeeler]

This PR does a couple things.

The first commit does:

1. Renames Cluster.AutoConfig RPC to AutoConfig.InitialConfiguration. This change includes updating the client side code to use the correct name.
2. Updates the AutoConfig RPC endpoint struct to take in an interface object that has methods to provide all the data it needs. Some logic (creating ACL tokens & generating a list of join addresses) was moved out of the RPC endpoint and into methods on the Server type itself. This simplifies the RPC endpoint quite a bit but also allows testing with fewer full test servers and utilizing more mocks. Utilizing the mocks to inject certain data also allows the tests to be simpler as much of the previous non-determinism that was in auto generated IDs and times has moved to being a function of the interface.

The second commit just renames Server.forward to Server.ForwardRPC. This satisfies the interface created in the first commit and removes the temporary shim method in place.

You should be able to glance at the second commit but then focus in on just the first one for all the more substantial changes.

This PR does not produce any functional difference and is simply refactoring.

[mkeeler]

A few things I still want to think over are:

1. Should the AutoConfig RPC endpoint be supplied a configuration when created? What if the configuration is reloaded? Right now we don't hold onto the RPC endpoint structs anywhere so if we did want to allow reloading config and having it see it, then we would need to start tracking these structs. It might still be the right way to go though.
2. Should this all be in one interface or should it require setting multiple smaller interfaces? It already sort of does with the authorizer and delegate interfaces?

[dnephin]

What do you think of something like AutoConfigBackend for the name of this interface? Delegate is kind of implied by it being an interface.

[mkeeler]

That sounds fine to me and I will make the change.

What do you think about the interface? In particular I was debating about whether this should all go into a single interface or multiple as well as whether the Config and TLS Configurator should just be provided to the AutoConfig RPC struct instead of retrieved via the interface.

Setting the TLS Configurator seems simple enough. The Config is less straightforward though as its reloadable. The server would have to track all the things it needed to notify about new configuration.

[dnephin]

The interface looks reasonable to me. I started digging into how it was used a bit, but I ran out of time.

For config reload, an idea I have been playing with would be to have all the components that need to reload implement an interface. I'm not sure exactly what that needs to look like yet, but I think it might be something like this:

type Reloadable interface {
    ReloadStart() (done func())
    UpdateConfig(config.Config)
}

Any component that needs to pause and lock all operations can do so with ReloadStart (and unlock with done), and everything would accept the new config from UpdateConfig().

The agent would store a []Reloadable and call all the ReloadStart() first, then all the UpdateConfig(), and finally all the done() in reverse order.

To get around the import cycle with config UpdateConfig might take a closure instead of a config struct, I'm not sure.

[mkeeler]

So I made a few changes since your review.

I got rid of the GetConfig and TLSConfigurator methods in the interface. Instead those concrete types are passed into a constructor. I also noticed that the only fields on the Config that were used were not runtime reloadable so I didn't bother implementing the reloadability of the configuration (the TLS CA certs are runtime reloadable but the Agent handles updating the shared tls configurator)

As for whats left in the interface, the 3 functions only relation is that they are needed by this RPC endpoint. With another soon to be opened PR I will be adding a couple more related to signing certificates and getting the latest connect CA roots. Still there isn't a strong relationship between these methods. I could have split this into multiple interfaces but that seems a little excessive for now.

[mkeeler]

It would appear that something in my tests hang forever now.

[crhino]

For 1, I kind of like the idea of passing in the configuration and then having the Server or something call ReloadConfig on the auto-config endpoint. That mimics the way we reload config from agent.Agent -> consul.Server and could be a good pattern to follow as we split up more endpoints. Not sure if there are any complications hidden here though.

Given that, I think passing in the tlsConfigurator would also make sense, so that all configuration is passed in on initialization.

As for 2, splitting out the interface, I don't think I have a strong opinion. I could see splitting out interfaces for each like: RPCForwarder, TokenCreator, and DatacenterReader (very quickly named), but I am not sure practically how much value that gives us at the moment. 🤷‍♂️ Do you see an advantage in splitting out more interfaces?

[mkeeler]

I mentioned it in some PR comment replies but for future onlookers, I decided to pass in an agent/consul.Config and a tlsutil.Configurator to the AutoConfig constructor now instead of retrieving those each time they are used from the interface.

For now we are not using any reloadable configurations on the Config struct (we are from the tls configurator but the Agent handles updating the shared configurator). Therefore I did not implement Config reloadability for this RPC endpoint.

[crhino]

LGTM. I think it is reasonable to not deal with reloading config for now and wait until if/when we need to. 👍

[hashicorp-ci]

🍒❌ Cherry pick of commit c707572 onto release/1.8.x failed! Build Log