[WIP] User-facing Configuration API Discussion Kickoff #1011
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package api | ||
|
|
||
| type Config struct { | ||
| Components Components `json:components` | ||
| } | ||
|
|
||
| type Components struct { | ||
| Network Network `json:network` | ||
| DNS DNS `json:dns` | ||
| } | ||
|
|
||
| type Network struct { | ||
| // IP address pool to use for pod IPs. | ||
| // This field is immutable after installation. | ||
| ClusterNetwork []ClusterNetworkEntry `json:"clusterNetwork"` | ||
|
|
||
| // IP address pool for services. | ||
| // Currently, we only support a single entry here. | ||
| // This field is immutable after installation. | ||
| ServiceNetwork []string `json:"serviceNetwork"` | ||
|
Contributor
There was a problem hiding this comment. Why are ServiceNetwork and ClusterNetwork being modelled differently?
Member
Author
There was a problem hiding this comment. This is modelled after the Network type in OpenShift config v1.
Contributor
There was a problem hiding this comment. Ok, I see ClusterNetworkEntry may also take a HostPrefix there. Wondering when this would be relevant and whether it'd be useful for MicroShift, too?
Contributor
There was a problem hiding this comment. Both of these network fields are marked as immutable. Do we have that restriction for MicroShift, or do we need to allow them to change in case a host switches networks and ends up on one that conflicts with older settings here?
Contributor
There was a problem hiding this comment. If you change something like the servicenetwork, then you will end up with a cluster that has services persisted in it that are not in a valid network range. Protection like this is part of the reason to unify the types.
Contributor
There was a problem hiding this comment. We have 2 reasons we can't say these fields are immutable:
That use case isn't a priority right now, but we can't ignore it. |
||
|
|
||
|
Contributor
There was a problem hiding this comment. We need to add the MTU field here. It's a feature we added for one of our testing customers; sometimes, the other side of a remote network cannot do PMTU, and reducing the Pod's MTU helps as a workaround. |
||
| // The port range allowed for Services of type NodePort. | ||
| // If not specified, the default of 30000-32767 will be used. | ||
| // Such Services without a NodePort specified will have one | ||
| // automatically allocated from this range. | ||
| // This parameter can be updated after the cluster is | ||
| // installed. | ||
| // +kubebuilder:validation:Pattern=`^([0-9]{1,4}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])-([0-9]{1,4}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$` | ||
| ServiceNodePortRange string `json:"serviceNodePortRange,omitempty"` | ||
|
Member
There was a problem hiding this comment. are we planning to add/support
Member
There was a problem hiding this comment. in OpenShift 4 configured via
Member
Author
There was a problem hiding this comment. We model the Network part of the config using Network type in OCP config v1. That type spec does have
Member
There was a problem hiding this comment. Ok. @fzdarsky @dhellmann just note that if this is not required / does not get used in the future, we can disable the
Contributor
There was a problem hiding this comment. @zshi-redhat or @mangelajo do either of you have an opinion about @atiratree's question about ExternalIPNetworkCIDR?
Contributor
There was a problem hiding this comment. @dhellmann sorry I missed this question. Yes, ovn-kubernetes supports k8s service of ExternalIP type and it can be used today with prerequisites that the network infrastructure (outside MicroShift cluster) needs to route traffic for the external IP address to MicroShift cluster. However, we probably don't need
Member
There was a problem hiding this comment. in that case I have created a PR that disables the controller so it doesn't have to run #1058 |
||
| } | ||
|
|
||
| type ClusterNetworkEntry struct { | ||
| // The complete block for pod IPs. | ||
| CIDR string `json:"cidr"` | ||
| } | ||
|
|
||
| type DNS struct { | ||
| // baseDomain is the base domain of the cluster. All managed DNS records will | ||
| // be sub-domains of this base. | ||
| // | ||
| // For example, given the base domain `openshift.example.com`, an API server | ||
| // DNS record may be created for `cluster-api.openshift.example.com`. | ||
| // | ||
| // Once set, this field cannot be changed. | ||
| BaseDomain string `json:"baseDomain"` | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,37 +37,37 @@ var ( | |
| ) | ||
|
|
||
| type ClusterConfig struct { | ||
| // URL string `json:"url"` | ||
|
Contributor
There was a problem hiding this comment. This parameter was meant to be used to point to the Kube API service (for other instances to connect to), but is instead used to specify the IP and port this instance's API server binds to.
Member
There was a problem hiding this comment.
I don't see why we would want that to be configurable, the address the API server binds to should always be the same. The url of the API server will also be present in the kubeconfig so other instances can use the kubeconfig to connect to the server.
Contributor
There was a problem hiding this comment. We can't assume that network addresses stay the same. MicroShift hosts can be autonomous devices that move between networks. Hosts may have multiple NICs and multiple IPs. Given those constraints, how do we ensure that the correct API service IP is used?
Member
There was a problem hiding this comment. Then we need dns if the ips are dynamic and we can't trust them. Adding a route to reach out to the API server doesn't sound too bad if we need to support scenarios where hosts have multiple NICs and IPs and they still need an immutable way to reach out to the API server. Also, can you confirm that these constraints are coming from the initial customers of microshift? We are trying to come up with a minimal version of the config to avoid having options configurable that we don't want to support in the future but can't remove because some of the initial customers may already depend on them. |
||
|
|
||
| // ClusterCIDR string `json:"clusterCIDR"` | ||
| // ServiceCIDR string `json:"serviceCIDR"` | ||
| // ServiceNodePortRange string `json:"serviceNodePortRange"` | ||
| // DNS string `json:"dns"` | ||
|
Contributor
There was a problem hiding this comment. The IP of the ClusterDNS probably doesn't need to be user-configurable (as it's hardcoded as 10th IP of the ServiceNetwork), but we may want to keep it as auto-configured value? |
||
| // Domain string `json:"domain"` | ||
| // MTU string `json:"mtu"` | ||
|
Contributor
There was a problem hiding this comment. We'd need to add auto-discovery of this parameter if we don't want it to be user-configurable.
Member
Author
There was a problem hiding this comment. The default value for MTU is set at 1400 at the moment so we can keep that default number.
Contributor
There was a problem hiding this comment. @zshi-redhat @mangelajo does MTU have to be user-configurable or can we auto-detect it somehow?
Contributor
There was a problem hiding this comment. @fzdarsky Until the path mtu is verified to work in ovnk, we need the mtu to be configurable.
Contributor
There was a problem hiding this comment. @benluddy @dgrisonnet MTU is stored on NetworkStatus. You sure you don't want the entire config objects as opposed to just spec?
Contributor
There was a problem hiding this comment. Sometimes PMTU won't work because something on a remote network is broken and out of the customer's control. In this case, they need to be able to limit MTU locally to workaround issues.
Member
There was a problem hiding this comment. Using the NetworkStatus that is supposed to be read-only in OCP to specify a configuration in Microshift is not ideal and kind of confusing, I'd rather diverge from the existing config spec and do conversion later on than bringing the status.
Contributor
There was a problem hiding this comment. I agree with @dgrisonnet. MTU is an example of a way that MicroShift needs to be configurable that (right now) OCP does not. |
||
| } | ||
|
|
||
| type DebugConfig struct { | ||
| // Pprof bool `json:"pprof"` | ||
|
Contributor
There was a problem hiding this comment. Is the suggestion to drop profiling support or implement this param differently?
Member
Author
There was a problem hiding this comment. We would like to enable it by default.
Contributor
There was a problem hiding this comment. Enabled by default would be consistent with upstream/OCP: https://github.com/kubernetes/kubernetes/blob/4516c7972d306918b8848488255d492f94d15aac/staging/src/k8s.io/apiserver/pkg/server/config.go#L348.
Contributor
There was a problem hiding this comment. It's super useful for MicroShift developers for sure, but I'm not sure why we'd want to keep it enabled in a production deployment? I assume it adds overhead (do we have data points on this?) and increases the attack surface, while it's not clear to me how you'd expect that information to be used by whom?
Contributor
There was a problem hiding this comment. The overhead is neglible and the endpoint is protected by the standard kube-apiserver authn/authz
Contributor
There was a problem hiding this comment. What's the benefit of having profiling on by default? Can we quantify the overhead?
Member
There was a problem hiding this comment. I agree with @deads2k that its best to leave this on. Simplifies support interactions and overhead is minimal. |
||
| } | ||
|
|
||
| type MicroshiftConfig struct { | ||
| // ConfigFile string `json:"configFile"` | ||
|
Contributor
There was a problem hiding this comment. We probably no longer have a need to keep this user-configurable. |
||
| // DataDir string `json:"dataDir"` | ||
|
Contributor
There was a problem hiding this comment. We probably no longer have a need to keep this user-configurable. |
||
| // | ||
| // AuditLogDir string `json:"auditLogDir"` | ||
|
Contributor
There was a problem hiding this comment. Is the proposal to log these to stdout instead?
Contributor
There was a problem hiding this comment. I don't think so. Can we get by with a well-known fixed location?
Contributor
There was a problem hiding this comment. We can probably put it somewhere under
Contributor
There was a problem hiding this comment. Keeping an on-disk layout consistent with OCP reduces our documentation, training, and tooling load. It's not configurable on OCP, let's use the same locations. log rotation is handled in-process by the kube-apiserver. When the config.openshift.io types are brought in, there's an option for audit logging levels. Disabling audit logging is accompanied with a warning that, "if something fails, it's quite likely we won't be able to find it without audit logs" |
||
| // LogVLevel int `json:"logVLevel"` | ||
|
Contributor
There was a problem hiding this comment. How else would we configure the verbosity level?
Member
Author
There was a problem hiding this comment. Would it be acceptable to use a default value that will be sufficient for most use cases such as level 3 or 4?
Contributor
There was a problem hiding this comment. Logging means overhead and disk space may be super limited, so I'd assume some users will dial verbosity down by default and will want to dynamically increase it again in case they need to troubleshoot.
Member
There was a problem hiding this comment. We need to have a top-level log-level parameter for convenience purposes. In OCP we usually have those defined at the operator level, but in the case of MicroShift since we don't have any operator, I think such an option should be handled at the MicroShift level directly.
I don't think so unless you are debugging a verbosity level of 0 is enough for the users. Also in OCP the default is 0, so I think we should stick to that. With regards to the actual name of this option as well as the values it can take, I think we should reuse the existing mechanism we have for OCP operators to keep it consistent with the existing products: https://github.com/openshift/api/blob/622889ac07cf3dd8787679af0d3176bf292b3185/operator/v1/types.go#L55-L62
Contributor
There was a problem hiding this comment. The log level is going to need to be configurable in MicroShift. |
||
| // | ||
| // Roles []string `json:"roles"` | ||
|
Contributor
There was a problem hiding this comment. We'll need this once we split control plane and node roles again, but until then this doesn't have to be user-configurable. |
||
| // | ||
| // NodeName string `json:"nodeName"` | ||
|
Contributor
There was a problem hiding this comment. We'll eventually need this override as host names cannot be guaranteed to be unique or stable.
Contributor
There was a problem hiding this comment. It could be selected arbitrarily (even randomly) and persisted to address uniqueness and stability. Is uniqueness an important consideration for MicroShift? My understanding is that there would never be more than one node, so even a fixed node name would be fine. What happens if a user reconfigures the node name after first start?
Contributor
There was a problem hiding this comment. We're starting with single node, because that's where 80% of the use cases are, but expect to need 3-node (HA) eventually. I guess we can cross that bridge when we get there, though. Right now, I believe we need to address mainly the following scenarios:
Contributor
There was a problem hiding this comment. Uniqueness will be important for fleet management, but do we have a separate cluster ID for that? Can we have MicroShift set the node name to a hardware device ID of some sort instead of asking users to configure it?
Contributor
There was a problem hiding this comment. If we agree we can use a hardware ID, we'll want to leave the field in place for now and file a ticket to do the work to change it later. |
||
| // NodeIP string `json:"nodeIP"` | ||
| // | ||
| // Cluster ClusterConfig `json:"cluster"` | ||
| // | ||
| // Manifests []string `json:"manifests"` | ||
|
Contributor
There was a problem hiding this comment. This is where the bootstrap / workload manifests live. Probably doesn't have to be user-configurable, but we'll need well-known locations (in /etc, /var) for different use cases.
Contributor
There was a problem hiding this comment. Are the defaults,
Contributor
There was a problem hiding this comment. To start with, I think so, yes. |
||
| // | ||
| // Debug DebugConfig `json:"debug"` | ||
| } | ||
|
|
||
| // KubeConfigID identifies the different kubeconfigs managed in the DataDir | ||
|
|
||
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.
We're currently using "Components" for the service components that we either embed into the binary (apiserver, kubelet, etc.) or host on the cluster (service-ca, openshift-dns, etc.), so this is overloading the term.
On the other hand, we'll need the possibility to disable hosted components (default CNI, CSI, Ingress, etc. implementations) so users can BYO and so may want to consider this requirement from the start.
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.
We certainly don't have to use the
Componentsterm as it is something we randomly picked. We can change it toInputsto differentiate it from other components.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.
Obviously you're grouping config params by some criterion and the name should reflect that criterion. If would have expected that you group them by the fact that they're pertaining to the cluster (--> "ClusterConfig"?) rather than, say, config parameters for the MicroShift instance/process.
Or is the intention to signal "this group is user-configurable" and not auto-configured?
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 think the intention is "this is a group of user-configurable parameters".