Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The cluster name in generate-yaml.sh is now a random string. #370

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

spew
Copy link
Contributor

@spew spew commented Jun 20, 2018

What this PR does / why we need it:
This change removes the static name from generate-yaml.sh and instead
generates a random string for the name. The reason for doing this is to
allow the script to be run over and over without failing due to accounts
already existing with the same name and to allow developers to verify
their changes with a more clean environment

Release note:

The cluster name in generate-yaml.sh is now a random string.

@kubernetes/kube-deploy-reviewers

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spew

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:

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 20, 2018
@spew
Copy link
Contributor Author

spew commented Jun 20, 2018

/assign @roberthbailey @mkjelland

@mkjelland
Copy link
Contributor

mkjelland commented Jun 20, 2018

What happens if the service account is still being used? For example, I have been running generate-yaml once and then using the same files to create multiple clusters. If I have a VM that is using the service account as it's default, I think that the deletion of the service account will fail, and then the script would still fail. An alternative would be to skip the part that creates the service account if it already exists, or to generate a random string to append to the account name so they'd be uniquely named every time you ran the script.

Also, thanks for doing this, it'll make the script easier to use :)

@spew
Copy link
Contributor Author

spew commented Jun 21, 2018

I considered that case and came up with the following pros / cons for both, so what I have implemented is delete and I would consider what @mkjelland proposed as if exists, skip create. Upon further thought I think it's less disruptive & better to do what @mkjelland proposed. Injecting random strings is probably the best solution. I'll take a quick look and see if that is 'easy'. It's only easy if the script somehow makes sure that all locations that reference the account names are generated by the script.

delete
Pros:

  • Ensures accounts are always in the desired state. For example, if roles / permissions or flags / options are added to the account but at a later date we change generate-yaml.sh to no longer add those roles / permissions or flags / options then they would be removed as the account would be deleted and a fresh clean account would be created.

Cons:

  • Doesn't work if the service account is in use with attached VMs, all clusters must be deleted first

if exists, skip create
Pros:

  • Can handle attached VMs: isn't disrupted if there is an existing VM associated with the service account.

Cons:

  • Account state isn't guaranteed to be what the script says it is -- for example if extra flags & options are added to the account then they would not be added or if permissions are removed from the list that are added then the account would continue to retain them.

@spew spew force-pushed the reentrant-generate-yaml branch from b680ae9 to 4cba86e Compare June 21, 2018 22:08
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 21, 2018
@spew spew changed the title Make generate-yaml.sh re-entrant for service accounts so the command can be run more than once. The cluster name in generate-yaml.sh is now a random string. Jun 21, 2018
@spew
Copy link
Contributor Author

spew commented Jun 21, 2018

Updated with the random string approach. Note that I renamed this PR. The random string is used for the entire cluster name.

@spew spew force-pushed the reentrant-generate-yaml branch from 4cba86e to e100bd5 Compare June 21, 2018 22:11
@@ -184,3 +186,5 @@ cat $ADDON_TEMPLATE_FILE \
| sed -e "s/\$CLUSTER_NAME/$CLUSTER_NAME/" \
| sed "s/\$LOADBALANCER_SA_KEY/$LOADBALANCER_SA_KEY/" \
> $ADDON_GENERATED_FILE

echo -e "\nYour generated cluster name is ''${CLUSTER_NAME}'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an extra ' before ${CLUSTER_NAME}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is, fixing and retesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and re-ran, the last few lines looks like this now:

Your public key has been saved in out/machine-controller-key.pub.
The key fingerprint is:
SHA256:YwcmDSyt8VmNtNwVH0dJbT74qcUNNnwDjZ8wxz2tNTA clusterapi
The key's randomart image is:
+---[RSA 2048]----+
|     o...o  oEBoB|
|    o o+oo.. *.X*|
|     =.o* .  .O=+|
|    . oo .   .*=o|
|        S .  .o+=|
|       . o     =.|
|              o  |
|             .   |
|                 |
+----[SHA256]-----+

Your generated cluster name is 'kqkv6tulrgs'

@spew spew force-pushed the reentrant-generate-yaml branch from e100bd5 to f94ed63 Compare June 21, 2018 22:17
Copy link
Contributor

@mkjelland mkjelland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Looks great, thanks for doing this!

@spew
Copy link
Contributor Author

spew commented Jun 21, 2018

Test failures will be fixed by this commit: #380

CLUSTER_NAME=test1
# Generate a somewhat unique cluster name, UUID is not an option as service-accounts are limited to 30 characters in length
# and have a 19 character prefix (i.e. 'machine-controller-')
CLUSTER_NAME=$(head -c11 < <(tr -dc 'a-zA-Z0-9' < /dev/urandom) | tr '[:upper:]' '[:lower:]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you test this command? Can you verify that it works on both mac and linux?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ tr -dc 'a-zA-Z0-9' < /dev/urandom
tr: Illegal byte sequence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested on mac -- are you getting that on linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried on a GCP VM, it's working for me there as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was the output on my mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is odd because i'm on mac too -- I just realized I installed gnutools on my mac (with brew) and I think I'm running a more linux-type flavor of tr. I'll try to find something that is cross platform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function get-random-string() {
    for i in {0..31};
    do
        string+=$(printf "%x" $(($RANDOM%16)) );
    done;
    echo $string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the default 'tr' on OSX is trying to interpret everything as UTF8 so it doesn't like the non-UTF8 characters coming out of urandom. Prefixing 'tr' with LC_ALL=C will tell tr that the locale is the C standard and therefore the input is treated as simply bytes. Updating with this change.

Tested on both a GCP VM, my mac with gnutools tr, and on my mac with the default OSX tr at /usr/bin/tr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR with the change.

@spew
Copy link
Contributor Author

spew commented Jun 21, 2018

/retest

@spew spew force-pushed the reentrant-generate-yaml branch from f94ed63 to dec5ea7 Compare June 22, 2018 13:11
CLUSTER_NAME=test1
# Generate a somewhat unique cluster name, UUID is not an option as service-accounts are limited to 30 characters in length
# and have a 19 character prefix (i.e. 'machine-controller-')
CLUSTER_NAME=$(head -c11 < <(LC_ALL=C tr -dc 'a-zA-Z0-9' < /dev/urandom) | tr '[:upper:]' '[:lower:]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version works on my mac! Yay!

CLUSTER_NAME=test1
# Generate a somewhat unique cluster name, UUID is not an option as service-accounts are limited to 30 characters in length
# and have a 19 character prefix (i.e. 'machine-controller-')
CLUSTER_NAME=$(head -c11 < <(LC_ALL=C tr -dc 'a-zA-Z0-9' < /dev/urandom) | tr '[:upper:]' '[:lower:]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to concatenate a human friendly name and a random string segment. Especially if you are trying to do things like manually clean up service accounts in the cloud console, it's much easier to think about looking for "test1" and just kill everything with that prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I debated that and went with all random due to the limited number of characters that we can use.

I'll add back in the friendly name with a comment about the restrictions on length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we could possibly follow up with another PR where we make some of the names shorter, for example, the service account for machine controller currently named:

machine-controller-${CLUSTER_NAME}

Could be coming something like,

m-control-${CLUSTER_NAME}

I'd prefer to follow up with that in another PR as the limitation on cluster name length is not a new issue introduced by this PR (although I guess it is made a bit worse by the random string).

This change removes the static name from generate-yaml.sh and instead
generates a random string for the name. The reason for doing this is to
allow the script to be run over and over without failing due to accounts
already existing with the same name and to allow developers to verify
their changes with a more clean environment
@spew spew force-pushed the reentrant-generate-yaml branch from dec5ea7 to fb3009b Compare June 22, 2018 16:30
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 22, 2018
@@ -21,7 +21,13 @@ set -o pipefail
GCLOUD_PROJECT=$(gcloud config get-value project)
ZONE=$(gcloud config get-value compute/zone)
ZONE="${ZONE:-us-central1-f}"
CLUSTER_NAME=test1
# Generate a somewhat unique cluster name, UUID is not an option as service-accounts are limited to 30 characters in length
# and one has a 19 character prefix (i.e. 'machine-controller-'). Of the 11 remaining characters 6 are reserved for the human
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since names must be so short we should consider making the fixed part of this one much shorter (e.g. mc-) at some point. For now I think making the human part be super short is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow up with that change. I agree that 'mc' is fine... for us, but for new hires it will be difficult. Maybe that's just what we have to live with for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also "cc" for cluster-controller will be a bit cryptic.

@spew
Copy link
Contributor Author

spew commented Jun 22, 2018

Updated with the friendly name reintroduced.

@roberthbailey
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2018
@k8s-ci-robot k8s-ci-robot merged commit a9881d5 into kubernetes-sigs:master Jun 22, 2018
jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020
…pport-maintenance-mode

Add support for Machine maintenance window
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants