KIC 2.0: handle dump-config flag

[rainest]

--dump-config enables a diagnostic dump of generated configuration. It is disabled by default.

[tharun208]

as #1408 is there. instead of writing to local files, can we expose the config in a separate URL /config ?.

thoughts @rainest @shaneutt

[rainest]

Yeah, that's the rough plan, probably under /debug/config-the filesystem dumps were a quick and easy way to get the files directly from the config sync loop, but my long-term plan was to instead expose a web interface, since copying stuff out is clunky.

The other thing I'd like to add is a short history of the last few generated configurations rather than only recording the latest good/bad config: we've found that one of the most common use cases for this is to figure out why the controller is updating configuration when we wouldn't expect it to. Getting several iterations of config updates with the single dumps is possible by just copying it out multiple times, but you have to time it manually, which is annoying.

[tharun208]

I like to work on this. I am thinking of providing the option to dump the config using the flag and also serving through an interface. and can we later deprecate the flag? WDYT @rainest

[rainest]

@tharun208 sure, go ahead.

We do want to leave the flag in place since enabling these can expose sensitive info within the config somewhere it wouldn't normally be accessible, and it should only be on temporarily when collecting diagnostic info though, so please don't remove that.

[tharun208]

@rainest, is it okay to extend the UpdateKongAdminSimple as we are already calling deckgen.ToDeckContent ?
https://github.com/Kong/kubernetes-ingress-controller/blob/next/pkg/sendconfig/common_workflows.go#L47

[rainest]

Yes with some caveats. UpdateKongAdminSimple() is where you'll add functionality to ship the dump elsewhere as the generated content doesn't leave it. It is roughly the 2.x equivalent of the 1.x OnUpdate(), where we handle config dumps currently.

You'll note that the dump handling there doesn't always use the original generated config. Dumping supports both a redacted mode (omitting credentials and certificates) and full/sensitive mode. The redacted mode needs to regenerate configuration by passing state.SanitizedCopy() instead of state. In sensitive mode, you do pass the original targetContent.

[tharun208]

started working on this.
One more question, I am thinking not to extend the signature of the UpdateKongAdminSimple with the dump config and dumpDir values. Instead, can I add these values to the kong config struct?

[rainest]

I don't see any particularly good place to stick it. Originally we added the dir to the user config struct and passed that (which already included the dump mode) to OnUpdate() (sort of the predecessor to UpdateKongAdminSimple()). OnUpdate() was also a method and had access to the controller config (sort of like the clientgoCachedProxyResolver struct).

They arguably shouldn't go into the Kong struct, since that holds connection information for the admin API, and the dump configuration doesn't fall into that category.

As-is, I think these should be new fields in clientgoCachedProxyResolver and should be arguments to UpdateKongAdminSimple().

The latter's signature is getting unwieldy at this point. @shaneutt do you think it'd make sense to refactor it to accept a new struct containing static config (probably ingressClassName, enableReverseSync, this dump configuration, and kongConfig, and possibly the cache, which don't change between invocations) and pass that instead?

[tharun208]

@rainest, also I am planning to expose the config on the web interface, can I include the same in this pr or?

[rainest]

Yep, that's fine. It'd be an additional part of https://github.com/Kong/kubernetes-ingress-controller/blob/next/railgun/internal/diagnostics/server.go

To implement that you'd presumably add a channel to ship the config blob from the update loop to the diagnostics server.

[rainest]

@tharun208 hey, checking in on this again. We'd like to put out a feature complete 2.x beta by the end of July, so we plan to take over development of this to get it out on time for that. Did you have any draft code for this already in progress that you wanted to publish for us to try and work on collaboratively, or was it as of yet not started?

[tharun208]

@rainest I haven't started working on this. I am un-assigning myself from this issue.