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

bug(controller buildnumbers): Encode PipelineActivity names to avoid collisions #2282

Merged
merged 4 commits into from Nov 23, 2018

Conversation

Projects
None yet
3 participants
@markawm
Copy link
Member

markawm commented Nov 19, 2018

Submitter checklist

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

Description

Current impl. converts any name-illegal chars into -, which could cause collisions and
therefore two pipelines to share the same build number ID space.
(there is also a bug in the current impl. that won't properly support upper case owner/repo/branch names).

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #

bug(controller buildnumbers): Encode PipelineActivity names to avoid …
…collisions.

Current impl. converts any name-illegal chars into -, which could cause collisions and
therefore two pipelines to share the same build number ID space.
(there is also a bug in the current impl. that won't properly support upper case owner/repo/branch names).

@jenkins-x-bot jenkins-x-bot requested review from i0n and jstrachan Nov 19, 2018

@markawm

This comment has been minimized.

Copy link
Member

markawm commented Nov 19, 2018

/assign @jtnord

@markawm

This comment has been minimized.

Copy link
Member

markawm commented Nov 19, 2018

/test

@jtnord
Copy link
Member

jtnord left a comment

1 potential issue and one question left inline.

// 'a-z', '0-9', '-', '.' -> left as-is.
// Any other characters: percent-encoded ('%' + rune hex code).
func EncodeKubernetesName(name string) string {
name = strings.ToLower(name)

This comment has been minimized.

@jtnord

jtnord Nov 20, 2018

Member

what about collisions?
I can have org/repo/branch and org/repo/BrAnCh and they are different things.

This comment has been minimized.

@markawm

markawm Nov 22, 2018

Member

Good point, that was the original point of this change, after all!
Upper case are now converted to lower case (to keep things as readable as possible), and given a prefix to avoid upper/lower collisions like this.
I also realised another key requirement hadn't been met with the original encoding, as % isn't a valid character in a K8S name. So the encoding is as-before with codes, but now uses period in place of percent.

if allowedInK8SName(ch) {
encodedName.WriteRune(ch)
} else {
encodedName.WriteString(fmt.Sprintf("%%%X", int(ch)))

This comment has been minimized.

@jtnord

jtnord Nov 20, 2018

Member

do we need a function to "UnEncode" these (e.g. jx get builds) ? what does the tools show (hopefully not the percent encoded version?)

This comment has been minimized.

@markawm

markawm Nov 22, 2018

Member

Not that I can see atm.
tbh, I'm not entirely sure where we display these atm, but yes tools could well show them. For the values that currently work, the tools will show the same stuff. For the values that currently don't work at all, the tools should display some weird stuff. But at least it'll work. :-)

@jtnord
Copy link
Member

jtnord left a comment

Code looks ok, tests should be able to run in parallel with T.parallel()

@@ -232,3 +232,22 @@ func TestCreatePipelineDetails(t *testing.T) {
}
}
}

func TestPipelineID(t *testing.T) {
// A simple ID.

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

T.parallel()?

This comment has been minimized.

@markawm

markawm Nov 22, 2018

Member

True... will add.

)

func TestEncodeKubernetesNameNoEncoding(t *testing.T) {
original := "abcdefghijklmnopqrstuvwxyz-.0123456789"

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

T.parallel()

}

func TestEncodeKubernetesNameUpper(t *testing.T) {
original := "ABCDEFGHIJKLMNOPQRSTUVWXYZ"

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

T.parallel()

}

func TestEncodeKubernetesPunctuationString(t *testing.T) {
//Note _valid_ - and . hidden in the string below.

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

T.parallel()

}

func TestEncodeKubernetesNameExtendedRunes(t *testing.T) {
original := "⌘日本語"

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

T.parallel()

@jenkins-x-bot jenkins-x-bot added size/L and removed size/M labels Nov 22, 2018

@jtnord
Copy link
Member

jtnord left a comment

/lgtm

@jenkins-x-bot jenkins-x-bot added the lgtm label Nov 23, 2018

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Nov 23, 2018

[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

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

@jenkins-x-bot jenkins-x-bot merged commit 97d1e14 into jenkins-x:master Nov 23, 2018

2 of 3 checks passed

tide Not mergeable.
Details
Hound No violations found. Woof!
serverless-jenkins succeeded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment