Bootstrap Configuration #302
Conversation
src/k8s/api/v1/bootstrap_config.go
Outdated
| DatastoreType *string `json:"datastore-type,omitempty" yaml:"datastore-type,omitempty"` | ||
| DatastoreURL *string `json:"datastore-url,omitempty" yaml:"datastore-url,omitempty"` | ||
| DatastoreCACert *string `json:"datastore-ca-crt,omitempty" yaml:"datastore-ca-crt,omitempty"` | ||
| DatastoreClientCert *string `json:"datastore-client-crt,omitempty" yaml:"datastore-client-crt,omitempty"` | ||
| DatastoreClientKey *string `json:"datastore-client-key,omitempty" yaml:"datastore-client-key,omitempty"` |
There was a problem hiding this comment.
DatastoreType seems superfluous. Help me know why its there?
if we're using a socket for k8sdqlite, isn't that socket file just a DatastoreURL?
secondarily -- DatastoreURL should really be a []string because i understand that the args are a comma joined set of URLs. right?
There was a problem hiding this comment.
@addyess We could use external datastore. If that's the case, we bootstrap the node with datastore-type: external
There was a problem hiding this comment.
@mateoflorido no i get that completely. I just think that if you override the datastore url, you're very obviously using an "external" datastore. from the kube-apiserver, it just wants to know the following:
etcd-servers
etcd-keyfile
etcd-certfile
etcd-cafile
it doesn't have a special name for the type. This bootstrap config should represent only those things. The "type" is a made up thing internally to this app but is unnecessary.
the default should be etcd-servers = DatastoreURL = unix:///var/snap..../k8sdqlite.socket
and if someone wants it to be something else like a URL -- cool so be it. Why is it necessary to drag this extra stringly typed field?
There was a problem hiding this comment.
@addyess The DatastoreType is external for etcd for example. We've got some logic based on the value of this type when we grab the cluster config from the bootstrap config. We're also using it to print our k8s status. In theory we could check if the DatastoreURL is set and that would mean we are dealing with an external datastore but that might make those switch statements quite ugly.
| switch b.GetDatastoreType() { | ||
| case "", "k8s-dqlite": | ||
| config.Datastore = Datastore{ | ||
| Type: vals.Pointer("k8s-dqlite"), | ||
| K8sDqlitePort: vals.Pointer(b.K8sDqlitePort), | ||
| K8sDqlitePort: b.K8sDqlitePort, | ||
| } | ||
| case "external": | ||
| config.Datastore = Datastore{ | ||
| Type: vals.Pointer("external"), | ||
| ExternalURL: vals.Pointer(b.DatastoreURL), | ||
| ExternalCACert: vals.Pointer(b.DatastoreCACert), | ||
| ExternalClientCert: vals.Pointer(b.DatastoreClientCert), | ||
| ExternalClientKey: vals.Pointer(b.DatastoreClientKey), | ||
| ExternalURL: b.DatastoreURL, | ||
| ExternalCACert: b.DatastoreCACert, | ||
| ExternalClientCert: b.DatastoreClientCert, | ||
| ExternalClientKey: b.DatastoreClientKey, | ||
| } | ||
| default: | ||
| return ClusterConfig{}, fmt.Errorf("unknown datastore type specified in bootstrap config %q", b.GetDatastoreType()) | ||
| } |
There was a problem hiding this comment.
I keep getting hung up here. why'd we invent our own switch for datastore type baked into the config. Feels mostly like a cli thing. Either there is an external datastore or there isn't. If there's no user configured DatastoreURL -- then we should default to the k8s-dqlite socket.
| } | ||
| case "external": | ||
| if len(b.DatastoreServers) == 0 { |
There was a problem hiding this comment.
Should we introduce a BootstrapConfig.validate method that checks that (and other validation, if any)?
|
Summary
Make sure we can set all fields with
BootstrapConfigwhen starting the cluster. Re-use the UserFacingClusterConfig, so that all fields fromk8s setare available on bootstrap.Changes
apiv1.BootstrapConfigapiv1.BootstrapConfig<->types.ClusterConfigyaml.UnmarshalStrictfor parsing the config file, such that unknown keys are not silently ignored.