tweaks to get tests to work with private registry #1870

duglin · 2019-09-11T17:22:25Z

Signed-off-by: Doug Davis dug@us.ibm.com

Proposed Changes

While trying to get the e2e tests to work with a private registry I had to make some "interesting"
choices. This PR isn't all of them, but they're most of the ones I can share at this time. They include:

when creating the various "cluster roles", if the role already exists then we just ignore the error. TBH, I'm not sure why other people are not seeing this, perhaps my env already has them by default, or perhaps they're left around from a previous run and never got cleaned up, but either way, I don't think this should be a test-stopping error condition.
I added code to look for a secret in the default namespace called
kn-eventing-test-pull-secret when creating new namespaces and service accounts. And
if it exists, we will now inject that secret as an ImagePullSecret into the namespace's
default service accounts, or the new service account.

I'm not 100% sure these are the best solutions, but they at least worked for me. I'm open to
other ideas for how to get the PullSecret injected. If the PullSecret approach sounds out
I can then update the test docs to mention it.

Note - this does not get around #1862

Release Note

NONE

Signed-off-by: Doug Davis <dug@us.ibm.com>

chizhg · 2019-09-11T17:38:50Z

/uncc @chaodaiG
/cc @grantr

vaikas · 2019-09-11T22:35:46Z

test/common/creation.go

 }

 // CreateClusterRoleOrFail creates the given ClusterRole or fail the test if there is an error.
 func (client *Client) CreateClusterRoleOrFail(cr *rbacv1.ClusterRole) {
 	crs := client.Kube.Kube.RbacV1().ClusterRoles()
 	if _, err := crs.Create(cr); err != nil {
-		client.T.Fatalf("Failed to create cluster role %q: %v", cr.Name, err)
+		if !strings.Contains(err.Error(), "already exists") {


Could you change these to use:
"k8s.io/apimachinery/pkg/api/errors"
and in particular IsNotFound()

I was looking for that - thanks!

vaikas · 2019-09-11T22:38:10Z

test/common/test_runner.go

+
+		testSecret, _ := defSecI.Get(TestPullSecretName, metav1.GetOptions{})
+
+		// Check again. I've seen cases where it lies and if we need it


this seems odd, and is there a reason why you don't check the err returned? I'm not sure what you mean by "It" lies, are you saying above line returns nil, but the one below returns true?

No I mean that the apiserver will return nil+err even though the secret does exist. I can't explain why or how, but I ran into it often enough that I decided to just ask again to be sure.

@vagababov Have you seen something like this before? Seems b0rk3n?

Not really. What is the actual error you're getting? If I were to guess you got either network problem error (connection refused, dial timeout) or overload error from API server.

Which leads me to the main problem I see here: "never ignore errors".

Also, I'd highly recommend to switch to informers.

What's the err that's returned?

Let me change the code to show the error (if I even get one) to see what's up.. But to add more intrigue, I've also seen cases like this....

I had this code:

if testSecret != nil { // Found the secret, so now make a copy in our new namespace newSecret, err := nsSecI.Create( &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: testSecret.ObjectMeta.Name, }, Data: testSecret.Data, Type: testSecret.Type, }) if err != nil { t.Fatalf("TestSetup: Error copying the secret: %s", err) }

Notice I would use the "name" from "testSecret", which should never be empty. Yet I would get this error:

test_runner.go:82: namespace is : "test-default-broker-with-many-deprecated-triggers" test_runner.go:168: TestSetup: Error copying the secret: Secret "" is invalid: metadata.name: Required value: name or generateName is required

I never saw this on my local testing, or IKS, only using Prow.

vaikas · 2019-09-11T22:52:16Z

test/common/test_runner.go

+			testSecret, _ = defSecI.Get(TestPullSecretName, metav1.GetOptions{})
+		}
+
+		if testSecret != nil {


This function seems similar in description, yet it's different and I'm trying to understand why? :) Can this be hoisted into a separate function and reused instead of having two versions of it.

they are similar but different. This one only deals with the "default" SA and knows it can blindly copy the secret. Th other one deals with a new SA (so not "default") and needs to check to see if the secret already exists so it doesn't try to duplicate it.

They both have the check and re-check (which I must say is extremely worrying and seems extremely flaky and a bug in the k8s if that's the case).
I think having a method that deals with copying the secret into the namespace would be good to hoist out, then rejiggering the ServiceAccount secret could be the only difference between the two if I understand correctly?

duglin · 2019-09-12T00:44:40Z

/test pull-knative-eventing-integration-tests

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2019-09-12T18:57:23Z

@vaikas-google are you ok with the general direction I went for the pull secrets? if so, I'd like to update the docs as part of this PR before it gets merged.

vagababov · 2019-09-13T18:26:41Z

test/common/test_runner.go

+			testSecret, _ = defSecI.Get(TestPullSecretName, metav1.GetOptions{})
+		}
+
+		if testSecret != nil {


Reverse the flow to be more go-style

if tS == nil { fail/return/break} // normal code goes here unindented

duglin · 2019-09-13T20:20:13Z

/hold

until I do more testing

vaikas · 2019-09-13T20:31:50Z

So from the test logs it's indeed failing with 'not found':
test_runner.go:83: namespace is : "test-default-broker-with-many-deprecated-triggers"
test_runner.go:201: Error copying the secret into ns "test-default-broker-with-many-deprecated-triggers": secrets "kn-eventing-test-pull-secret" not found

Where is this secret created, is there a race there somewhere?

duglin · 2019-09-13T20:36:21Z

those logs were old - it shouldn't print that "not found" error in Prow. That should be fixed now, but I'm testing locally, where I do have the secret created, to see if it does the copy correctly.

Signed-off-by: Doug Davis <dug@us.ibm.com>

vagababov · 2019-09-14T17:56:57Z

test/common/test_runner.go

+
+	// Just double checking
+	if srcSecret == nil {
+		return nil, fmt.Errorf("Error copying secret, it's nil w/o error")


no formatting errors.New.

vagababov · 2019-09-14T17:57:49Z

test/common/test_runner.go

+// it.
+// It'll either return a pointer to the new Secret or and error indicating
+// why it couldn't do it.
+func CopySecret(client *Client, srcNS string, srcSecretName string, tgtNS string, svcAccount string) (*corev1.Secret, error) {


The function is inconsistent. Sometimes it calls Fatalf sometimes returns errors (should always do second, though).

yep missed that - fixed

Signed-off-by: Doug Davis <dug@us.ibm.com>

vaikas · 2019-09-16T21:51:53Z

/approve

leaving lgtm for @vagababov in case he had anything else.

knative-prow-robot · 2019-09-16T21:52:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: duglin, vaikas-google

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:

~~test/OWNERS~~ [vaikas-google]

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

vagababov

Well, since you asked for a real review, I'll leave some obligatory nits :-)

vagababov · 2019-09-16T22:06:29Z

test/common/test_runner.go

@@ -17,6 +17,7 @@ limitations under the License.
 package common

 import (
+	goerrs "errors"


I'd rather we alias k8s package rather than built in, but 🤷‍♂

yeah I thought about that but decided to keep the amount of changes lower - but I'll do it

vagababov · 2019-09-16T22:06:50Z

test/common/test_runner.go

+		_, err = tgtNSSvcAccI.Patch(svcAccount, types.StrategicMergePatchType,
+			[]byte(`{"imagePullSecrets":[{"name":"`+srcSecretName+`"}]}`))
+		if err != nil {
+			return nil, fmt.Errorf("Patch failed on NS/SA (%s/%s): %s",


Suggested change

return nil, fmt.Errorf("Patch failed on NS/SA (%s/%s): %s",

return nil, fmt.Errorf("patch failed on NS/SA (%s/%s): %s",

vagababov · 2019-09-16T22:06:55Z

test/common/test_runner.go

+	// If the secret already exists then that's ok - some other test
+	// must have created it
+	if err != nil && !errors.IsAlreadyExists(err) {
+		return nil, fmt.Errorf("Error copying the Secret: %s", err)


Suggested change

return nil, fmt.Errorf("Error copying the Secret: %s", err)

return nil, fmt.Errorf("frror copying the Secret: %s", err)

See: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

I think I'll go with error instead of frror :-)

vagababov · 2019-09-16T22:07:02Z

test/common/test_runner.go

+
+	// Just double checking
+	if srcSecret == nil {
+		return nil, goerrs.New("Error copying Secret, it's nil w/o error")


Suggested change

return nil, goerrs.New("Error copying Secret, it's nil w/o error")

return nil, goerrs.New("error copying Secret, it's nil w/o error")

vagababov · 2019-09-16T22:07:48Z

test/common/test_runner.go

+		// This is needed for cases where the images are in a private registry.
+		_, err := CopySecret(client, "default", TestPullSecretName, namespace, "default")
+		if err != nil && !errors.IsNotFound(err) {
+			t.Fatalf("Error copying the secret into ns %q: %s", namespace, err)


Suggested change

t.Fatalf("Error copying the secret into ns %q: %s", namespace, err)

t.Fatalf("error copying the secret into ns %q: %s", namespace, err)

vagababov · 2019-09-16T22:08:31Z

test/common/test_runner.go

@@ -38,6 +40,8 @@ import (
 	_ "k8s.io/client-go/plugin/pkg/client/auth/oidc"
 )

+var TestPullSecretName = "kn-eventing-test-pull-secret"


Suggested change

var TestPullSecretName = "kn-eventing-test-pull-secret"

const TestPullSecretName = "kn-eventing-test-pull-secret"

vagababov

/lgtm
Thanks.

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2019-09-17T19:43:37Z

forgot to re-ping after my latest push. I think all comments are addressed.
@vaikas-google @vagababov

vagababov

/lgtm

chizhg · 2019-09-17T20:57:35Z

Not sure if you'll have to manually merge this change on master - #1898

/retest

chizhg · 2019-09-17T21:20:25Z

/retest

chizhg · 2019-09-17T23:37:43Z

/retest

tweaks to get tests to work with private registry

4c14a75

Signed-off-by: Doug Davis <dug@us.ibm.com>

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 11, 2019

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Sep 11, 2019

knative-prow-robot requested review from chaodaiG and chizhg September 11, 2019 17:22

knative-prow-robot requested review from grantr and removed request for chaodaiG September 11, 2019 17:38

vaikas reviewed Sep 11, 2019

View reviewed changes

duglin force-pushed the hackTests branch 2 times, most recently from e40f7ff to 6844dad Compare September 12, 2019 00:20

use errors apis

02c4138

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the hackTests branch from 6844dad to 02c4138 Compare September 12, 2019 01:02

vagababov reviewed Sep 13, 2019

View reviewed changes

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2019

duglin force-pushed the hackTests branch 3 times, most recently from 0cb93f7 to 8280bfd Compare September 13, 2019 20:00

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2019

duglin force-pushed the hackTests branch from 8280bfd to 809da74 Compare September 13, 2019 20:27

duglin force-pushed the hackTests branch from 809da74 to 2e8a1fe Compare September 13, 2019 23:05

shift some stuff around and add some debugging

55c4fc0

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the hackTests branch from 2e8a1fe to 55c4fc0 Compare September 14, 2019 01:01

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2019

vagababov reviewed Sep 14, 2019

View reviewed changes

duglin force-pushed the hackTests branch 5 times, most recently from 3f0d474 to 4a5b5fc Compare September 14, 2019 23:47

review tweaks

45b0732

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the hackTests branch from 4a5b5fc to 45b0732 Compare September 15, 2019 17:12

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2019

vagababov reviewed Sep 16, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2019

vagababov reviewed Sep 16, 2019

View reviewed changes

knative-prow-robot assigned vagababov Sep 16, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019

duglin force-pushed the hackTests branch from 9f77c24 to d344d29 Compare September 16, 2019 22:24

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019

minor nits

23619ef

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the hackTests branch from d344d29 to 23619ef Compare September 16, 2019 22:30

vagababov reviewed Sep 17, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2019

knative-prow-robot merged commit 72ba863 into knative:master Sep 17, 2019

duglin deleted the hackTests branch September 18, 2019 08:11


		testSecret, _ := defSecI.Get(TestPullSecretName, metav1.GetOptions{})

		// Check again. I've seen cases where it lies and if we need it

	return nil, fmt.Errorf("Patch failed on NS/SA (%s/%s): %s",
	return nil, fmt.Errorf("patch failed on NS/SA (%s/%s): %s",

	return nil, fmt.Errorf("Error copying the Secret: %s", err)
	return nil, fmt.Errorf("frror copying the Secret: %s", err)

	return nil, goerrs.New("Error copying Secret, it's nil w/o error")
	return nil, goerrs.New("error copying Secret, it's nil w/o error")

	t.Fatalf("Error copying the secret into ns %q: %s", namespace, err)
	t.Fatalf("error copying the secret into ns %q: %s", namespace, err)

	var TestPullSecretName = "kn-eventing-test-pull-secret"
	const TestPullSecretName = "kn-eventing-test-pull-secret"

tweaks to get tests to work with private registry #1870

tweaks to get tests to work with private registry #1870

Conversation

duglin commented Sep 11, 2019

Proposed Changes

chizhg commented Sep 11, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Sep 12, 2019

duglin commented Sep 12, 2019

Choose a reason for hiding this comment

duglin commented Sep 13, 2019

vaikas commented Sep 13, 2019

duglin commented Sep 13, 2019

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

vaikas commented Sep 16, 2019

knative-prow-robot commented Sep 16, 2019

vagababov left a comment

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

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

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

Choose a reason for hiding this comment

vagababov left a comment

Choose a reason for hiding this comment

duglin commented Sep 17, 2019

vagababov left a comment

Choose a reason for hiding this comment

chizhg commented Sep 17, 2019

chizhg commented Sep 17, 2019

chizhg commented Sep 17, 2019