Netman inherits its etcd configuration from CF etcd

[markstgodard]

Hi Diego team

Container networking team PR to do the following:

• netman pulls etcd properties from config_from_cf
• remove unused cf_etcd section

Please let us know if you have any questions or concerns.

For more information, please see
https://www.pivotaltracker.com/story/show/131579069

Thanks

@markstgodard && @karampok
Container Networking Team

[cfdreddbot]

Hey markstgodard!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

[cfdreddbot]

Hey markstgodard!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/131853557

The labels on this github issue will be updated when the story is started.

[emalm]

Thanks, @markstgodard, generally sounds reasonable. A few details:

• The "input" slots in the internal template are all (( merge )) and so require those cert and key values to be specified in the CF manifest. The CF manifest-generation templates do always provide some value for those keys in https://github.com/cloudfoundry/cf-release/blob/bd14bed3efe9faac88332c7177b7b09102738b66/templates/cf.yml#L609-L614, but to be maximally flexible those should be (( merge || nil )) (or the equivalent ~) in case the operator relies on etcd job spec defaults.
• The "output" slots in the internal template should each have a space between the key reference and the )) delimiter.

Could you please make those changes?

Best,
Eric

[markstgodard]

Hi @ematpl

Thanks good catch, updated PR.

Cheers

[emalm]

Thanks. @markstgodard. Generally, it's best to set nil as the fallback everywhere instead of "" or false, so that the final manifest will use job spec defaults. If you look at that capture section in the bottom half of the internal template, you'll see that the only values are (( merge )) or (( merge || nil ))/~. I don't see why you would need specific overrides for those defaults, since the client configuration for your components should be determined entirely by the information in the CF manifest, and in any case if you did they should go in the diego.yml template, not the config-from-cf ones.

Best,
Eric

[mcwumbly]

Thanks @ematpl

@markstgodard and I have pushed a new commit with the suggested changes.

[jenspinney]

Hi @markstgodard and @mcwumbly,

It doesn't look like the data in these fields are actually being included in the final diego.yml file. You'd also need to modify diego-release/manifest-generation/diego.yml to reference these properties in order for it to have an effect.

When I manually modified my cf.yml file to have bogus values for the certs and keys, these values didn't show up at all in the final diego.yml. Or are you using this intermediate file, config-from-cf.yml, directly during your manifest generation and not actually referencing diego.yml?

Thanks,
Jen

[markstgodard]

Hi @jenspinney

Correct, the later, we are referencing in our netman stubs

For example: this branch etcd-tls-from-cf is what we will eventually merge into master and use: https://github.com/cloudfoundry-incubator/netman-release/tree/etcd-tls-from-cf

https://github.com/cloudfoundry-incubator/netman-release/blob/etcd-tls-from-cf/manifest-generation/stubs/bosh-lite-netman-diego.yml#L24-L31

If this PR is getting too wonky to discuss async, we can quickly x-team pair if you like

Cheers

[jenspinney]

Ok, looks good then. Sorry for the delay!