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.
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.
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.
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.
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.
A plugin_instance, which represents a plugin_name+plugin_config.
There was a problem hiding this comment.
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.
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.
The key of this map should be instance, not name, right?
There was a problem hiding this comment.
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.
| 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.
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.
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}.
easwars
changed the title
Changes include:
GetBuilderfunction fromcertproviderpackage.CertProviderConfigsfield in theConfigtype exported by thebootstrappackage. This will contain the parsed certprovider configs.