Remove `AllowAll` IDP dependency for extended and integration tests #11269

enj · 2016-10-07T19:16:09Z

enj · 2016-10-07T19:16:40Z

liggitt · 2016-10-07T19:19:14Z

test/util/client.go

@@ -64,52 +65,59 @@ func GetClusterAdminClientConfig(adminKubeConfigFile string) (*restclient.Config
 }

 func GetClientForUser(clientConfig restclient.Config, username string) (*client.Client, *kclient.Client, *restclient.Config, error) {


add a first param of adminClient *client.Client instead of assuming the clientConfig is both the admin config and the template for the user config

liggitt · 2016-10-07T19:20:39Z

test/util/client.go

-	if _, _, _, err := GetClientForUser(clientConfig, username); err != nil {
-		return nil, nil, nil, err
+func getOrCreateUser(adminClient *client.Client, username string) (*userapi.User, error) {
+	user, err := adminClient.Users().Create(&userapi.User{ObjectMeta: kapi.ObjectMeta{Name: username}})


user, err := ... create if err == nil { return user, nil } if kerrs.IsAlreadyExists(err) { return adminClient.Users().Get(username) } return nil, err

liggitt · 2016-10-07T19:21:45Z

test/util/client.go

-		return nil, nil, nil, err
+
+	var err error
+	token, err = adminClient.OAuthAccessTokens().Create(token)


return adminClient.OAuthAccessTokens().Create(token)

liggitt · 2016-10-07T19:22:31Z

test/util/client.go

+	return token, nil
+}
+
+func getScopedClientForConfig(clientConfig *restclient.Config, token string) (*client.Client, *kclient.Client, *restclient.Config, error) {


better name, this doesn't have anything to do with scopes

brenton · 2016-10-07T19:35:35Z

Thanks for looking at this @enj!

Would it be possible to have the tests remove the users when the tests terminate as part of this PR?

enj · 2016-10-07T22:13:11Z

[test]

enj · 2016-10-07T22:23:06Z

While I think it is worthwhile to remove the dependency on the AllowAll IDP, I do not think it is valid to suggest that users actually run our test suite on a real cluster. The tests essentially use the cluster as an ephemeral scratchpad. I am not even sure if we cleanup the test namespaces.

Conformance tests are useful in some running clusters however admin can't always want to disable authentication

Useful how? The tests validate our code, not our customer's cluster's health.

Would it be possible to have the tests remove the users when the tests terminate as part of this PR?

This could delete valid users just as easily as it would delete the test users.

enj · 2016-10-11T00:32:09Z

re[test] flake #11094 #10162

enj · 2016-10-11T00:33:14Z

@liggitt minus the flakes this seems to working. I may have been overzealous with the refactor though...

brenton · 2016-10-11T13:27:51Z

@enj , To clean up projects and users we could probably improve our logging to make it easy for admins to run a cleanup job afterwards.

While the conformance tests are primarily concerned with validating OpenShift/Kube code itself. They do exercise a fair amount of the environment. Many admins have expressed the need to be able to produce large numbers of builds and deployments to validate their infrastructure changes. Eg, post upgrade they like to know if something is broken rather than waiting for their users to report it.

enj · 2016-10-11T13:37:46Z

The problem with using these tests is that any developer at any point could add a test that does oc delete all --all (for example, if someone asked them to implement cleanup logic after the tests). IMO some other form of test is needed to validate cluster health (and ideally it would be something that either ran in some type of --dry-run mode or some other isolated manner).

brenton · 2016-10-11T13:40:53Z

@enj, Ahh, I see your concern. For what it's worth, we certainly are going to review the subset of tests that customers would be allowed to run. There would be another wrapper script to explicitly run certain tests. QE would even validate this wrapper script doesn't break a running environment before we would ever ship them in the product.

enj · 2016-10-11T14:59:56Z

@brenton IMO this is the wrong approach, especially with the amount of work that is being proposed via QE validation. I believe a better way to go about it would be to make an entirely separate repository (something like openshift/stress-test). This repo would house the tooling needed specifically for this use case, and could provide a nice CLI interface to the cluster admin. Anyone committing to repo would understand the need for caution in all changes. Some important features of such tooling could be:

Keeping track of all resources created in a fault tolerant way
Allowing the admin to constrain how resources are created (name prefixes, resource kind, rate of creation, etc)
Extremely cautious clean up code

@liggitt thoughts?

brenton · 2016-10-11T15:12:12Z

If someone had time to do that I don't deny it would be ideal. My understanding is that our own performance team is also using the Conformance tests to generate load today. They are another team that has requested being able to run these tests on an environment that has authentication configured (though they were not concerned with the upgrade scenario, only fresh installs of OCP).

brenton · 2016-10-11T15:14:28Z

I don't entirely see the need for an entirely new repository though. Perhaps a new directory in Origin with a README. Otherwise there's a little more overhear maintaining it along with new PRs that come in.

liggitt · 2017-01-14T01:05:23Z

test/integration/authorization_test.go

@@ -1191,7 +1191,7 @@ func TestOldLocalSubjectAccessReviewEndpoint(t *testing.T) {
 			},
 		}
 		actualResponse := &authorizationapi.SubjectAccessReviewResponse{}
-		err := haroldClient.Post().Namespace(namespace).Resource("subjectAccessReviews").Body(sar).Do().Into(actualResponse)
+		err := haroldClient.(*client.Client).Post().Namespace(namespace).Resource("subjectAccessReviews").Body(sar).Do().Into(actualResponse)


why are these casts needed?

Because I changed the functions to return the client interface instead of the raw client. I should be able to get rid of the casts by implementing anything missing.

liggitt · 2017-01-14T01:15:35Z

test/util/client.go

 	token := &oauthapi.OAuthAccessToken{
-		ObjectMeta:  kapi.ObjectMeta{Name: fmt.Sprintf("%s-token-plus-some-padding-here-to-make-the-limit-%d", username, rand.Int())},
+		ObjectMeta:  kapi.ObjectMeta{Name: fmt.Sprintf("%s-token-plus-some-padding-here-to-make-the-limit-%d", user.Name, rand.Int())},


auto-creating a token named like this is just one of the many reasons this should never be run on a production cluster

liggitt · 2017-01-14T01:17:01Z

most of this seems fine, though I wouldn't merge it until we could get rid of the type casts...

@brenton, the assumptions that random usernames are available to run tests as, the destruction of random namespaces, and the generation of insecure tokens are all reasons these tests shouldn't run against a production server

brenton · 2017-01-16T13:12:35Z

@liggitt, that definitely makes sense given the current state of things. I still hear a lot of people wishing they had a suite of tests to run on production environments and they are going to have many of the same bootstrapping challenges as the e2e tests. Does it seem reasonable to reserve certain resources as test only? Take the example.com domain. They built that in to the dns spec for the purposes of testing.

enj · 2017-01-19T19:45:14Z

test/integration/authorization_test.go

+	if err != nil {
+		return nil, err
+	}
+	rawClient, ok := adminClient.(*client.Client)


@liggitt This is the only type cast I was not able to remove without adding a bunch of code.

enj · 2017-01-19T19:46:58Z

test/util/client.go

+}
+
+// do not use this func unless you need to do raw REST or other low level operations
+func GetClusterAdminClientRaw(adminKubeConfigFile string) (*client.Client, error) {


@liggitt This allowed me to get around the other type casts in the places where we needed the raw client.

enj · 2017-01-19T19:48:30Z

@liggitt This is ready assuming the tests pass.

Signed-off-by: Monis Khan <mkhan@redhat.com>

openshift-bot · 2017-01-20T15:37:47Z

Evaluated for origin test up to c6dd940

openshift-bot · 2017-01-20T17:05:50Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13092/) (Base Commit: 9a006a8)

enj · 2017-01-20T17:26:55Z

Flake on #12558

openshift-bot · 2017-06-16T08:39:43Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

enj · 2017-06-20T08:58:21Z

@soltysh did you ever end up doing anything with this?

soltysh · 2017-06-20T13:03:28Z

@enj nope I didn't do anything with it, this is the first time I hear about it.

enj · 2017-06-21T01:09:01Z

@soltysh I just remember you wanting to switch tests to use the client interface, which I had started to do here.

soltysh · 2017-06-21T07:31:15Z

Oh that, hmm... that is still on my todo list ;)

enj assigned liggitt Oct 7, 2016

liggitt reviewed Oct 7, 2016

View reviewed changes

enj mentioned this pull request Oct 12, 2016

Remove -a flag from hack/test-integration.sh #11341

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 28, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 17, 2016

liggitt reviewed Jan 14, 2017

View reviewed changes

enj force-pushed the enj/i/et_explicit_user_token/11255 branch from 6754be3 to c07d082 Compare January 19, 2017 19:38

enj removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2017

enj commented Jan 19, 2017

View reviewed changes

Remove AllowAll IDP dependency for extended and integration tests

c6dd940

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj force-pushed the enj/i/et_explicit_user_token/11255 branch from 29076b2 to c6dd940 Compare January 20, 2017 15:36

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 8, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 28, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 6, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 6, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 14, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2017

smarterclayton closed this Jun 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `AllowAll` IDP dependency for extended and integration tests #11269

Remove `AllowAll` IDP dependency for extended and integration tests #11269

enj commented Oct 7, 2016

enj commented Oct 7, 2016

liggitt Oct 7, 2016

liggitt Oct 7, 2016

liggitt Oct 7, 2016

liggitt Oct 7, 2016

brenton commented Oct 7, 2016

enj commented Oct 7, 2016

enj commented Oct 7, 2016

enj commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

brenton commented Oct 11, 2016

liggitt Jan 14, 2017

enj Jan 14, 2017

liggitt Jan 14, 2017

liggitt commented Jan 14, 2017

brenton commented Jan 16, 2017

enj Jan 19, 2017

enj Jan 19, 2017

enj commented Jan 19, 2017

openshift-bot commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

enj commented Jan 20, 2017

openshift-bot commented Jun 16, 2017

enj commented Jun 20, 2017

soltysh commented Jun 20, 2017

enj commented Jun 21, 2017

soltysh commented Jun 21, 2017

		@@ -64,52 +65,59 @@ func GetClusterAdminClientConfig(adminKubeConfigFile string) (*restclient.Config
		}

		func GetClientForUser(clientConfig restclient.Config, username string) (client.Client, kclient.Client, *restclient.Config, error) {

Remove AllowAll IDP dependency for extended and integration tests #11269

Remove AllowAll IDP dependency for extended and integration tests #11269

Conversation

enj commented Oct 7, 2016

enj commented Oct 7, 2016

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

brenton commented Oct 7, 2016

enj commented Oct 7, 2016

enj commented Oct 7, 2016

enj commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

enj commented Oct 11, 2016

brenton commented Oct 11, 2016

brenton commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 14, 2017

brenton commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Jan 19, 2017

openshift-bot commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

enj commented Jan 20, 2017

openshift-bot commented Jun 16, 2017

enj commented Jun 20, 2017

soltysh commented Jun 20, 2017

enj commented Jun 21, 2017

soltysh commented Jun 21, 2017

Remove `AllowAll` IDP dependency for extended and integration tests #11269

Remove `AllowAll` IDP dependency for extended and integration tests #11269