-
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
certprovider-api: bootstrap support for certificate providers #3901
Conversation
// If no builder is registered with the provided name, nil will be returned. | ||
func getBuilder(name string) Builder { | ||
func GetBuilder(name string) Builder { |
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 package is not internal, so this means the users can GetBuilder() directly.
Did we want users to use the store only?
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.
Switched back to not exporting, and using the internal package trick now.
if err != nil { | ||
return nil, fmt.Errorf("xds: Config parsing for plugin %q failed: %v", name, err) | ||
} | ||
configs[instance] = c |
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.
Do we not need the name
in the returned config as well? How will the consumer of this config know which builder to use?
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.
Good catch. Thank you. Fixed.
return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) | ||
} | ||
configs := make(map[string]certprovider.StableConfig) | ||
for instance, data := range providerInstances { |
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.
What is an instance
?
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.
A plugin_instance
, which represents a plugin_name
+plugin_config
.
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 is probably minor detail.
Why does the instance
need an "instance_name"? Would we have two instances with the same plugin_name
, but different config? And since we always stop at the first valid config, what's the point?
This is similar to what we need for the balancing policy + config. We didn't make it a map[string]{policy+config}
, but a []{policy+config}
.
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.
Yes, we can have two different instances with same plugin_name
but different config. An instance
is supposed to represent an instantiation of a plugin (which includes the plugin_name
+config
).
We don't stop at the first valid config here. We process all configs and use the instance
specified by the control plane.
if len(gotCfgs) != len(wantCfgs) { | ||
return fmt.Errorf("config.CertProviderConfigs is %d entries, want %d", len(gotCfgs), len(wantCfgs)) | ||
} | ||
for name, gotCfg := range gotCfgs { |
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.
The key of this map should be instance
, not name
, right?
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.
It was going the right thing, just using the wrong variable name. :) Fixed it too.
* Do not export `getBuilder` from `certprovider` package. * Include `pluginName` in configs exported by the `bootstrap` package.
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.
Handled all your comments. Thanks for the catch.
I can't seem to be able to reply to your comments because some of those changes are undone now.
return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) | ||
} | ||
configs := make(map[string]certprovider.StableConfig) | ||
for instance, data := range providerInstances { |
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.
A plugin_instance
, which represents a plugin_name
+plugin_config
.
Now, I was able to reply to your individual comments as well. |
return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) | ||
} | ||
configs := make(map[string]certprovider.StableConfig) | ||
for instance, data := range providerInstances { |
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 is probably minor detail.
Why does the instance
need an "instance_name"? Would we have two instances with the same plugin_name
, but different config? And since we always stop at the first valid config, what's the point?
This is similar to what we need for the balancing policy + config. We didn't make it a map[string]{policy+config}
, but a []{policy+config}
.
Changes include:
GetBuilder
function fromcertprovider
package.CertProviderConfigs
field in theConfig
type exported by thebootstrap
package. This will contain the parsed certprovider configs.