Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul the auto-config translation #8193

Merged
merged 2 commits into from
Jun 27, 2020

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jun 26, 2020

This fixes some issues around spurious warnings about using enterprise configuration in OSS.

While I am incredibly unsatisfied with how this turned out, it would take huge changes to our config to do it better. If we were willing to do that then it would be better to unify the config structure of the protobuf encoded configuration sent for auto-config with the agent config.

See the doc comment for the translateConfig function for all the intricate details.

@mkeeler mkeeler requested a review from a team June 26, 2020 18:55
@mkeeler mkeeler force-pushed the feature/auto-config/suppress-config-warnings branch from e5b3e31 to 7b77837 Compare June 26, 2020 19:14
This fixes some issues around spurious warnings about using enterprise configuration in OSS.
@mkeeler mkeeler force-pushed the feature/auto-config/suppress-config-warnings branch 2 times, most recently from 0ccbbf2 to b373252 Compare June 26, 2020 19:47
@mkeeler mkeeler force-pushed the feature/auto-config/suppress-config-warnings branch from b373252 to be576c9 Compare June 26, 2020 20:01
Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with the caveat that I don't really understand the change in recordAutoConfigReply

Comment on lines +395 to +401
// overwrite the auto encrypt DNS SANs with the ones specified in the auto_config stanza
if len(ac.config.AutoConfig.DNSSANs) > 0 && reply.Config.AutoEncrypt != nil {
reply.Config.AutoEncrypt.DNSSAN = ac.config.AutoConfig.DNSSANs
}

// overwrite the auto encrypt IP SANs with the ones specified in the auto_config stanza
if len(ac.config.AutoConfig.IPSANs) > 0 && reply.Config.AutoEncrypt != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about AuthEncyrpt, why does this need to be overwritten?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This configuration could be provided on the client. If there is client specific configuration like DNSSAN, it shouldn't be overwritten by the server configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now instead of generating TLS certificates and pushing them down (which we will transition to doing at some point hopefully soon), the servers push down configuration that will enable auto_encrypt for generating the TLS certificates.

The AutoConfig settings allow specifying IP/DNS SANs and the AutoEncrypt settings do too. When auto_encrypt is being enabled by auto_config we copy over the requested IP/DNS SANs from the AutoConfig settings to the AutoEncrypt settings.

@mkeeler mkeeler merged commit 85fd8c5 into master Jun 27, 2020
@mkeeler mkeeler deleted the feature/auto-config/suppress-config-warnings branch June 27, 2020 14:06
@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 85fd8c5 onto release/1.8.x succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants