Bug 1970805: Replace slashes in suggested ImageStream name #922

alicerum · 2021-09-08T10:36:31Z

When figuring out an image stream name for created build objects, SuggestName function can return name with slashes in some situations.

It is important to hold correct data (that including slashes, too) in the ImageRef object, because it is used in other places and can lead to errors.

SuggestName method of the ImageRef however, should return name without slashes when suggesting name for an ImageStream.

openshift-ci · 2021-09-08T10:36:34Z

@alicerum: This pull request references Bugzilla bug 1970805, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "4.9.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1970805: Replace slashes in suggested ImageStream name

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.

alicerum · 2021-09-08T11:01:18Z

/bugzilla refresh

openshift-ci · 2021-09-08T11:01:24Z

@alicerum: This pull request references Bugzilla bug 1970805, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

/bugzilla refresh

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.

gabemontero

/lgtm

alicerum · 2021-09-08T18:12:11Z

/retest

coreydaley · 2021-09-08T18:25:59Z

pkg/helpers/newapp/app/imageref.go

+		if r.AsImageStream {
+			// in some certain situations, suggested ImageStream name might contain slashes
+			// in this case we want to replace it with dashes, referenced object might
+			// contain said dashes, but imagestream must cannot
+			// example: https://bugzilla.redhat.com/show_bug.cgi?id=1970805
+			name = strings.ReplaceAll(name, "/", "-")
+		}


I think we could future-proof this some by eliminating the r.AsImageStream check and just always replacing slashes with dashes.

Suggested change

if r.AsImageStream {

// in some certain situations, suggested ImageStream name might contain slashes

// in this case we want to replace it with dashes, referenced object might

// contain said dashes, but imagestream must cannot

// example: https://bugzilla.redhat.com/show_bug.cgi?id=1970805

name = strings.ReplaceAll(name, "/", "-")

}

// in some certain situations, suggested ImageStream name might contain slashes

// in this case we want to replace it with dashes, referenced object might

// contain said dashes, but imagestream must cannot

// example: https://bugzilla.redhat.com/show_bug.cgi?id=1970805

name = strings.ReplaceAll(name, "/", "-")

At first I was going to argue that this is only the ImageStream issue, and other types of image references must be left intact, but after giving it a thought, I realized that you are right.
SuggestedName is used to generate names or part of names for internal k8s objects, and those names must follow the DNS standard, hence, no slashes. I think it is safe to assume we will never need slashes in any names here.

Do you think we might go even further and replace all the non[alphanumeric, dash or dot] symbols here?

So, basically replace this with a regexp that replaces all non-valid characters with dashes? I think that sounds like a great idea.

@coreydaley yeah, [^0-9a-zA-Z-.] should do the trick. From what I read, k8s names should go along with rfc1123, which references rfc952, which says, only alphanumeric, dots and dashes are allowed in a hostname. So, I thought it might be better to go with this more uniform approach rather than just slashes.

pkg/helpers/newapp/app/imageref.go

pkg/helpers/newapp/app/imageref_test.go

alicerum · 2021-09-09T08:20:40Z

/retest

coreydaley · 2021-09-09T11:25:38Z

pkg/helpers/newapp/app/imageref.go

+	switch {
+	case r.Stream != nil:
+		name = r.Stream.Name
+	case len(r.ObjectName) > 0:
+		name = r.ObjectName
+	case len(r.Reference.Name) > 0:
+		name = r.Reference.Name


+1, love seeing switch statements instead of multiple if or if/else

coreydaley · 2021-09-09T11:55:28Z

pkg/helpers/newapp/app/imageref.go

+)
+
+func init() {
+	dnsNamePattern = regexp.MustCompile("[^a-zA-Z0-9-.]")


https://github.com/kubernetes/apimachinery/blob/v0.22.1/pkg/util/validation/validation.go#L177-L196

ah, so no dots either. thanks.

coreydaley · 2021-09-09T17:36:31Z

/lgtm

coreydaley · 2021-09-09T17:36:58Z

/assign @adambkaplan

coreydaley · 2021-09-09T18:57:29Z

/retest

coreydaley · 2021-09-14T15:10:45Z

pkg/helpers/newapp/app/imageref.go

+var (
+	dnsNamePattern *regexp.Regexp
+)
+
+func init() {
+	dnsNamePattern = regexp.MustCompile("[^a-zA-Z0-9-]")
+}
+


Suggested change

var (

dnsNamePattern *regexp.Regexp

)

func init() {

dnsNamePattern = regexp.MustCompile("[^a-zA-Z0-9-]")

}

var (

dnsNamePattern *regexp.Regexp = regexp.MustCompile("[^a-zA-Z0-9-]")

)

I don't think you need this init here, the regexp can just be a var, unless there is a specific reason to use init that I am missing?

openshift-bot · 2021-09-24T01:03:38Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T02:51:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T04:15:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T05:39:16Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T08:16:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T08:54:40Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T10:25:39Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T13:14:41Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T13:51:40Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T15:03:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-24T16:03:45Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

adambkaplan · 2021-09-24T16:06:12Z

/hold

Something appears to be broken;

FAILURE after 1.000s: test/cmd/printer.sh:8: executing 'oc new-app openshift/nodejs' expecting success: the command returned the wrong error code
Standard output from the command:
warning: Cannot find git. Ensure that it is installed and in your path. Git is required to work with git repositories.

Standard error from the command:
W0924 14:21:58.794600     126 dockerimagelookup.go:300] container image remote registry lookup failed: you may not have access to the container image "docker.io/openshift/nodejs:latest"
error: multiple images or templates matched "openshift/nodejs"

Argument 'openshift/nodejs' was classified as an image, image~source, or loaded template reference.

The argument "openshift/nodejs" could apply to the following container images, OpenShift image streams, or templates:

* Template "nodejs-postgresql-example" in project "openshift"
  Use --template="openshift/nodejs-postgresql-example" to specify this image or template

* Template "nodejs-postgresql-persistent" in project "openshift"
  Use --template="openshift/nodejs-postgresql-persistent" to specify this image or template

* Image stream "jenkins-agent-nodejs" (tag "v4.0") in project "openshift"
  Use --image-stream="openshift/jenkins-agent-nodejs:v4.0" to specify this image or template

* Image stream "nodejs" in project "openshift"
  Use --image-stream="openshift/nodejs" --allow-missing-imagestream-tags to specify this image or template

alicerum · 2021-10-04T15:26:29Z

/retest

adambkaplan · 2021-10-06T17:39:46Z

/retest

adambkaplan · 2021-10-06T19:25:23Z

@soltysh looks like another broken test for e2e-agnostic-cmd

Running test/cmd/help.sh:22: executing 'oc adm ca' expecting failure and text 'Manage certificates'...
FAILURE after 0.000s: test/cmd/help.sh:22: executing 'oc adm ca' expecting failure and text 'Manage certificates': the output content test failed
There was no output from the command.
Standard error from the command:
error: unknown command "ca"
See 'oc adm -h' for help and examples
[ERROR] hack/lib/cmd.sh:41: `return "${return_code}"` exited with status 1.

alicerum · 2021-10-11T15:12:55Z

/retest

alicerum · 2021-10-11T18:36:32Z

/retest

alicerum · 2021-10-12T08:10:44Z

@adambkaplan finally test has passed

soltysh

/hold cancel

openshift-ci · 2021-10-13T19:57:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, alicerum, coreydaley, gabemontero, soltysh

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:

~~pkg/helpers/newapp/OWNERS~~ [adambkaplan,soltysh]

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

openshift-bot · 2021-10-13T23:06:05Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-13T23:32:01Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-14T01:42:02Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-14T02:21:02Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-14T06:01:35Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-14T06:13:34Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-10-14T08:05:41Z

@alicerum: All pull requests linked via external trackers have merged:

openshift/oc#922

Bugzilla bug 1970805 has been moved to the MODIFIED state.

In response to this:

Bug 1970805: Replace slashes in suggested ImageStream name

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.

openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 8, 2021

openshift-ci bot requested review from coreydaley and gabemontero September 8, 2021 10:36

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 8, 2021

openshift-ci bot requested a review from jitendar-singh September 8, 2021 11:01

gabemontero approved these changes Sep 8, 2021

View reviewed changes

openshift-ci bot assigned gabemontero Sep 8, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2021

coreydaley suggested changes Sep 8, 2021

View reviewed changes

alicerum force-pushed the bug-1970805 branch from f7df518 to 3f63f26 Compare September 8, 2021 21:26

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2021

coreydaley reviewed Sep 9, 2021

View reviewed changes

alicerum force-pushed the bug-1970805 branch from 3f63f26 to 2f53c08 Compare September 9, 2021 11:46

coreydaley reviewed Sep 9, 2021

View reviewed changes

alicerum force-pushed the bug-1970805 branch from 2f53c08 to e1add72 Compare September 9, 2021 12:00

openshift-ci bot assigned coreydaley Sep 9, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021

openshift-ci bot assigned adambkaplan Sep 9, 2021

coreydaley reviewed Sep 14, 2021

View reviewed changes

alicerum force-pushed the bug-1970805 branch from e1add72 to 39ec815 Compare September 15, 2021 10:03

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2021

soltysh approved these changes Oct 13, 2021

View reviewed changes

openshift-merge-robot merged commit 3f24f58 into openshift:master Oct 14, 2021

Bug 1970805: Replace slashes in suggested ImageStream name #922

Bug 1970805: Replace slashes in suggested ImageStream name #922

Conversation

alicerum commented Sep 8, 2021

openshift-ci bot commented Sep 8, 2021

alicerum commented Sep 8, 2021

openshift-ci bot commented Sep 8, 2021

gabemontero left a comment

Choose a reason for hiding this comment

alicerum commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicerum commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Sep 9, 2021

coreydaley commented Sep 9, 2021

coreydaley commented Sep 9, 2021

Choose a reason for hiding this comment

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

openshift-bot commented Sep 24, 2021

adambkaplan commented Sep 24, 2021

alicerum commented Oct 4, 2021

adambkaplan commented Oct 6, 2021

adambkaplan commented Oct 6, 2021

alicerum commented Oct 11, 2021

alicerum commented Oct 11, 2021

alicerum commented Oct 12, 2021

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 13, 2021

openshift-bot commented Oct 13, 2021

openshift-bot commented Oct 13, 2021

openshift-bot commented Oct 14, 2021

openshift-bot commented Oct 14, 2021

openshift-bot commented Oct 14, 2021

openshift-bot commented Oct 14, 2021

openshift-ci bot commented Oct 14, 2021