Add support for installing Keel

[nimakaviani]

Keel is the microservice required to enable Managed Delivery for Spinnaker. As part of our efforts to make Managed Delivery available to the larger community, we would like to have Kleat generate the config file for Keel and also to add the required adjustments to the config files for other Spinnaker microservices.

Here is a PoC implementation: https://github.com/nimakaviani/kleat

The implementation introduces a feature flag --enable-keel that once set, makes the required config changes. From our discussions in the k8s sig meeting, @ezimanyi suggested we might be able to remove the feature flag and instead pre-populate the config files with acceptable defaults.

Any other changes to the above implementation that you see appropriate?

We can start working on the PR once we settle on the final design. thanks!

/cc @queueburt

[maggieneterval]

Thanks again for the fantastic demo at yesterday's Kubernetes SIG, @nimakaviani!

A few comments to consider before making a pull request with the Managed Delivery changes:

1. I agree with @ezimanyi that we should include a mananagedDelivery block in the halconfig rather than creating an --enable-keel flag. We don't have separate flags for the other optional services (Fiat, Kayenta) and instead have blocks in the halconfig in which the functionality these optional services provide (authz, canary) is enabled and configured.
2. It looks like you've also added support for configuring plugins and configuring Redis and SQL as persistent stores: would you mind making a separate PR for these components? I'm pretty unfamiliar with the implementation details of both Managed Delivery and Spinnaker plugins, but does Managed Delivery require one or more plugins to work?
3. You've added messages to keel.proto for Retrofit and Okhttp configuration: we've so far omitted those from other service messages in an attempt to keep Kleat scoped to mostly only Spinnaker-specific application configuration. I think we'll want to put some more thought into if/how we want to expose this type of configuration in Kleat, but we'll at least want to be consistent across all microservices, so would you mind omitting these messages from the initial PR adding Keel support?
4. Many of the proto messages added in this commit do not have a comment for documentation; these will need to be documented prior to merging upstream.

Thanks again for the awesome contribution. So excited that you are using Kleat!

[ezimanyi]

Thanks a lot for the demo and for the interest in supporting Keel!

In addition to @maggieneterval's comments above, it looks like your commit adds a lot of places where you've had kleat handle the defaulting of fields. For example the defaulting of the ConnectTimeoutMs, and the defaulting of the port to 7002. In general, the philosophy of kleat is that each microservice should be responsible for setting reasonable defaults for its values, and that kleat should only set values when they are different from these reasonable defaults.

This is not what Halyard did, which led to a lot of complexity, where defaults where often quite different for users using Halyard vs. deploying and directly writing their config. Ideally the config for kleat should be empty (or almost) for users who have not explicitly overridden default values.

Similarly, for the config properties that are telling keel how to communicate with other services, we've generally relied on the spinnaker.yml service discovery file rather than explicitly set those in every microservice. We have not yet solved the problem of what to do when that spinnaker.yml depends on the input config, though we are aware we'll need to solve that at some point (maybe as part of this work).

Thanks again for the interest here!

[nimakaviani]

Thank you @maggieneterval and @ezimanyi for the quick feedback.

yes the implementation that I shared is a quick and dirty proof of concept. so all your points on missing proper comments on proto fields, lack of tests, and premature defaulting of ports and services are totally valid. I address some of the specifics below.

I agree with @ezimanyi that we should include a mananagedDelivery block in the halconfig rather than creating an --enable-keel flag. We don't have separate flags for the other optional services (Fiat, Kayenta) and instead have blocks in the halconfig in which the functionality these optional services provide (authz, canary) is enabled and configured.

+1

It looks like you've also added support for configuring plugins and configuring Redis and SQL as persistent stores: would you mind making a separate PR for these components? I'm pretty unfamiliar with the implementation details of both Managed Delivery and Spinnaker plugins, but does Managed Delivery require one or more plugins to work?

as for the persistent store, MD uses SQL to store its state. Redis however was required by Front50 in the absence of an S3 or other object storage to persist Spinnaker application state. The PR doesn't necessary need to add Redis but I suppose it is a common persistent store that could be used by others deploying Spinnaker. I'd be happy to go with your suggestion on whether to drop Redis or to have a separate PR to have it added.

The notion of plugins in the context of Keel and MD is slightly different from how plugins are interpreted in other places in Spinnaker. in MD it more or less serves as a feature flag to enable support for a particular provider. I can embed the notion of Keel specific plugins into the Keel proto message object. or if we want to have a placeholder to extend Kleat with plugins more broadly, we can keep them where they are.

You've added messages to keel.proto for Retrofit and Okhttp configuration: we've so far omitted those from other service messages in an attempt to keep Kleat scoped to mostly only Spinnaker-specific application configuration. I think we'll want to put some more thought into if/how we want to expose this type of configuration in Kleat, but we'll at least want to be consistent across all microservices, so would you mind omitting these messages from the initial PR adding Keel support?

sounds good.

In general, the philosophy of kleat is that each microservice should be responsible for setting reasonable defaults for its values, and that kleat should only set values when they are different from these reasonable defaults.

+1 on the above philosophy. Current implementation of Keel assumes the defaults to come from the config file. Some of the defaulting was required for the demo to work. But setting reasonable defaults in the code makes sense.

Similarly, for the config properties that are telling keel how to communicate with other services, we've generally relied on the spinnaker.yml service discovery file rather than explicitly set those in every microservice. We have not yet solved the problem of what to do when that spinnaker.yml depends on the input config, though we are aware we'll need to solve that at some point (maybe as part of this work).

totally agreed. In fact in my first try, I relied on the default service discovery in spinnaker.yml but Keel did not pick up those defaults and specifically required the setting of those service configurations at a higher level config definition.

I can certainly work with the Keel team to address the defaulting issues. However in the meantime, for the config values where there is a lack of proper defaulting or where some of the previous conventions are broken (eg. with service discovery), how do you suggest to move forward? Does it make sense to have Kleat set some of those defaults, and as we push those defaults to Keel, we will clean them up in Kleat? My worry is that it may take quite some time if we wait for Keel to make necessary changes in all places where proper defaulting is needed, and we will lose momentum on having the community experiment with it.

thanks again for the great feedback.

[ezimanyi]

I'd like to at least discuss with the Keel team first how to push the defaults into keel; unless there's something specific about keel that I'm not understanding, this should be fairly straightforward. (Much of the work we did in kleat was actually just updating the microservices to better default values.)

In general, this is just a matter of adding the appropriate values to the base config in the repo (ex: for clouddriver). I'd definitely want to involve the Keel team either way, as I'm not really in a great position to know what sensible defaults are for keel...at which point it will probably be easier to just update Keel with these.

If after discussing with them there is a reason why defaulting in kleat makes sense, we can re-consider, but I'd definitely want a clear plan (and timeline) first. Thanks!