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

Validate benchmark name in createBenchmark.sh script #8455

Merged
1 commit merged into from
Jan 10, 2022

Conversation

korthout
Copy link
Member

Description

The create benchmark script uses the benchmark name among others for the
k8s namespace. However, k8s does not accept dots in the namespace. I
found this out after fully building Zeebe and the docker images locally,
which takes a few minutes. We can simply check for this at the start of
the script to fail early and help users of the script with such
mistakes.

Related issues

NA

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

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

Hey @korthout thanks I also ran into this multiple times. I think it make sense to use the real regex here.

So the k8 docs says https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

RFC 1123 Label Names
Some resource types require their names to follow the DNS label standard as defined in RFC 1123. This means the name must:

contain at most 63 characters
contain only lowercase alphanumeric characters or '-'
start with an alphanumeric character
end with an alphanumeric character

Found on SO that if you use a wrong namespace kubectl will tell you which regex they use

$ k create ns not-valid1234234$%
The Namespace "not-valid1234234$%" is invalid: metadata.name: Invalid value: "not-valid1234234$%": a lowercase RFC 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]([-a-z0-9]*[a-z0-9])?')

Based on that and some other posts

I end with this: ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$

Tests:

[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ test =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$ ]] ; then echo "valid"; fi
valid
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ test.test =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$ ]] ; then echo "valid"; fi
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ test-test =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$ ]] ; then echo "valid"; fi
valid
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ tes123t-test =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$ ]] ; then echo "valid"; fi
valid
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ tes123.t-test =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9]){,63}$ ]] ; then echo "valid"; fi

So my suggestion would be to use that regex to match the benchmark name. :)

@Zelldon
Copy link
Member

Zelldon commented Dec 21, 2021

Sorry the first one was not complete (or allowed to many characters):

New one: ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$

[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-123
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi
valid


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-1234
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-12-
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-12A
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-12
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi
valid


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=123456789-123456789-123456789-123456789-123456789-123456789-1.2
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=2
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi
valid


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=-
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=2-
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=2-2
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi
valid


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=22
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi
valid


[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ test=2.2
[zell ~/ cluster: zeebe-cluster ns:zell-st-gw-v3]$ if [[ $test =~ ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ ]] ; then echo "valid"; fi

@korthout korthout force-pushed the korthout-create-benchmark-dots branch from cae468f to 96fe4d5 Compare December 22, 2021 17:01
@korthout
Copy link
Member Author

korthout commented Dec 22, 2021

Tested manually:

./createBenchmark.sh
<benchmark-name> not provided

Usage ./createBenchmark <benchmark-name>

----

./createBenchmark.sh a
<benchmark-name> 'a' not a valid DNS label
See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names

Usage ./createBenchmark <benchmark-name>

----

./createBenchmark.sh release-1.3.0
<benchmark-name> 'release-1.3.0' not a valid DNS label
See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names

Usage ./createBenchmark <benchmark-name>

----

./createBenchmark.sh release-1-3-0
+ mvn clean install -DskipTests -T1C
[INFO] Scanning for projects...

fi
benchmark=$1

if [[ ! $benchmark =~ ^[a-z0-9][-a-z0-9]{0,61}[a-z0-9]$ ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Specific reason why not using ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$? Single letters are normally allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It forced a - to be present for anything longer than 1 character

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's interesting. We can rework it to also allow 1 char

Copy link
Member

Choose a reason for hiding this comment

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

It forced a - to be present for anything longer than 1 character

Not sure what you mean but above you can see in my tests #8455 (comment) it worked also for 22

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm perhaps I had my test wrong. I'll have another look tomorrow :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked among many others ab is not accepted by ^[a-z0-9]([-a-z0-9]{,61}[a-z0-9])?$ see https://regex101.com/r/vjsrEy/1

We can use ^[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?$ see https://regex101.com/r/vjsrEy/2 🎉

@korthout korthout force-pushed the korthout-create-benchmark-dots branch from 96fe4d5 to a0524d1 Compare December 23, 2021 09:56
@korthout korthout mentioned this pull request Dec 23, 2021
9 tasks
ghost pushed a commit that referenced this pull request Dec 24, 2021
8473: Add recreate benchmark script r=korthout a=korthout

## Description

<!-- Please explain the changes you made here. -->
One of the steps in the daily release process is to check whether there are new commits on the release branch. If there are any, the benchmark must be restarted. There did not exist any script for this yet, and the manual steps are not straight forward (e.g. `newBenchmark` will fail because the namespace is already in use).

To avoid these problems, a new `recreateBenchmark.sh` script is introduced that:
- checks whether the namespace already exists and if not suggests solutions
- rebuilds the project and the docker images
- redeploys the benchmark

This uses a special check for the benchmark name to be a valid DNS label. See #8455 for more details

## Related issues

<!-- Which issues are closed by this PR or are related -->

NA



Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
ghost pushed a commit that referenced this pull request Dec 24, 2021
8473: Add recreate benchmark script r=korthout a=korthout

## Description

<!-- Please explain the changes you made here. -->
One of the steps in the daily release process is to check whether there are new commits on the release branch. If there are any, the benchmark must be restarted. There did not exist any script for this yet, and the manual steps are not straight forward (e.g. `newBenchmark` will fail because the namespace is already in use).

To avoid these problems, a new `recreateBenchmark.sh` script is introduced that:
- checks whether the namespace already exists and if not suggests solutions
- rebuilds the project and the docker images
- redeploys the benchmark

This uses a special check for the benchmark name to be a valid DNS label. See #8455 for more details

## Related issues

<!-- Which issues are closed by this PR or are related -->

NA



Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
ghost pushed a commit that referenced this pull request Dec 24, 2021
8478: [Backport release-1.3.0] Add recreate benchmark script r=korthout a=github-actions[bot]

# Description
Backport of #8473 to `release-1.3.0`.

relates to #8455

Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
ghost pushed a commit that referenced this pull request Dec 24, 2021
8477: [Backport stable/1.2] Add recreate benchmark script r=korthout a=github-actions[bot]

# Description
Backport of #8473 to `stable/1.2`.

relates to #8455

Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
ghost pushed a commit that referenced this pull request Dec 24, 2021
8476: [Backport stable/1.1] Add recreate benchmark script r=korthout a=github-actions[bot]

# Description
Backport of #8473 to `stable/1.1`.

relates to #8455

Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
ghost pushed a commit that referenced this pull request Dec 24, 2021
8477: [Backport stable/1.2] Add recreate benchmark script r=korthout a=github-actions[bot]

# Description
Backport of #8473 to `stable/1.2`.

relates to #8455

Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
@korthout korthout added the version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0 label Jan 4, 2022
The create benchmark script uses the benchmark name among others for the
k8s namespace. However, k8s does not accept dots in the namespace. I
found this out after fully building Zeebe and the docker images locally,
which takes a few minutes. We can simply check for this at the start of
the script to fail early and help users of the script with such
mistakes.
@korthout korthout force-pushed the korthout-create-benchmark-dots branch from 60ab749 to 8a6d421 Compare January 6, 2022 13:23
@korthout korthout requested a review from Zelldon January 6, 2022 13:23
Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

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

Thanks @korthout 💟 🎖️

bors r+

@ghost ghost merged commit 5e0ac5a into develop Jan 10, 2022
@ghost ghost deleted the korthout-create-benchmark-dots branch January 10, 2022 11:41
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants