config: InfrastructureSpec reference cloud provider configuration #245

derekwaynecarr · 2019-03-11T15:19:45Z

Adds a CloudConfig config map reference to InfrastructureSpec that contains the cloud provider configuration file used for in-tree cloud provider integration as well as the external cloud controller manager once enabled.

see:
https://github.com/kubernetes/kubernetes/blob/master/cmd/cloud-controller-manager/app/apis/config/v1alpha1/types.go#L33
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/apis/config/types.go#L180

derekwaynecarr · 2019-03-11T15:20:50Z

Need a place-holder so we can start stubbing writing out the --cloud-conf argument to kubelet, kube-apiserver, kube-controller-manager.

/cc @mfojtik @childsb @enxebre @sjenning

derekwaynecarr · 2019-03-11T15:21:22Z

/hold

so we can another api reviewer to ack.

enxebre · 2019-03-11T15:28:18Z

config/v1/types_infrastructure.go

+	// when using the built-in cloud provider integration or the external cloud controller manager.
+	// The namespace for this config map is openshift-config.
+	// +optional
+	CloudConfig ConfigMapNameReference `json:"cloudConfig"`


since this is optional, would it make sense for it to be a pointer and omitempty?

soltysh

/lgtm
/hold

soltysh · 2019-03-12T17:10:21Z

config/v1/types_infrastructure.go

@@ -22,8 +22,12 @@ type Infrastructure struct {

 // InfrastructureSpec contains settings that apply to the cluster infrastructure.
 type InfrastructureSpec struct {
-	// secret reference?
-	// configmap reference to file?
+	// CloudConfig is a reference to a ConfigMap containing the cloud provider configuration file.


cloudConfig

sjenning · 2019-03-12T21:30:36Z

fyi @rphillips

enxebre · 2019-03-20T11:02:17Z

is there anything else that needs to be done here? should we proceed merging so control plane components can start to watch this?

sjenning · 2019-03-21T20:15:26Z

The hold on this is by @soltysh #245 (comment)

I think he is just asking for s/CloudConfig/cloudConfig/

enxebre · 2019-03-22T10:01:46Z

@derekwaynecarr the PR needs to include make generate so make verify is happy

soltysh · 2019-03-26T09:57:17Z

The hold on this is by @soltysh #245 (comment)

I think he is just asking for s/CloudConfig/cloudConfig/

That is correct, and I still don't see that solved.

derekwaynecarr · 2019-03-26T16:18:08Z

this should be all set.

derekwaynecarr · 2019-03-26T16:18:17Z

/hold cancel

abhinavdahiya · 2019-03-26T16:21:06Z

this should be all set.

in platforms like None. AWS this is going to be empty...

abhinavdahiya · 2019-03-26T16:23:57Z

Also in the past .spec fields we supposed to be user configurable. Do you think cloudConfig reference is user configurable.

soltysh

/lgtm

openshift-ci-robot · 2019-03-26T20:57:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, soltysh

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:

~~OWNERS~~ [derekwaynecarr,soltysh]

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

gnufied · 2019-03-26T22:07:55Z

I have same question as @abhinavdahiya , is cloudConfig user configurable? say on vsphere user changes datastore from ds1 to ds2 - we will have to decide if we should delete existing default storageclass and create new one or we should not touch storageclass once created. Deleting the SC may be problematic, there are parts of code that assume SC to be present throughout the lifecycle of PV (although we are moving away from it).

derekwaynecarr · 2019-03-26T22:16:41Z

@gnufied @abhinavdahiya good point.

i think the username and password that interacts with the iaas provider must support changing, but for both vsphere and openstack these are managed via secrets. if we put this in status instead of spec, we will have to still support editability of the referenced secret.

derekwaynecarr · 2019-03-26T22:37:16Z

there are a few things i guess we should think about.

we could move it to status
should the cloud credential operator manage the secret for vsphere/openstack?

/cc @dgoodwin thoughts?

dgoodwin · 2019-03-27T11:31:43Z

Does Spec imply human user editable? Reading through https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status it doesn't seem to match, certainly anything human editable should be in Spec, but the key differentiator is desired state, whereas status is for observed state. Cloud provider config to me feels like desired state, not observed. There are ways we can make fields immutable once set. However we're already moving fast in this direction for this infrastructure object, but it still feels wrong to be tucking these things into status. I will raise this on the group g arch call this week to see what everyone thinks.

CCO could definitely manage the secrets assuming we can get CCO running prior to when cloud config is needed. Does CCO need to know anything about cloud config? (because that feels off to me at least initially) We'll discuss on a call later this morning.

abhinavdahiya · 2019-03-27T12:06:15Z

Does Spec imply human user editable? Reading through https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status it doesn't seem to match, certainly anything human editable should be in Spec, but the key differentiator is desired state, whereas status is for observed state.

The desired state by who? users drive the desired state and therefore there is the expectation of users that spec fields are configurable unless specifically denied. Since the cloud provider config location is setup by the installer at install time, keeping in status provides the current observed state.

Cloud provider config to me feels like desired state, not observed. There are ways we can make fields immutable once set. However we're already moving fast in this direction for this infrastructure object, but it still feels wrong to be tucking these things into status.

It's easier to start with status because

there is user expectation that this is not configurable but observed state.
no controller that watches changes to spec and ensures any changes have trickled through to.update status means spec is not achievable for now.

I will raise this on the group g arch call this week to see what everyone thinks.

CCO could definitely manage the secrets assuming we can get CCO running prior to when cloud config is needed. Does CCO need to know anything about cloud config? (because that feels off to me at least initially)

I think Derek was talking about the secret with infracreds present inside the cloud config for openstack/vshpere

dgoodwin · 2019-03-27T12:49:35Z

Does Spec imply human user editable? Reading through https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status it doesn't seem to match, certainly anything human editable should be in Spec, but the key differentiator is desired state, whereas status is for observed state.

The desired state by who? users drive the desired state and therefore there is the expectation of users that spec fields are configurable unless specifically denied. Since the cloud provider config location is setup by the installer at install time, keeping in status provides the current observed state.

Desired state for the cluster, be it by the user, a programmatic component, or a user via a programmatic component. Some of these fields definitely should be immutable, no question, but I'm concerned that "anything we don't want humans changing = status" is not consistent with the API guidelines. Anything human editable should be in spec, but I can't find anything that says the inverse is true. If that's accurate, we should not push things to status when what we're really wanting is immutable fields.

Cloud provider config to me feels like desired state, not observed. There are ways we can make fields immutable once set. However we're already moving fast in this direction for this infrastructure object, but it still feels wrong to be tucking these things into status.

It's easier to start with status because
* there is user expectation that this is not configurable but observed state.

* no controller that watches changes to spec and ensures any changes have trickled through to.update status means spec is not achievable for now.

I'll agree it's easier right now, I'm questioning if it's correct/consistent API design and if it's easier in the long run. If we drop this information into status and people start coding to it, it becomes very difficult to pull it out later. We should get APIs right as early as possible.

If this cloud config (a) was observed, (b) can be reconstructed if lost, (c) never needs to modified by a user then perhaps this makes sense in status.

The other fields I'm thinking about:

Platform: Did we observe that this is an AWS cluster, or did the user specify this when they installed? Can we know it's AWS if the status were cleared? IMO the user set this, it's desired state for the cluster and most users of the value will be treating it as such.

InfrastructureName (from the other PR). We didn't observe this, the installer (most likely) set it, if lost we're in trouble, and most users of the value are looking for desired state, not observed.

abhinavdahiya · 2019-03-27T13:16:18Z

If you take installer as a one shot controller. Installer sets the observed state in status for this object. There was no desired state for this object.

Installer one shot controller would be equivalent to controller with internal logic and decisions

dgoodwin · 2019-03-27T15:12:18Z

Not following the train of thought, it's not about desired state for the Infrastructure object, I'm viewing the Infrastructure object as desired state for the cluster. Will raise in group g arch call this week.

dgoodwin · 2019-03-28T17:24:58Z

From group G arch call: api conventions doesn't cover a lot of case law, if it's a fundamental fact of the cluster that cannot be changed it is observed state, spec can be things you were told, status can be reality.

The cloud provider config is optionaly present on the bootstrap node based on the user choice or platform. When the `.spec.cloudConfig` [1] is set, that requires the cloud provider config file to be provided. In cluster operator uses the same field from `infrastructure.config.openshift.io/cluster` object to selectively fetch the cloud config configmap reference. [1]: openshift/api#245

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 11, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan March 11, 2019 15:19

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2019

openshift-ci-robot requested review from childsb, enxebre, mfojtik and sjenning March 11, 2019 15:20

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2019

enxebre reviewed Mar 11, 2019

View reviewed changes

soltysh approved these changes Mar 12, 2019

View reviewed changes

openshift-ci-robot assigned soltysh Mar 12, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2019

config: InfrastructureSpec reference cloud provider configuration

07f28f5

derekwaynecarr force-pushed the cloud-conf branch from c24365d to 07f28f5 Compare March 26, 2019 16:17

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2019

abhinavdahiya mentioned this pull request Mar 26, 2019

pkg/types: add vsphere platform openshift/installer#1458

Merged

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019

soltysh approved these changes Mar 26, 2019

View reviewed changes

openshift-merge-robot merged commit db56c6d into openshift:master Mar 26, 2019

abhinavdahiya mentioned this pull request Mar 28, 2019

operator: sync the cloud config in bootstrap and cluster mode openshift/machine-config-operator#591

Merged

wking mentioned this pull request Apr 23, 2019

pkg: Migrate from cluster-config-v1 to Infrastructure.config.openshift.io openshift/cluster-image-registry-operator#260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: InfrastructureSpec reference cloud provider configuration #245

config: InfrastructureSpec reference cloud provider configuration #245

derekwaynecarr commented Mar 11, 2019

derekwaynecarr commented Mar 11, 2019

derekwaynecarr commented Mar 11, 2019

enxebre Mar 11, 2019

soltysh left a comment

soltysh Mar 12, 2019

sjenning commented Mar 12, 2019

enxebre commented Mar 20, 2019

sjenning commented Mar 21, 2019

enxebre commented Mar 22, 2019 •

edited

Loading

soltysh commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

abhinavdahiya commented Mar 26, 2019

abhinavdahiya commented Mar 26, 2019

soltysh left a comment

openshift-ci-robot commented Mar 26, 2019

gnufied commented Mar 26, 2019 •

edited

Loading

derekwaynecarr commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

dgoodwin commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

dgoodwin commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

dgoodwin commented Mar 27, 2019

dgoodwin commented Mar 28, 2019

config: InfrastructureSpec reference cloud provider configuration #245

config: InfrastructureSpec reference cloud provider configuration #245

Conversation

derekwaynecarr commented Mar 11, 2019

derekwaynecarr commented Mar 11, 2019

derekwaynecarr commented Mar 11, 2019

enxebre Mar 11, 2019

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

soltysh Mar 12, 2019

Choose a reason for hiding this comment

sjenning commented Mar 12, 2019

enxebre commented Mar 20, 2019

sjenning commented Mar 21, 2019

enxebre commented Mar 22, 2019 • edited Loading

soltysh commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

abhinavdahiya commented Mar 26, 2019

abhinavdahiya commented Mar 26, 2019

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 26, 2019

gnufied commented Mar 26, 2019 • edited Loading

derekwaynecarr commented Mar 26, 2019

derekwaynecarr commented Mar 26, 2019

dgoodwin commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

dgoodwin commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

dgoodwin commented Mar 27, 2019

dgoodwin commented Mar 28, 2019

enxebre commented Mar 22, 2019 •

edited

Loading

gnufied commented Mar 26, 2019 •

edited

Loading