Adds platform-specific specs to Infrastructure in config.openshift.io #231

fabianofranz · 2019-03-01T20:31:17Z

This adds platform-specific specs to the specs of infrastructures in config.openshift.io/v1.

Specifically, this allows in-cluster components to read settings related to the underlying infrastructure where they're running. For example, operators like the cloud credentials operator can read the infra ID and tags set by the installer so that they can tag their own resources.

Targeting AWS as of now, but adding more infrastructure providers is supported.

/cc @dgoodwin @wking @crawford

fabianofranz · 2019-03-01T20:32:52Z

/assign @jwforres

config/v1/types_infrastructure.go

fabianofranz · 2019-03-06T19:20:56Z

Comments addressed, @abhinavdahiya @wking @smarterclayton PTAL?

abhinavdahiya · 2019-03-06T22:54:54Z

/approve

abhinavdahiya · 2019-03-06T22:56:04Z

config/v1/types_infrastructure.go

+	// infrastructureName uniquely identifies a cluster with a human friendly name.
+	// Once set it should not be changed. Must be of max length 27 and must have only
+	// alphanumeric or hyphen characters.
+	InfrastructureName string `json:"infrastructureName"`


nit: InfrastructureID because it is an identifier. but looks like @smarterclayton wanted it this way so feel free to keep it as-is

I'm good with either InfrastructureID or InfrastructureName. To me the difference sound more like in terms of uniqueness (an unique IDentifier) than about the content and readability. So in that sense I'd have it as InfrastructureID. But Name seems to be compliant with similar use cases in the API, e.g. the GroupName.

Name in our API is a unique in space (but not time) identifier. Names are things that refer to entities in other systems. UID is reserved for unique in time and space things we generate ourselves. If you can set it, it's not a UID, because someone else could set it to the same value.

Name in our API is a unique in space (but not time) identifier. Names are things that refer to entities in other systems. UID is reserved for unique in time and space things we generate ourselves. If you can set it, it's not a UID, because someone else could set it to the same value.

I'm going to pin this comment in my bookmarks :)

derekwaynecarr · 2019-03-06T23:01:41Z

can we have a follow-on pr that ensures we write a validation that these status fields are not mutable once set by the installer? we could loosen that further if they are observable at a later time.

https://github.com/openshift/origin/tree/master/pkg/admission/customresourcevalidation

derekwaynecarr · 2019-03-06T23:02:38Z

in upi install mode, who sets these values?

derekwaynecarr · 2019-03-06T23:02:54Z

/assign

fabianofranz · 2019-03-07T00:23:14Z

in upi install mode, who sets these values?

@wking ideas?

wking · 2019-03-07T00:27:31Z

in upi install mode, who sets these values?

The installer will generate these values for anyone who wants them. In the UPI mode where the user runs the installer to generate an Ignition config for the bootstrap machine, that Ignition config will contain a manifest for this object, as well as all the other usual manifests. In the UPI mode where the user manually configures their own bootstrap machine, it's going to be up to them to set up the manifests (although they could still have the installer generate them and just, for example, extract them from the Ignition config without actually using Ignition).

wking · 2019-03-07T00:28:18Z

/approve

openshift-ci-robot · 2019-03-07T00:28:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abhinavdahiya, fabianofranz, wking
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: derekwaynecarr

If they are not already assigned, you can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

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

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

fabianofranz · 2019-03-10T21:30:05Z

@derekwaynecarr for approval.

derekwaynecarr · 2019-03-11T19:21:36Z

config/v1/types_infrastructure.go

+// AWSPlatformStatus holds the current status of the Amazon Web Services infrastructure provider.
+type AWSPlatformStatus struct {
+	// tags holds key and value pairs that are set as tags in the AWS resources created by the cluster.
+	Tags map[string]string `json:"tags,omitempty"`


does this actually extend to loadbalancers, volumes, or machines created by machine api?

for in-tree aws cloud provider, it uses the cluster id here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L548

I think we can say that it should after this merges. I can notify those teams to make sure they're respecting it, if possible.

Machine API already has this info on each MachineSet, so user created ones wouldn't match unless the user thought to add the same tags.

I'm not sure how ingress/registry/storage are tagging but it would be nice to get everyone on same page. (kubernetes.io/cluster/dgoodwin-dev-6vzj7=owned)

I believe the infrastructureName being added above should match the kube one you link to.

does this actually extend to loadbalancers, volumes, or machines created by machine api?

the installer currently tags all the machine/machinesets, loadbalancers with these tags on aws
eg: https://github.com/openshift/installer/blob/5455c4ac317900208a92983e35e6b87fc74fd354/pkg/asset/machines/aws/machines.go#L114-L129

for in-tree aws cloud provider, it uses the cluster id here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L548

These are the tags important to openshift on AWS, all controllers must add these and then add more if it likes.

this is a bit chicken/egg i guess.

the in-tree aws cloud provider from kube only applies the tags it knows about here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/tags.go

which right now is of the form:
kubernetes.io/cluster/cluster-id=owned | shared

which we are getting assigned to entities by a bit of luck because of how we pre-tag ec2 instances. on startup, kube cloud provider sees its aws, sees no cloud.conf file to pull the cluster id from, and then falls back to reading the ec2 instance for its tags.

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L1256

i would prefer that we update the language a bit here to reflect that the full set of tags enumerated may not be used by all entities unless we are saying the only tag we will set here. what will we set in this map? do we care about both key names and values?

If it's holding up this PR, I'm fine adding language about anything beyond kubernetes.io/cluster/... being best-effort.

For our purposes it was really the InfrastructureName we need as that should be the basis for the KubernetesClusterID, the tag can be inferred.

If we drop specification of arbitrary tags can we get this through @derekwaynecarr ?

the tag can be inferred.

I would be cautious of operators making same decision independently.

a best-effort API is hard to understand and enforce.

@abhinavdahiya

the tag can be inferred.

I would be cautious of operators making same decision independently.

How can we make this explicit then?

InfrastructureTag:
Key: kubernetes.io/cluster/infraName
Value: owned

InfrastructureTag: kubernetes.io/cluster/infraName=owned (string)

My vote would be just to leave it implicit, if you need to tag cloud resources we expect you to apply the standard kube tag using the InfrastructureName as the Kube cluster-id.

dgoodwin · 2019-03-25T18:36:56Z

I will reopen with some additional commits, taking this over for Fabiano.

…t.io Mostly, this is replacing cluster-config-v1 (which is deprecated [1]) with the Infrastructure config object. As part of that, I'm dropping UserTags support (see discussion in [2,3]). I'm also replacing the openshiftClusterID tag with a "kubernetes.io/cluster/{infraName}: owned" tag [4,5]. [1]: openshift/installer#680 [2]: openshift/api#231 (comment) [3]: openshift/api#266 [4]: openshift/installer#1280 [5]: https://bugzilla.redhat.com/show_bug.cgi?id=1685089#c11

…t.io Mostly, this is replacing cluster-config-v1 (which is deprecated [1]) with the Infrastructure config object. As part of that, I'm dropping UserTags support (see discussion in [2,3]). I'm also replacing the openshiftClusterID tag with a "kubernetes.io/cluster/{infraName}: owned" tag [4,5]. UnversionedRESTClientFor (vs. RESTClientFor) avoids errors like: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1304/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-fc9948cc-b767n_cluster-image-registry-operator.log.gz | gunzip I0423 05:43:03.599910 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-g38fc8f9-dirty ... I0423 05:43:04.942285 1 bootstrap.go:38] generating registry custom resource E0423 05:43:04.942775 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: GroupVersion is required when initializing a RESTClient, requeuing InClusterConfig does not set GroupVersion [6] and RESTClientFor requires it [7] while UnversionedRESTClientFor can fill it in [8]. [1]: openshift/installer#680 [2]: openshift/api#231 (comment) [3]: openshift/api#266 [4]: openshift/installer#1280 [5]: https://bugzilla.redhat.com/show_bug.cgi?id=1685089#c11 [6]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L422-L428 [7]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L269-L270 [8]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L331-L333

…t.io Mostly, this is replacing cluster-config-v1 (which is deprecated [1]) with the Infrastructure config object. As part of that, I'm dropping UserTags support (see discussion in [2,3]). I'm also replacing the openshiftClusterID tag with a "kubernetes.io/cluster/{infraName}: owned" tag [4,5]. The trip through clientcorev1.NewForConfig is because calling RESTClientFor on the InClusterConfig raises: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1304/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-fc9948cc-b767n_cluster-image-registry-operator.log.gz | gunzip I0423 05:43:03.599910 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-g38fc8f9-dirty ... I0423 05:43:04.942285 1 bootstrap.go:38] generating registry custom resource E0423 05:43:04.942775 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: GroupVersion is required when initializing a RESTClient, requeuing InClusterConfig does not set GroupVersion [6] and RESTClientFor requires it [7] while UnversionedRESTClientFor can fill it in [8]. But UnversionedRESTClientFor leaves NegotiatedSerializer unset: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1305/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-66d8b56784-5wt9v_cluster-image-registry-operator.log.gz | gunzip I0423 09:21:42.998300 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-g64d5f62-dirty ... I0423 09:21:44.345677 1 bootstrap.go:38] generating registry custom resource E0423 09:21:44.346403 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: NegotiatedSerializer is required when initializing a RESTClient, requeuing So I'm washing through clientcorev1.NewForConfig to get defaults for both [9]. [1]: openshift/installer#680 [2]: openshift/api#231 (comment) [3]: openshift/api#266 [4]: openshift/installer#1280 [5]: https://bugzilla.redhat.com/show_bug.cgi?id=1685089#c11 [6]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L422-L428 [7]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L269-L270 [8]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L331-L333 [9]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/kubernetes/typed/core/v1/core_client.go#L144-L155

…t.io Mostly, this is replacing cluster-config-v1 (which is deprecated [1]) with the Infrastructure config object. As part of that, I'm dropping UserTags support (see discussion in [2,3]). I'm also replacing the openshiftClusterID tag with a "kubernetes.io/cluster/{infraName}: owned" tag [4,5]. The trip through clientcorev1.NewForConfig is because calling RESTClientFor on the InClusterConfig raises: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1304/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-fc9948cc-b767n_cluster-image-registry-operator.log.gz | gunzip I0423 05:43:03.599910 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-g38fc8f9-dirty ... I0423 05:43:04.942285 1 bootstrap.go:38] generating registry custom resource E0423 05:43:04.942775 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: GroupVersion is required when initializing a RESTClient, requeuing InClusterConfig does not set GroupVersion [6] and RESTClientFor requires it [7] while UnversionedRESTClientFor can fill it in [8]. But UnversionedRESTClientFor leaves NegotiatedSerializer unset: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1305/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-66d8b56784-5wt9v_cluster-image-registry-operator.log.gz | gunzip I0423 09:21:42.998300 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-g64d5f62-dirty ... I0423 09:21:44.345677 1 bootstrap.go:38] generating registry custom resource E0423 09:21:44.346403 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: NegotiatedSerializer is required when initializing a RESTClient, requeuing So I'm washing through clientcorev1.NewForConfig to get defaults for both [9]. The RBAC change avoids: $ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/260/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/1306/artifacts/e2e-aws/pods/openshift-image-registry_cluster-image-registry-operator-7f59899bf6-8c8x8_cluster-image-registry-operator.log.gz | gunzip | head -n7 I0423 10:44:51.121540 1 main.go:18] Cluster Image Registry Operator Version: 4.0.0-142-gfa5b59b-dirty ... I0423 10:44:52.461691 1 bootstrap.go:38] generating registry custom resource E0423 10:44:52.464035 1 controller.go:235] unable to sync: unable to get storage configuration from cluster install config: infrastructures "cluster" is forbidden: User "system:serviceaccount:openshift-image-registry:cluster-image-registry-operator" cannot get resource "infrastructures" in API group "" at the cluster scope, requeuing [1]: openshift/installer#680 [2]: openshift/api#231 (comment) [3]: openshift/api#266 [4]: openshift/installer#1280 [5]: https://bugzilla.redhat.com/show_bug.cgi?id=1685089#c11 [6]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L422-L428 [7]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L269-L270 [8]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/rest/config.go#L331-L333 [9]: https://github.com/kubernetes/client-go/blob/59781b88d0faa9d51be48b4ca49eb08e05246de4/kubernetes/typed/core/v1/core_client.go#L144-L155

openshift-ci-robot requested review from abhinavdahiya and adambkaplan March 1, 2019 20:31

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 1, 2019

openshift-ci-robot assigned jwforres Mar 1, 2019

abhinavdahiya reviewed Mar 1, 2019

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

fabianofranz commented Mar 2, 2019

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

smarterclayton reviewed Mar 4, 2019

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

smarterclayton reviewed Mar 4, 2019

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

fabianofranz force-pushed the add-infra-id-to-infrastructure branch from 771331c to 2665158 Compare March 6, 2019 19:19

fabianofranz added 2 commits March 6, 2019 16:22

Adds platform-specific specs to Infrastructure in config.openshift.io

c9b4e5b

generated

5a515e7

fabianofranz force-pushed the add-infra-id-to-infrastructure branch from 2665158 to 5a515e7 Compare March 6, 2019 19:22

abhinavdahiya reviewed Mar 6, 2019

View reviewed changes

openshift-ci-robot assigned derekwaynecarr Mar 6, 2019

derekwaynecarr reviewed Mar 11, 2019

View reviewed changes

fabianofranz closed this Mar 25, 2019

dgoodwin mentioned this pull request Mar 26, 2019

Add InfrastructureName. #266

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

wking mentioned this pull request Apr 25, 2019

pkg/types/installconfig: Add an (*InstallConfig).Tags() helper openshift/installer#465

Closed

ironcladlou mentioned this pull request Jun 19, 2019

Adding Custom Tags to Ingress Operator LB openshift/cluster-ingress-operator#253

Closed

gregsheremeta mentioned this pull request Mar 23, 2021

Bug 1950113: config: add aws usertags to infrastructure #864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds platform-specific specs to Infrastructure in config.openshift.io #231

Adds platform-specific specs to Infrastructure in config.openshift.io #231

fabianofranz commented Mar 1, 2019 •

edited

Loading

fabianofranz commented Mar 1, 2019

fabianofranz commented Mar 6, 2019

abhinavdahiya commented Mar 6, 2019

abhinavdahiya Mar 6, 2019

fabianofranz Mar 7, 2019

smarterclayton Mar 12, 2019

abhinavdahiya Mar 12, 2019

derekwaynecarr commented Mar 6, 2019

derekwaynecarr commented Mar 6, 2019

derekwaynecarr commented Mar 6, 2019

fabianofranz commented Mar 7, 2019

wking commented Mar 7, 2019

wking commented Mar 7, 2019

openshift-ci-robot commented Mar 7, 2019

fabianofranz commented Mar 10, 2019

derekwaynecarr Mar 11, 2019

derekwaynecarr Mar 11, 2019

dgoodwin Mar 11, 2019

abhinavdahiya Mar 12, 2019

derekwaynecarr Mar 12, 2019 •

edited

Loading

wking Mar 13, 2019

dgoodwin Mar 22, 2019

abhinavdahiya Mar 22, 2019 •

edited

Loading

derekwaynecarr Mar 22, 2019

dgoodwin Mar 25, 2019

dgoodwin commented Mar 25, 2019

Adds platform-specific specs to Infrastructure in config.openshift.io #231

Adds platform-specific specs to Infrastructure in config.openshift.io #231

Conversation

fabianofranz commented Mar 1, 2019 • edited Loading

fabianofranz commented Mar 1, 2019

fabianofranz commented Mar 6, 2019

abhinavdahiya commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Mar 6, 2019

derekwaynecarr commented Mar 6, 2019

derekwaynecarr commented Mar 6, 2019

fabianofranz commented Mar 7, 2019

wking commented Mar 7, 2019

wking commented Mar 7, 2019

openshift-ci-robot commented Mar 7, 2019

fabianofranz commented Mar 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoodwin commented Mar 25, 2019

fabianofranz commented Mar 1, 2019 •

edited

Loading

derekwaynecarr Mar 12, 2019 •

edited

Loading

abhinavdahiya Mar 22, 2019 •

edited

Loading