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

[WIP] start cluster-wide configuration structs #107

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@deads2k
Contributor

deads2k commented Oct 8, 2018

This starts the idea of a cluster-wide configuration struct using images as an example for us to try to choose over/under the line.

@openshift/api-review @derekwaynecarr @bparees

@openshift-ci-robot

This comment has been minimized.

Show comment
Hide comment
@openshift-ci-robot

openshift-ci-robot Oct 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot commented Oct 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ExternalRegistryHostname string `json:"externalRegistryHostname,omitempty"`
// AdditionalTrustedCA is a path to a pem bundle file containing additional CAs that
// should be trusted during imagestream import.
AdditionalTrustedCA ConfigMapReference `json:"additionalTrustedCA,omitempty"`

This comment has been minimized.

@deads2k

deads2k Oct 8, 2018

Contributor

I think this needs to be common and I made a struct that gives an alternative to the old filename approach.

@deads2k

deads2k Oct 8, 2018

Contributor

I think this needs to be common and I made a struct that gives an alternative to the old filename approach.

This comment has been minimized.

@bparees

bparees Oct 8, 2018

Contributor

i've got no problem w/ making it a configmapreference, but since the old code read the CAs from the path defined here, this means the code needs to be updated to read the configmap instead.

Furthermore is the expectation that code that cares about the CAs should setup a watch on the configmap? or otherwise cache the value? Or we're ok with every time we go through a codepath that needs the additional CAs, we make an api call to fetch the configmap?

@bparees

bparees Oct 8, 2018

Contributor

i've got no problem w/ making it a configmapreference, but since the old code read the CAs from the path defined here, this means the code needs to be updated to read the configmap instead.

Furthermore is the expectation that code that cares about the CAs should setup a watch on the configmap? or otherwise cache the value? Or we're ok with every time we go through a codepath that needs the additional CAs, we make an api call to fetch the configmap?

This comment has been minimized.

@deads2k

deads2k Oct 8, 2018

Contributor

i've got no problem w/ making it a configmapreference, but since the old code read the CAs from the path defined here, this means the code needs to be updated to read the configmap instead.

I don't think that's the case. I think that your operator would take care of plumbing the reference here down into a mountable configmap local to your pod and you could simply mount that new configmap and reference it. That avoid granting exposed processes powers to read configmaps in other namespaces and keeps API code out of binaries that don't need it. Also, it makes code more easily testable because you don't need an apiserver up in order to read a config value.

@deads2k

deads2k Oct 8, 2018

Contributor

i've got no problem w/ making it a configmapreference, but since the old code read the CAs from the path defined here, this means the code needs to be updated to read the configmap instead.

I don't think that's the case. I think that your operator would take care of plumbing the reference here down into a mountable configmap local to your pod and you could simply mount that new configmap and reference it. That avoid granting exposed processes powers to read configmaps in other namespaces and keeps API code out of binaries that don't need it. Also, it makes code more easily testable because you don't need an apiserver up in order to read a config value.

This comment has been minimized.

@bparees

bparees Oct 8, 2018

Contributor

what operator? We don't have an "image import api" operator, so is the openshift-api operator going to do it?

@bparees

bparees Oct 8, 2018

Contributor

what operator? We don't have an "image import api" operator, so is the openshift-api operator going to do it?

This comment has been minimized.

@bparees

bparees Oct 8, 2018

Contributor

and how does the code in my pod know where to read the value from? it used to get it from the filepath stored in "AdditionalTrustedCA" that it found in the master-config, but if that's a configmap reference that's no longer the way it can be done.

@bparees

bparees Oct 8, 2018

Contributor

and how does the code in my pod know where to read the value from? it used to get it from the filepath stored in "AdditionalTrustedCA" that it found in the master-config, but if that's a configmap reference that's no longer the way it can be done.

This comment has been minimized.

@deads2k

deads2k Oct 9, 2018

Contributor

what operator? We don't have an "image import api" operator, so is the openshift-api operator going to do it?

Sadly, yes. By organizing by feature, we have a config driven by the user goal, not the topology of the cluster.

and how does the code in my pod know where to read the value from? it used to get it from the filepath stored in "AdditionalTrustedCA" that it found in the master-config, but if that's a configmap reference that's no longer the way it can be done.

We have examples of how to do this inside of the openshift-apiserver operator where you can copy configmaps and secrets and then reference them in your config: https://github.com/openshift/cluster-openshift-apiserver-operator/blob/master/pkg/operator/sync_openshiftapiserver_v311_00.go#L103-L116

The actual binary never knows the difference because it is driven by a config file on disk to enable unit testing.

@deads2k

deads2k Oct 9, 2018

Contributor

what operator? We don't have an "image import api" operator, so is the openshift-api operator going to do it?

Sadly, yes. By organizing by feature, we have a config driven by the user goal, not the topology of the cluster.

and how does the code in my pod know where to read the value from? it used to get it from the filepath stored in "AdditionalTrustedCA" that it found in the master-config, but if that's a configmap reference that's no longer the way it can be done.

We have examples of how to do this inside of the openshift-apiserver operator where you can copy configmaps and secrets and then reference them in your config: https://github.com/openshift/cluster-openshift-apiserver-operator/blob/master/pkg/operator/sync_openshiftapiserver_v311_00.go#L103-L116

The actual binary never knows the difference because it is driven by a config file on disk to enable unit testing.

Show outdated Hide outdated config/v1/types.go Outdated
DisableScheduledImport bool `json:"disableScheduledImport"`
// ScheduledImageImportMinimumIntervalSeconds is the minimum number of seconds that can elapse between when image streams
// scheduled for background import are checked against the upstream repository. The default value is 15 minutes.
ScheduledImageImportMinimumIntervalSeconds int32 `json:"scheduledImageImportMinimumIntervalSeconds"`

This comment has been minimized.

@deads2k

deads2k Oct 8, 2018

Contributor

This only used by the controller, but it feels more config-y somehow....

@deads2k

deads2k Oct 8, 2018

Contributor

This only used by the controller, but it feels more config-y somehow....

This comment has been minimized.

@smarterclayton

smarterclayton Oct 11, 2018

Member

I need to think about these as tuning.

@smarterclayton

smarterclayton Oct 11, 2018

Member

I need to think about these as tuning.

This comment has been minimized.

@smarterclayton

smarterclayton Oct 11, 2018

Member

I do think these are things we might have to allow to be set by someone. How would the tuning controller set them?

@smarterclayton

smarterclayton Oct 11, 2018

Member

I do think these are things we might have to allow to be set by someone. How would the tuning controller set them?

This comment has been minimized.

@smarterclayton

smarterclayton Oct 11, 2018

Member

If we don't have a tuning controller yet, would these go into spec? Should we have to move something from spec to status or duplicate it?

@smarterclayton

smarterclayton Oct 11, 2018

Member

If we don't have a tuning controller yet, would these go into spec? Should we have to move something from spec to status or duplicate it?

Namespace string `json:"namespace"`
Name string `json:"name"`
// FileName allows pointing to a specific file inside of the configmap. This is useful for logical file references.
FileName string `json:"filename,omitempty"`

This comment has been minimized.

@bparees

bparees Oct 8, 2018

Contributor

why isn't this just a key within the configmap?

in what sense does a configmap even contain "files"? (doesn't it just contain keys+values where the values can be mounted as files?)

@bparees

bparees Oct 8, 2018

Contributor

why isn't this just a key within the configmap?

in what sense does a configmap even contain "files"? (doesn't it just contain keys+values where the values can be mounted as files?)

This comment has been minimized.

@deads2k

deads2k Oct 8, 2018

Contributor

in what sense does a configmap even contain "files"? (doesn't it just contain keys+values where the values can be mounted as files?)

I think most people who use them think of the keys as filenames since mounting a configmap mounts each key as a file.

@deads2k

deads2k Oct 8, 2018

Contributor

in what sense does a configmap even contain "files"? (doesn't it just contain keys+values where the values can be mounted as files?)

I think most people who use them think of the keys as filenames since mounting a configmap mounts each key as a file.

This comment has been minimized.

@bparees

bparees Oct 8, 2018

Contributor

but there's no obligation here that the configmap be mounted. At the point in time at which you are declaring the reference, you are declaring a reference to a configmap key.

The consumer of this configmapreference could choose to mount that key into a pod, or read it directly. I think it's incorrect to claim it's a filename here unless the only valid way to use this configmapreference is to mount that specific key as that specific filename.

@bparees

bparees Oct 8, 2018

Contributor

but there's no obligation here that the configmap be mounted. At the point in time at which you are declaring the reference, you are declaring a reference to a configmap key.

The consumer of this configmapreference could choose to mount that key into a pod, or read it directly. I think it's incorrect to claim it's a filename here unless the only valid way to use this configmapreference is to mount that specific key as that specific filename.

This comment has been minimized.

@deads2k

deads2k Oct 9, 2018

Contributor

but there's no obligation here that the configmap be mounted. At the point in time at which you are declaring the reference, you are declaring a reference to a configmap key.

The consumer of this configmapreference could choose to mount that key into a pod, or read it directly. I think it's incorrect to claim it's a filename here unless the only valid way to use this configmapreference is to mount that specific key as that specific filename.

No issue with renaming it.

@deads2k

deads2k Oct 9, 2018

Contributor

but there's no obligation here that the configmap be mounted. At the point in time at which you are declaring the reference, you are declaring a reference to a configmap key.

The consumer of this configmapreference could choose to mount that key into a pod, or read it directly. I think it's incorrect to claim it's a filename here unless the only valid way to use this configmapreference is to mount that specific key as that specific filename.

No issue with renaming it.

deads2k added some commits Oct 8, 2018

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 9, 2018

Contributor

@smarterclayton updated to spec/status split based on comments. Things certainly start to look a little odd. I think this one may need merging and dealing with tuning that is not yet system managed looks weird too.

Contributor

deads2k commented Oct 9, 2018

@smarterclayton updated to spec/status split based on comments. Things certainly start to look a little odd. I think this one may need merging and dealing with tuning that is not yet system managed looks weird too.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 9, 2018

Member

/cc @crawford for visibility

Member

ashcrow commented Oct 9, 2018

/cc @crawford for visibility

@openshift-ci-robot openshift-ci-robot requested a review from crawford Oct 9, 2018

@crawford

How is this configuration going to be written to disk? I hope the plan is to translate this config into a base MachineConfig and then let MCD/Ignition write the changes to disk.

//DisableScheduledImport bool `json:"disableScheduledImport"`
// AllowedRegistriesForImport limits the docker registries that normal users may import
// images from. Set this list to the registries that you trust to contain valid Docker

This comment has been minimized.

@crawford

crawford Oct 9, 2018

Member

s/Docker/container/

@crawford

crawford Oct 9, 2018

Member

s/Docker/container/

}
// ConfigMapReference references the location of a configmap.
type ConfigMapReference struct {

This comment has been minimized.

@crawford

crawford Oct 9, 2018

Member

What is the intended purpose of this type?

@crawford

crawford Oct 9, 2018

Member

What is the intended purpose of this type?

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 9, 2018

Contributor

How is this configuration going to be written to disk? I hope the plan is to translate this config into a base MachineConfig and then let MCD/Ignition write the changes to disk.

It would be fluffed onto disk only by the operators consuming it. So as a for instance, it would be observed and used here: https://github.com/openshift/cluster-openshift-apiserver-operator/blob/master/pkg/operator/observe_config.go#L66-L74

Contributor

deads2k commented Oct 9, 2018

How is this configuration going to be written to disk? I hope the plan is to translate this config into a base MachineConfig and then let MCD/Ignition write the changes to disk.

It would be fluffed onto disk only by the operators consuming it. So as a for instance, it would be observed and used here: https://github.com/openshift/cluster-openshift-apiserver-operator/blob/master/pkg/operator/observe_config.go#L66-L74

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 9, 2018

Member

The idea of exposing configuration to admins makes sense. I do worry about this design in that files that don't come through the MCO->MCD will be overwritten during upgrades or OCP patching since these changes wouldn't be merged into MCs.

Member

ashcrow commented Oct 9, 2018

The idea of exposing configuration to admins makes sense. I do worry about this design in that files that don't come through the MCO->MCD will be overwritten during upgrades or OCP patching since these changes wouldn't be merged into MCs.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 9, 2018

Contributor

The idea of exposing configuration to admins makes sense. I do worry about this design in that files that don't come through the MCO->MCD will be overwritten during upgrades or OCP patching since these changes wouldn't be merged into MCs.

These are API resources managed by the cluster admin. Why would they be overwritten by an automated process?

Contributor

deads2k commented Oct 9, 2018

The idea of exposing configuration to admins makes sense. I do worry about this design in that files that don't come through the MCO->MCD will be overwritten during upgrades or OCP patching since these changes wouldn't be merged into MCs.

These are API resources managed by the cluster admin. Why would they be overwritten by an automated process?

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 9, 2018

Member

@deads2k that may be the disconnect here. The current/previous plan was that all configuration which would be persisted to the hosts disk would come through an MC that eventually made it's way to the MCD who was in charge of deploying or rejecting (and marking degradation) of nodes. In otherwords, the MachineConfig was the single point of configuration that would then apply down files, system/OCP updates, etc... If an operator also owned the master configuration files then the MCD will overwrite it's changes when the next OCP update occurs which wouldn't be ideal.

Member

ashcrow commented Oct 9, 2018

@deads2k that may be the disconnect here. The current/previous plan was that all configuration which would be persisted to the hosts disk would come through an MC that eventually made it's way to the MCD who was in charge of deploying or rejecting (and marking degradation) of nodes. In otherwords, the MachineConfig was the single point of configuration that would then apply down files, system/OCP updates, etc... If an operator also owned the master configuration files then the MCD will overwrite it's changes when the next OCP update occurs which wouldn't be ideal.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 9, 2018

Member

Don't get me wrong here, I like the idea that features can be modified from within the cluster itself, I'm just worried about how these things will be working together (or accidentally work against each other).

Edit: I should note I'm speaking to the whole cluster-wide configuration idea and it's possible that we are about two different things as configuration 😄

Member

ashcrow commented Oct 9, 2018

Don't get me wrong here, I like the idea that features can be modified from within the cluster itself, I'm just worried about how these things will be working together (or accidentally work against each other).

Edit: I should note I'm speaking to the whole cluster-wide configuration idea and it's possible that we are about two different things as configuration 😄

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 9, 2018

Contributor

@deads2k that may be the disconnect here. The current/previous plan was that all configuration which would be persisted to the hosts disk would come through an MC that eventually made it's way to the MCD who was in charge of deploying or rejecting (and marking degradation) of nodes. In otherwords, the MachineConfig was the single point of configuration that would then apply down files, system/OCP updates, etc... If an operator also owned the master configuration files then the MCD will overwrite it's changes when the next OCP update occurs which wouldn't be ideal.

I don't think that this is related to nodes. It is related to how a feature is configured on the cluster. I see machine configuration as a domain that is distinct from general feature configuration.

/cc @derekwaynecarr

Contributor

deads2k commented Oct 9, 2018

@deads2k that may be the disconnect here. The current/previous plan was that all configuration which would be persisted to the hosts disk would come through an MC that eventually made it's way to the MCD who was in charge of deploying or rejecting (and marking degradation) of nodes. In otherwords, the MachineConfig was the single point of configuration that would then apply down files, system/OCP updates, etc... If an operator also owned the master configuration files then the MCD will overwrite it's changes when the next OCP update occurs which wouldn't be ideal.

I don't think that this is related to nodes. It is related to how a feature is configured on the cluster. I see machine configuration as a domain that is distinct from general feature configuration.

/cc @derekwaynecarr

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Oct 11, 2018

Member

machine config is downstream of cluster config. For instance, if I say this cluster is AWS, the source of truth is cluster config. it might be possible for machine config to override what cluster a particular node is on (maybe to disable the cloud provider on a node), but the machine config is not the source of truth.

machine config needs to take into account cluster config in a few cases. certainly any config that we want a cluster admin to be able to change must be respected by the machine config operators. David's example of cluster CIDR is one of the best - that is the source of truth, not machine config.

Member

smarterclayton commented Oct 11, 2018

machine config is downstream of cluster config. For instance, if I say this cluster is AWS, the source of truth is cluster config. it might be possible for machine config to override what cluster a particular node is on (maybe to disable the cloud provider on a node), but the machine config is not the source of truth.

machine config needs to take into account cluster config in a few cases. certainly any config that we want a cluster admin to be able to change must be respected by the machine config operators. David's example of cluster CIDR is one of the best - that is the source of truth, not machine config.

InternalRegistryHostname string `json:"internalRegistryHostname,omitempty"`
// Here's a neat thing. I suspect that we may need to set one in spec and one in status and merge them so that something
// like the service serving cert CA bundle can be added here....

This comment has been minimized.

@bparees

bparees Oct 11, 2018

Contributor

the service CA doesn't necessarily need to be set here. The registry operator can simply setup a configmap for the service-ca by itself and mount that configmap. The service CA is a special and well understood case.

There may come a time when we need to introduce a machine driven status field, but this doesn't feel like that time yet.

@bparees

bparees Oct 11, 2018

Contributor

the service CA doesn't necessarily need to be set here. The registry operator can simply setup a configmap for the service-ca by itself and mount that configmap. The service CA is a special and well understood case.

There may come a time when we need to introduce a machine driven status field, but this doesn't feel like that time yet.

@bparees

This comment has been minimized.

Show comment
Hide comment
@bparees

bparees Oct 17, 2018

Contributor

restarting this discussion here: #111

Contributor

bparees commented Oct 17, 2018

restarting this discussion here: #111

@bparees bparees closed this Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment