Remove clusterID from installconfig and move it to cluster

[rajatchopra]

ClusterID is now removed from installconfig. The reason is that it should not be possible for a user to override this value. The clusterID is still needed for destroy - and hence it is now a separate asset which gets stored in ClusterMetadata. Other assets needing the clusterID (e.g. legacy manifests) issue a separate dependency on this asset.
For convenience of package depenencies, the asset still lives in the installconfig package.
A follow up PR can move it to cluster package if needed (currently blocked by legacy CVO overrides).

Fixes issue #761

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rajatchopra

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 [rajatchopra]

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

[wking]

One nit, but once we green up CI, what you have here looks good to me.

[crawford]

What happens if the user doesn't use the cluster target and instead stops at ignition-configs? Are they responsible for generating the cluster ID themselves?

[rajatchopra]

What happens if the user doesn't use the cluster target and instead stops at ignition-configs? Are they responsible for generating the cluster ID themselves?

Does anyone need a cluster ID for ignition configs?
Nevertheless, the state file does have the created cluster ID (if the target is ignition-configs) because the manifests still depend on it. So it will be picked up again.

[crawford]

the manifests still depend on it

Okay, so the ID will get generated. This does mean that clusters from the same set of manifests or Ignition Configs will have the same identity and will be indistinguishable from telemetry's point of view. Is this going to be a problem @smarterclayton?

[abhinavdahiya]

why not just type ClusterID string ?

[rajatchopra]

Just for uniformity of initialization, here for example by replacing installconfig.ClusterID{} with new(installconfig.ClusterID)
Also looks kludgy to dereference and assign 'self' in a class function here when it appears like *a = uuid.New()

The sizeof() should also not be different, but drop one hint and I can change it, I am fine either way.

[staebler]

It makes sense to me to have ClusterID be a struct. The type is not the cluster ID itself: The type is the asset that generates the cluster ID.

[staebler]

There is precedent for this in all the other assets. For comparison, installconfig.baseDomain could be a string instead of a struct, too.

[abhinavdahiya]

A follow up PR can move it to cluster package if needed (currently blocked by legacy CVO overrides).\

I thought i removed the legacy CVO overrides... #739

[wking]

This does mean that clusters from the same set of manifests or Ignition Configs will have the same identity and will be indistinguishable from telemetry's point of view.

Oh, hrm. What is Hive going to do about this? I think we may need to be baking the generated ID into the Ignition configs and then just telling folks to not use the same Ignition config for multiple clusters?

And really, if we're using this for telemetry, I think "try and ensure folks use a new, unique UUID for each cluster" is going to be tough to get right. Do we really have a problem with folks using the same cluster ID for multiple clusters? We could use their cloud.openshift.com auth secret to distinguish between different users, and if a given user wants to dump all of their clusters into the same ID bucket (which probably also means "into the same teardown bucket") despite us warning them not to, I'm ok letting them shoot that particular foot-gun.

[smarterclayton]

I don't want to let customers select UIDs, unless they want to shoot themselves in the foot, so that's roughly the same thing.

…

On Wed, Dec 5, 2018 at 2:16 PM W. Trevor King ***@***.***> wrote: This does mean that clusters from the same set of manifests or Ignition Configs will have the same identity and will be indistinguishable from telemetry's point of view. Oh, hrm. What is Hive going to do about this? I think we may need to be baking the generated ID into the Ignition configs and then just telling folks to not use the same Ignition config for multiple clusters? And really, if we're using this for telemetry, I think "try and ensure folks use a new, unique UUID for each cluster" is going to be tough to get right. Do we really have a problem with folks using the same cluster ID for multiple clusters? We could use their cloud.openshift.com auth secret to distinguish between different users, and if a given user wants to dump all of their clusters into the same ID bucket (which probably also means "into the same teardown bucket") despite us warning them not to, I'm ok letting them shoot that particular foot-gun. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#783 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pyfo5m_RU6lGhF-AIUEi_sCeREs8ks5u2BuggaJpZM4ZBP1z> .

[rajatchopra]

Okay, so the ID will get generated. This does mean that clusters from the same set of manifests or Ignition Configs will have the same identity and will be indistinguishable from telemetry's point of view. Is this going to be a problem @smarterclayton?

CVOConfig and ClusterVersion are the two CRDs that use the ClusterID object.

[rajatchopra]

I thought i removed the legacy CVO overrides... #739

Okay, filing another PR to remove the actual template files also, as well as the template variables in manifests.

But not nearly enough because of CVOOverrides

[wking]

I don't want to let customers select UIDs, unless they want to shoot themselves in the foot...

So how do you see this working for folks like Hive that take installer-generated Ignition configs and go forth to do something with them? Should those Ignition configs contain the cluster ID? Or is it up to Hive to stuff in a fresh one before using the Ignition configs to launch a cluster?

[ironcladlou]

@openshift/sig-network-edge if clusterID disappears from the install ConfigMap, wildcard DNS management will be broken, and I don't think e2e-aws will capture that failure yet.

[ironcladlou]

Is https://github.com/openshift/api/blob/master/config/v1/types_cluster_version.go#L33 the correct alternative for consuming cluster ID from an operator?

[staebler]

What happens if the user doesn't use the cluster target and instead stops at ignition-configs? Are they responsible for generating the cluster ID themselves?

Does anyone need a cluster ID for ignition configs?
Nevertheless, the state file does have the created cluster ID (if the target is ignition-configs) because the manifests still depend on it. So it will be picked up again.

We cannot rely on there being a state file. If the end user comes with just the ignition config files, there will be no state file.

[ironcladlou]

Spoke a bit with @rajatchopra off-thread about what consumers of this value should use instead, and I don't know that I have the definitive answer yet.

Does anybody have thoughts on which API within config.openshift.io operators should use instead?

[rajatchopra]

@ironcladlou https://github.com/openshift/api/blob/master/config/v1/types_cluster_version.go#L33 is going to be correct becasue of https://github.com/openshift/installer/blob/master/data/data/manifests/bootkube/cvo-overrides.yaml.template#L9

So, you can pick it up freely. At least as freely as anybody has access to the clusterversion object.

[abhinavdahiya]

still a pending question #783 (comment)

[ironcladlou]

openshift/cluster-ingress-operator#88 opened to fix one usage.

[bparees]

openshift/cluster-image-registry-operator#127 is removing the use of the old value in the registry operator, i would expect e2e-aws to fail in this PR until that PR merges.

[dgoodwin]

Before this goes in would it be possible to get the metadata.json being written before create cluster? Hive has a worrying problem if we can't get the UUID until after cluster is created, if we fail to upload the metadata afterwards we could be creating clusters we cannot cleanup and re-try, which is very much part of what we're supposed to do. If we can upload metadata after generating the manifests, we could upload, and if that fails we can safely re-try, nothing has been created in the cloud yet.

[wking]

Would be good to make metadata.json a new asset attached to ignition-configs (like #1008 is doing for the kubeconfig).

[openshift-ci-robot]

@rajatchopra: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/unit	1d8b8e7	link	/test unit
ci/prow/govet	1d8b8e7	link	/test govet

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[dgoodwin]

This sounds good to me.

[ironcladlou]

Just to be 100% clear, @openshift/sig-network-edge is now okay with this to merge

[wking]

@rajatchopra, looks like some failing unit tests, e.g.:

# github.com/openshift/installer/pkg/asset/installconfig
pkg/asset/installconfig/installconfig_test.go:25:12: unknown field 'ClusterID' in struct literal of type "github.com/openshift/installer/pkg/types".InstallConfig
# github.com/openshift/installer/pkg/types/validation
pkg/types/validation/installconfig_test.go:24:12: unknown field 'ClusterID' in struct literal of type "github.com/openshift/installer/pkg/types".InstallConfig
pkg/types/validation/installconfig_test.go:77:6: c.ClusterID undefined (type *"github.com/openshift/installer/pkg/types".InstallConfig has no field or method ClusterID)

[wking]

/close

Carried into #1057.

[openshift-ci-robot]

@wking: Closed this PR.

In response to this:

/close

Carried into #1057.

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.