Improve construction of ProcessImpl

[mum4k]

PR #734 changed how ProcessImpl is constructed.

The construction is non-trivial, because of a circular dependency. As described in the code:

  // Ideally we would create the bootstrap first and then pass it to the
  // constructor of Envoy::Api::Api. That cannot be done because of a circular
  // dependency:
  // 1) The constructor of Envoy::Api::Api requires an instance of Bootstrap.
  // 2) The bootstrap generator requires an Envoy::Event::Dispatcher to resolve
  //    URIs to IPs required in the Bootstrap.
  // 3) The constructor of Envoy::Event::Dispatcher requires Envoy::Api::Api.
  //
  // Replacing the bootstrap_ after the Envoy::Api::Api has been created is
  // assumed to be safe, because we still do it while constructing the
  // ProcessImpl, i.e. before we start running the process.

As a workaround ProcessImpl::CreateProcessImpl creates Envoy::Api::Api with an empty Bootstrap, the uses that API to generate the Bootstrap and patches it back in. This doesn't have any functional impact, since all of this happens during construction of Process, but is non-trivial and non-intuitive.

This issue is opened to discuss ideas for impromevent.

[mum4k]

The most obvious workaround would be to avoid using Envoy internals to do the DNS resolution. If we use some other DNS resolution mechanism, we can resolve, generate the bootstrap and then construct the objects.

@oschaaf any thoughts on this? Wondering if there are any hidden reasons why the resolution has to be done using the Envoy stack.

[dubious90]

Another thing I'd like to understand is whether or not this is specific to a weirdness of nighthawk.

All of these components are Envoy components. Every part of this appears to me to be Envoy interacting with envoy. So why isn't this a circular dependency for them too? If it is, it feels like it should be removed upstream. If it is something we are doing strangely, then we should identify what our strange component is.

The only part of this that isn't specifically an Envoy constructor appears to be our bootstrap generation. Offline, I believe you said it is not possible to remove the necessity to resolve URIs in our bootstrap generation, because that is an envoy requirement?

[mum4k]

I believe this is specific to Nighthawk, because Nighthawk reuses Envoy components. Any other user of Envoy would generate the Bootstrap and then just run Envoy with it, having none of these issues. I don't think we have a case for any change upstream.

[mum4k]

To be said, we also have the option of doing nothing. This code weirdness doesn't have any real impact, so it may be hard to justify larger changes.

Of course happy to change my position once we identify such impact.

[oschaaf]

So from what I remember we perform DNS resolution for two reasons:

• Validating hostnames / avoiding a DIY on that. We didn't have a convenient/proper way to get to GoogURL back then, but these days Envoy has it as a dependency and we could piggy-back on that. But today we defer to a dns server for some of it.
• More importantly, some parts of the bootstrap we build need actual socket addresses / ports, and we need to resolve those based on hostnames we get through configuration. The difference with Envoy is that we need to do this earlier in our startup code (before we created the bootstrap). For example:

  nighthawk/source/client/process_impl.cc

  Line 366 in 9a6d718

  socket->set_address(uri.address()->ip()->addressAsString());

@mum4k pointed out we could avoid relying on Envoy for dns resolution (cares directly?). Alternatively maybe we could switch from static bootstrap generation to dynamically adding clusters after Envoy starts up but I think that won't be trivial.

Yet .. another though, looking at the Envoy side of this, it seems like [1] ApiImpl started depending on the top level bootstrap, but instead just needs BufferFactoryConfig, which in turn holds only a single int (minimum_account_to_track_power_of_two).. I wonder would just accepting the integer value be reasonable?

[1] https://github.com/envoyproxy/envoy/pull/17562/files#diff-d91d19ee698b649f3fb585552964b5d6bd3f218aa992e30816d928f90045c913R161

[mum4k]

Thank you Otto, for the summary. My preference would be to resolve DNS directly in our Bootstrap generation. While the argument that Envoy only needs a single int value holds today, it may not hold later on, so I think the API change makes sense from the Envoy perspective.

The reason I like the direct DNS resolution is that once we do it, we will move Nighthawk a step closer to how a regular user of Envoy interacts with it. So hopefully less surprises in the future.

With that said, my position is still on the "do nothing" part, unless we find a reason to justify the changes (time investment).

[mum4k]

For the posterity - we concluded that this isn't work doing. Any implementation of DNS resolution will have to deal with OS and Arch specifics, so we are better off resolving via Envoy.

We don't really have a good reason to change how the creation of objects works in Envoy now. If one of the future updates breaks us in an incompatible way, we can reopen this and discuss with the Envoy community on how to simplify this at the Envoy end.