fix: Handle shortening cluster names for clusters named by their FQDN.

[jsharpe]

Submitter checklist

• Change is code complete and matches issue description.
• Change is covered by existing or new tests.

Description

Fixes the case that the cluster name is a FQDN which then ends up with a short name ending in a dot which forms an invalid name used for creating the vault service account

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #3755

[jenkins-x-bot]

Hi @jsharpe. Thanks for your PR.

I'm waiting for a jenkins-x member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[jsharpe]

/assign @dwnusbaum

[jsharpe]

/hold
This doesn't quite work - the ShortName needs to fulfil the criteria from the error message:

a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is 'a-z0-9?')

Apache Spark had a similar issue: https://issues.apache.org/jira/browse/SPARK-24894 their fix was apache/spark#23781

[jsharpe]

/hold cancel
Added a revised version that replaces any . with a -

[pmuir]

/ok-to-test

[jsharpe]

/retest

[amoussawi]

check this #3755 (comment)

[jsharpe]

check this #3755 (comment)

Yes I saw this comment but I shouldn't have to work around it in this way. The code is broken.

[jtnord]

see #2864 and #3795 for why just truncating was a bad idea to begin with.

[jsharpe]

see #2864 and #3795 for why just truncating was a bad idea to begin with.

Can we merge this anyway as it fixes the install for clusters that are created using kops? The issue of duplicate names from the truncation can be handled later?

[jsharpe]

check this #3755 (comment)

Note that if you make this change it breaks the kops tooling as it needs the name in kube config with the dots if that is how it was created.

[hferentschik]

Can we merge this anyway as it fixes the install for clusters that are created using kops? The issue of duplicate names from the truncation can be handled later?

@jtnord Is this not correct? This change does not address #2864 and #3795, but kube.ToValidNameTruncated seems to provide the better name shortening than ShortClusterName

As next step, one could concatenate the truncated string with a truncated hash of the original name (eg n characters of the sha-1 hash)

[jtnord]

/lgtm

[jenkins-x-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtnord

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

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