-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
e5b3e31
to
7b77837
Compare
This fixes some issues around spurious warnings about using enterprise configuration in OSS.
0ccbbf2
to
b373252
Compare
b373252
to
be576c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
// 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
🍒✅ Cherry pick of commit 85fd8c5 onto |
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.