*: use a human friendly ID to tag and name cloud resources.

[abhinavdahiya]

AWS:

• All the resources are named <cluster_id>-<something> and all names use - for separator.
  This helps with consistently naming all the resources, and keeping the cluster_id in the reduces the probability of collisions.

• All tags include Name key with name mathcing the name for the resource.
  This allows cluster to find resources using tags based on names.
  Also move some of the Name tags from aws root module into specific modules because setting Name at root level makes it difficult for overriding the Name locally.

• All resources are tagged with kubernetes.io/cluster/<cluster_id>: owned

The loadbalancer target group names also have a max length of 32, therefore the target groups are using 4 character long suffixes.

Libvirt:

• All resources are prefixed with cluster_id

Openstack:

• all resources are prefixed with cluster_id
• openstack continues to use openshiftClusterID: cluster_id to tag all its resources.

The machine-api requires that all machine objects reference back to cluster object as specified by sigs.k8s.io/cluster-api-cluster label
on the machine objects to make sure masters are correctly adopted. Therfore the name for cluster object is the id used to tag the resources.

[wking]

Can we drop this? A non-uniquified name seems like something we'd never use. Edit: except for creating DNS entries, so nevermind.

[abhinavdahiya]

3711b75

[wking]

While we're cleaning up, I think we can drop -role, which is already implied by the resource type. Basically, every bootstrap resource can use ${var.cluster_id}-bootstrap unless we create several of a single resource type.

[abhinavdahiya]

I think it is fine if we keep the names as is. it doesn't hurt here as iam roles do not seem to have any limitation on length of name.

[wking]

See 8a37f72 and f56c341 about not putting kubernetes.io/cluster/... on bootstrap resources except the instance. Do you feel like the accidental-adoption concern is ungrounded?

[abhinavdahiya]

everybidy needs to have kubernetes.io/cluster/... tag so that destroy captures bootstrap resources.

[staebler]

Are we still planning to use the kubernetes.io/cluster/... tag for destroy? Since everything is tagged with the Name tag, can't we search on that instead?

[abhinavdahiya]

Are we still planning to use the kubernetes.io/cluster/... tag for destroy? Since everything is tagged with the Name tag, can't we search on that instead?

Name is resource name and has nothing to do with the cluster. AWS will use kubernetes.io/cluster/... tag.

[wking]

I'm in favor of keeping a tag that Kubernetes ignores (like openshiftClusterID) for tearing down bootstrap resources that are not part of the Kubernetes cluster. But if folks think we don't have to worry about the cluster accidentally adopting bootstrap resources, I'm ok collapsing to just the kubernetes.io/cluster/... tag.

[staebler]

These changes make me happy.

• Should the following use cluster_id?
  https://github.com/openshift/installer/blob/e9073fd3dcdef7595b7eca927b7944e7e1e76dcd/data/data/aws/bootstrap/main.tf#L37

• pkg/asset/machines/worker.go is using the UUID instead of the human ID.

• I don't like that the term cluster ID is overloaded. It would be nice if one of the two uses were qualified in some way so that it is evident from the term which of the cluster IDs is being used.

[staebler]

Can we add human-readable to the description to increase the differentiation between the UUID?

[abhinavdahiya]

Why should we differentiate between UUID and HumanID in terraform?

[staebler]

Within terraform we do not need to differentiate. However, this is at the boundary between terraform and go. The go code is ambiguous with the term cluster ID. As a reader of the code, it is not immediately clear to me here whether the cluster ID that terraform is expecting is the UUID or the human ID. We could either (1) expect the reader of the code to dig further into the code to figure out which one is expected here or (2) add more information to the description of the variable to make it clear.

[abhinavdahiya]

the boundary says give me an identifier for the cluster, it doesn't need to care if it was given an UUID or a ID that is human readable... ?

Let me update it to include constraints like length, characters, because that's what it cares any nothing more.

[abhinavdahiya]

e0614f2

[staebler]

I think it would be worthwhile to replace this with ClusterDomain. That way the logic for how the cluster domain is constructed is contained in only one place rather than in both go and terraform.

[abhinavdahiya]

done with 3711b75

[wking]

Why set Name on resources that have a name property, if we're setting the same value in both? I think it only makes sense when we feel like name must be overly truncated and want the tag for the unabbreviated version.

[abhinavdahiya]

from d6de59a

- All tags include `Name` key with name mathcing the name for the resource.
This allows cluster to find resources using tags based on names.

[wking]

I think we want to trim sensitive characters too, see #1089.

[wking]

Hmm, this is also imposing AWS-specific length and (eventually) charset restrictions on a nominally-platform-agnostic property. Will this end up as a race to the bottom? Maybe it should be a platform-specific asset? Or do we expect this to be something almost all platforms will need?

[staebler]

I considered this same thing with the PR that I put out earlier. My decision was that for the time being there is no reason to make the HumanID characteristics unique to the platform as we do not have any platforms currently that place more onerous restrictions than we face with ELBs from AWS. It should be easy enough to change to platform-specific HumanIDs in the future if and when they are beneficial.

[abhinavdahiya]

Yeah, we should be able to send cluste_ids to different platforms if that becomes a problem.
IMO this HumanID has enough constraints to work through most.

I think we want to trim sensitive characters too, see #1089.

Will do.

[abhinavdahiya]

I think we want to trim sensitive characters too, see #1089.

Will do.

added here 55b2410

[staebler]

What is the motivation behind bringing this here from the route53 module? I would have expected that we would want to go the other way with these route53 resources and bring aws_route53_record.etcd_a_nodes, aws_route53_record.etcd_cluster, and aws_route53_zone.int to the route53 module instead of bringing route53 resources out of the route53 module.

[abhinavdahiya]

with how the current r53 module is setup it takes private zone id and base_domain, and I think pulling the public zone out makes it so that the module takes ids for both for better consistency.

from d6de59a

Also moves the public zone discover to root module and pass the `public_route53_id`, similar to private zone, to `route53` zone.

and for moving the etcd records into the module, that makes sense. let me see and if the change is simple i'll do that.

[staebler]

Well, my contention is that aws_route32_zone.int would be better placed in the route53 module. If that were moved, then there would be consistency in that neither public zone id nor private zone id would be passed into the route53 module.

[abhinavdahiya]

How would you then turn off either of them (not that you can do them right now..)

[staebler]

I don't know. How would you turn off either of them if they were in main instead of the route53 module? How would that be different if they were in the route53 module?

[abhinavdahiya]

for example if we move public and private zone into module, then the module needs 2 variables... base_domain cluster_domain. And if I want to switch off the public zone records, I will have to pass empty base_domain IMO i don't like this indicator.

on the other hand if we pass zone ids, in the root module i pass empty zone id for public zone and that IMO is better indicator.

thoughts?

[staebler]

If we were to make the public zone records optional, then there would need to be a variable coming into terraform to cover that configuration. That same variable would be passed down to the route53 module. I would not suggest overloading the semantics of base_domain to use it to indicate whether the public zone records are off or on. In my opinion, adding an additional variable to the route53 module that explicitly configures whether the public zone records are enabled is a worthwhile trade-off for keeping route53-specific resources contained within the route53 module.

[abhinavdahiya]

done with d4ce44e

[abhinavdahiya]

These changes make me happy.

* Should the following use cluster_id?
  
    
      
        [installer/data/data/aws/bootstrap/main.tf](https://github.com/openshift/installer/blob/e9073fd3dcdef7595b7eca927b7944e7e1e76dcd/data/data/aws/bootstrap/main.tf#L37)
      
      
           Line 37
        in
        [e9073fd](/openshift/installer/commit/e9073fd3dcdef7595b7eca927b7944e7e1e76dcd)
      
      
      
      
  
          
            
             name = "${var.cluster_name}-bootstrap-profile"

Fixed here d2d9bbc

* `pkg/asset/machines/worker.go` is using the UUID instead of the human ID.

Fixed here 1a2a37f bda7b94 6e6d71f

* I don't like that the term cluster ID is overloaded. It would be nice if one of the two uses were qualified in some way so that it is evident from the term which of the cluster IDs is being used.

I think it is fair to say the clusterID gived to 2 options a UUID that you can use where you absolutely want global uniquenes and HumanID where you care about uniqueness to only small level.

[staebler]

I think it is fair to say the clusterID gived to 2 options a UUID that you can use where you absolutely want global uniquenes and HumanID where you care about uniqueness to only small level.

Fair enough. If you think that it will always be completely obvious which specific kind of cluster ID is required for every parameter passed to every function, then I am satisfied.

[staebler]

The max length is 27, not 32.

[abhinavdahiya]

done b156efa

[staebler]

s/alphabumeric/alphanumeric/

[abhinavdahiya]

done with 144ab05a5

[staebler]

is of max length maxNameLen

[abhinavdahiya]

Oh Yeah :) will fix that.

[abhinavdahiya]

done with 9f5c0c0

[staebler]

I would rather see the expected replacements written out explicitly in the test cases. This just ends up running the same code that is used in generateHumanID. If there is a bug in the replacement, this won't capture the bug.

[abhinavdahiya]

I tried to make it more meaningful with 9f5c0c0 -> 4650c15 compare

[abhinavdahiya]

looks like network operator error e2e-aws:

level=info msg="Waiting up to 30m0s for the cluster to initialize..."
level=fatal msg="failed to initialize the cluster: timed out waiting for the condition"

        {
            "apiVersion": "config.openshift.io/v1",
            "kind": "ClusterOperator",
            "metadata": {
                "creationTimestamp": "2019-02-20T23:36:45Z",
                "generation": 1,
                "name": "network",
                "resourceVersion": "29767",
                "selfLink": "/apis/config.openshift.io/v1/clusteroperators/network",
                "uid": "628e2788-3568-11e9-b802-0efd314e1e72"
            },
            "spec": {},
            "status": {
                "conditions": [
                    {
                        "lastTransitionTime": "2019-02-20T23:36:53Z",
                        "status": "False",
                        "type": "Failing"
                    },
                    {
                        "lastTransitionTime": "2019-02-20T23:36:53Z",
                        "message": "DaemonSet \"openshift-multus/multus\" is not available (awaiting 1 nodes)\nDaemonSet \"openshift-sdn/ovs\" is not available (awaiting 1 nodes)\nDaemonSet \"openshift-sdn/sdn\" is not available (awaiting 1 nodes)",
                        "reason": "Deploying",
                        "status": "True",
                        "type": "Progressing"
                    },
                    {
                        "lastTransitionTime": "2019-02-20T23:36:53Z",
                        "message": "DaemonSet \"openshift-multus/multus\" is not available (awaiting 1 nodes)\nDaemonSet \"openshift-sdn/ovs\" is not available (awaiting 1 nodes)\nDaemonSet \"openshift-sdn/sdn\" is not available (awaiting 1 nodes)",
                        "reason": "Deploying",
                        "status": "False",
                        "type": "Available"
                    }
                ],
                "extension": null,
                "version": ""
            }
        },

/retest

[abhinavdahiya]

/retest

[abhinavdahiya]

missed dropping variable during rebase :P

[abhinavdahiya]

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel/minimal]

router flaking
/retest

[staebler]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

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 [abhinavdahiya,staebler]

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

[dgoodwin]

What should Hive be passing when we call this function? This looks like it's still cluster ID, but should probably be infraID?

[dgoodwin]

N/M found where it was passed in, we're sending infraID, var could probably use a rename though.