config/v1/types_infrastructure: Add AWS Region by wking · Pull Request #300 · openshift/api

wking · 2019-04-26T19:57:39Z

The image registry needs this so it can figure out where to create an S3 bucket for installs with cluster-provisioned infrastructure. We need the registry to be happy after:

$ openshift-install create cluster

so we don't want to rely on the installer pushing this config in as a day-2 operation. You can extract "what AWS region am I in?" from the cloud provider, but the registry operator is not authorized to do so. With this addition, we provide a way for the cluster creator to declare a default region for new infrastructure.

This field is in Status and not Spec, because we're not going to support transitioning clusters between regions by reconciling this. The cluster admin can write it once (in the manifest that goes up with the bootstrap Ignition config), and then it's read-only reality forever.

~~The field is optional~~ [edit: not anymore], because you only need to set it if you want the cluster to provision resources for you. For user-provided infrastructure flows, you can leave this unset. However, the registry needs the region for its S3 client, even if it's just to manage data in a user-provided bucket. So folks who leave the property off will have an Available:false registry until they set region or regionEndpoint via the configs.imageregistry.operator.openshift.io custom resource.

adambkaplan · 2019-04-29T18:52:11Z

/cc @deads2k regarding pointer discussion

openshift-ci-robot · 2019-04-29T18:52:34Z

@adambkaplan: GitHub didn't allow me to request PR reviews from the following users: regarding, pointer, discussion.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @deads2k regarding pointer discussion

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

coreydaley · 2019-05-01T00:44:45Z

@openshift/api-reviewers ptal at the above conversation and weigh in.

adambkaplan · 2019-05-02T02:19:09Z

ping @deads2k @derekwaynecarr

adambkaplan

I'm reverting my position and am OK with pointers.

The image registry needs this so it can figure out where to create an S3 bucket for installs with cluster-provisioned infrastructure [1]. We need the registry to be happy after: $ openshift-install create cluster so we don't want to rely on the installer pushing this config in as a day-2 operation. You can extract "what AWS region am I in?" from the cloud provider, but the registry operator is not authorized to do so. With this addition, we provide a way for the cluster creator to declare a default region for new infrastructure. This field is in Status and not Spec, because we're not going to support transitioning clusters between regions by reconciling this. The cluster admin can write it once (in the manifest that goes up with the bootstrap Ignition config), and then it's read-only reality forever. I'd prefer an +optional Region property, because you only need to set it if you want the cluster to provision resources for you. For user-provided infrastructure flows, you could leave this unset. However, the registry needs the region for its S3 client [2], even if it's just to manage data in a user-provided bucket. So folks who leave the property off will have an Available:false registry until they set region or regionEndpoint via the configs.imageregistry.operator.openshift.io custom resource [3,4]. But Adam wanted it required [5], and going from required -> optional later is a non-breaking change while optional -> required would be breaking, so it's required with this commit. [1]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/clusterconfig/clusterconfig.go#L44 [2]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/storage/s3/s3.go#L53-L65 [3]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/manifests/00-crd.yaml [4]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/apis/imageregistry/v1/types.go#L179-L184 [5]: openshift#300 (comment)

adambkaplan

/lgtm

coreydaley · 2019-05-03T01:58:25Z

@openshift/api-approvers ptal

deads2k · 2019-05-03T14:24:58Z

+// PlatformStatus holds the current status specific to the underlying infrastructure provider
+// of the current cluster. Since these are used at status-level for the underlying cluster, it
+// is supposed that only one of the status structs is set.
+type PlatformStatus struct {


This needs a discriminator field which indicates which one of the many values is going to be set. CRD discriminators are assumed to be in the same struct as the subfields I think. @sttts

in the CRD it must be in the same JSON object (which in case of struct embedding might differ from "same struct"). Generators like crd-gen from controller-tools will probably require the same struct.

So bring

api/config/v1/types_infrastructure.go

Line 48 in a1f3bdc

Platform PlatformType `json:"platform,omitempty"`

down into here? And deprecate the old property?

...deprecate for removal in v2 in four years ;)

and that's not only a restriction for CRDs, but will be for native types as well.

...deprecate for removal in v2 in four years ;)

I think you're left with no other choice. Duplicate the value

I'm not sure if native API adds discriminators like
https://godoc.org/k8s.io/api/core/v1#VolumeSource

Ok, pushed the discriminator down into platformStatus deprecating the old property with a1f3bdc -> 81ef6b4.

bparees · 2019-05-03T18:36:38Z

/approve

bparees · 2019-05-03T18:37:07Z

/hold

should this be going into master-next?

bparees · 2019-05-03T18:40:32Z

(apparently i'm not an approver yet. working on that: #307)

bparees · 2019-05-03T18:41:51Z

holding for clarity on whether this should be/needs to be going into 4.1 or not.

wking · 2019-05-03T19:02:04Z

holding for clarity on whether this should be/needs to be going into 4.1 or not.

Motivation here. Dropping cluster-config-v1 in favor of Infrastructure simplifies user-facing docs because all you need to tear down an AWS cluster is the kubernetes.io/cluster/{infrastructureName}: owned tag (and not also the deprecated openshiftClusterID tag). That's tied up in the 4.1-targetted rhbz#1685089. There may be some slush about delivering that bug, although at one point it was a beta-blocker.

wking · 2019-05-03T19:04:03Z

I guess if you wanted to punt on this but still address rhbz#1685089 for 4.1, you could have the registry operator pull both Infrastructure (for InfrastructureName) and cluster-config-v1 (for the default region). Seems easier to land this backwards compatible change though ;). Or just decide that rhbz#1685089 can slip to 4.2 and we'll accept more-complicated tagging docs in the meantime.

bparees · 2019-05-03T20:08:12Z

/hold cancel

adambkaplan · 2019-05-07T13:29:41Z

@wking needs a rebase, which will wipe out any lgtm.

The image registry needs this so it can figure out where to create an S3 bucket for installs with cluster-provisioned infrastructure [1]. We need the registry to be happy after: $ openshift-install create cluster so we don't want to rely on the installer pushing this config in as a day-2 operation. You can extract "what AWS region am I in?" from the cloud provider, but the registry operator is not authorized to do so. With this addition, we provide a way for the cluster creator to declare a default region for new infrastructure. This field is in Status and not Spec, because we're not going to support transitioning clusters between regions by reconciling this. The cluster admin can write it once (in the manifest that goes up with the bootstrap Ignition config), and then it's read-only reality forever. I'd prefer an +optional Region property, because you only need to set it if you want the cluster to provision resources for you. For user-provided infrastructure flows, you could leave this unset. However, the registry needs the region for its S3 client [2], even if it's just to manage data in a user-provided bucket. So folks who leave the property off will have an Available:false registry until they set region or regionEndpoint via the configs.imageregistry.operator.openshift.io custom resource [3,4]. But Adam wanted it required [5], and going from required -> optional later is a non-breaking change while optional -> required would be breaking, so it's required with this commit. I've pushed the type specifier down into platformStatus, deprecating the old property, because discriminator fields need to live inside the struct whose properties they discriminate [6]. It would be nice to be able to drop the old type property and rename platformStatus to platform, but it's too close to API freeze to be able to do that, so we're stuck with the old name. We'll drop the old type in v2, if we ever have a v2 of this type. [1]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/clusterconfig/clusterconfig.go#L44 [2]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/storage/s3/s3.go#L53-L65 [3]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/manifests/00-crd.yaml [4]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/apis/imageregistry/v1/types.go#L179-L184 [5]: openshift#300 (comment) [6]: openshift#300 (comment)

With: $ make generate

wking · 2019-05-07T17:42:46Z

Rebased around #308 with 81ef6b4 -> 78e39eb.

adambkaplan

/lgtm

/assign @deads2k @sttts

openshift-ci-robot · 2019-05-07T18:26:47Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: adambkaplan, bparees, wking

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

The pull request process is described here

Details

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

Pulling in openshift/api@dedfb47b1f (config/v1/types_infrastructure: Add AWS Region, 2019-04-26, openshift/api#300). Generated with: $ dep ensure -update github.com/openshift/api using: $ dep version dep: version : v0.5.1 build date : 2019-03-20 git hash : faa61893 go version : go1.10.3 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false

Using openshift/api@dedfb47b1f (config/v1/types_infrastructure: Add AWS Region, 2019-04-26, openshift/api#300) so the registry can use it as a default for their config resource [1,2]. [1]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/manifests/00-crd.yaml [2]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/apis/imageregistry/v1/types.go#L179-L184

Commit from: openshift@8b3b375 Using openshift/api@dedfb47b1f (config/v1/types_infrastructure: Add AWS Region, 2019-04-26, openshift/api#300) so the registry can use it as a default for their config resource [1,2]. NOTE: keeps the status.platformType set for all platforms compared to the source commit. [1]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/manifests/00-crd.yaml [2]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/apis/imageregistry/v1/types.go#L179-L184

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan April 26, 2019 19:57

wking force-pushed the record-aws-region branch from 40e05ca to 532e4df Compare April 26, 2019 19:59

coreydaley mentioned this pull request Apr 26, 2019

Infrastructure name openshift/cluster-image-registry-operator#267

Merged

adambkaplan suggested changes Apr 26, 2019

View reviewed changes

Comment thread config/v1/types_infrastructure.go Outdated

Comment thread config/v1/types_infrastructure.go

wking force-pushed the record-aws-region branch 3 times, most recently from 1c8501c to c832814 Compare April 26, 2019 21:27

abhinavdahiya reviewed Apr 27, 2019

View reviewed changes

Comment thread config/v1/types_infrastructure.go Outdated

wking force-pushed the record-aws-region branch from c832814 to 30a6215 Compare April 27, 2019 02:28

openshift-ci-robot requested a review from deads2k April 29, 2019 18:52

adambkaplan suggested changes May 2, 2019

View reviewed changes

Comment thread config/v1/types_infrastructure.go

Comment thread config/v1/types_infrastructure.go Outdated

wking force-pushed the record-aws-region branch from 30a6215 to a1f3bdc Compare May 2, 2019 21:48

adambkaplan approved these changes May 3, 2019

View reviewed changes

openshift-ci-robot assigned adambkaplan May 3, 2019

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

deads2k reviewed May 3, 2019

View reviewed changes

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

wking force-pushed the record-aws-region branch from a1f3bdc to 81ef6b4 Compare May 3, 2019 18:57

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 3, 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 May 3, 2019

wking added 2 commits May 7, 2019 10:40

generated

78e39eb

With: $ make generate

wking force-pushed the record-aws-region branch from 81ef6b4 to 78e39eb Compare May 7, 2019 17:42

adambkaplan approved these changes May 7, 2019

View reviewed changes

openshift-ci-robot assigned deads2k and sttts May 7, 2019

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

bparees added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2019

openshift-merge-robot merged commit 8c71d43 into openshift:master May 7, 2019

wking mentioned this pull request May 7, 2019

pkg/asset/manifests/infrastructure: Set status.platformStatus.aws.region openshift/installer#1725

Closed

abhinavdahiya mentioned this pull request Jul 2, 2019

manifests/infrastructure: Setup the platform specific status fields. openshift/installer#1930

Merged

adambkaplan mentioned this pull request Jul 19, 2019

Bug 1705752: update crd openshift/cluster-image-registry-operator#319

Closed

damemi mentioned this pull request Jul 30, 2019

[release-4.1] Bug 1732934: remove defaultservingcert description in apiserver config openshift/cluster-config-operator#75

Merged

wking deleted the record-aws-region branch September 25, 2019 21:51

Conversation

wking commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adambkaplan commented Apr 29, 2019

Uh oh!

openshift-ci-robot commented Apr 29, 2019

Uh oh!

coreydaley commented May 1, 2019

Uh oh!

adambkaplan commented May 2, 2019

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

coreydaley commented May 3, 2019

Uh oh!

deads2k May 3, 2019

Choose a reason for hiding this comment

Uh oh!

sttts May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking May 3, 2019

Choose a reason for hiding this comment

Uh oh!

wking May 3, 2019

Choose a reason for hiding this comment

Uh oh!

sttts May 3, 2019

Choose a reason for hiding this comment

Uh oh!

deads2k May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya May 3, 2019

Choose a reason for hiding this comment

Uh oh!

wking May 3, 2019

Choose a reason for hiding this comment

Uh oh!

bparees commented May 3, 2019

Uh oh!

bparees commented May 3, 2019

Uh oh!

bparees commented May 3, 2019

Uh oh!

bparees commented May 3, 2019

Uh oh!

wking commented May 3, 2019

Uh oh!

wking commented May 3, 2019

Uh oh!

bparees commented May 3, 2019

Uh oh!

adambkaplan commented May 7, 2019

Uh oh!

wking commented May 7, 2019

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

wking commented Apr 26, 2019 •

edited

Loading

sttts May 3, 2019 •

edited

Loading

deads2k May 3, 2019 •

edited

Loading