Merge pull request #16849 from enj/enj/i/new_project_admin_sar/16716

Automatic merge from submit-queue.

Make admin project creation wait for SAR

This change adds a SAR check to direct project creation to ensure that the designated user can get the project which was created for them.  It also updates the project integration tests to be more tolerant of the project ACL being out of date.  This race condition became more apparent as we moved to the generated clients since those clients were smaller and had their own rate limiters (instead of a one big client that could do everything and shared the same rate limiter).  Since the new clients would perform actions at a faster pace, the race against the project ACL would occur more frequently.

Signed-off-by: Monis Khan <[email protected]>

Fixes #16716

/kind bug

/assign @smarterclayton @simo5 @deads2k

Author: openshift-merge-robot

pkg/oc/admin/project/new_project.go

@@ -4,16 +4,19 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"time"
 
 	"github.com/spf13/cobra"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	errorsutil "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 
 	oapi "github.com/openshift/origin/pkg/api"
+	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
 	authorizationtypedclient "github.com/openshift/origin/pkg/authorization/generated/internalclientset/typed/authorization/internalversion"
 	authorizationregistryutil "github.com/openshift/origin/pkg/authorization/registry/util"
 	"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
@@ -33,6 +36,7 @@ type NewProjectOptions struct {
 
 	ProjectClient     projectclient.ProjectInterface
 	RoleBindingClient authorizationtypedclient.RoleBindingsGetter
+	SARClient         authorizationtypedclient.SubjectAccessReviewInterface
 
 	AdminRole string
 	AdminUser string
@@ -93,7 +97,9 @@ func (o *NewProjectOptions) complete(f *clientcmd.Factory, args []string) error
 	if err != nil {
 		return err
 	}
-	o.RoleBindingClient = authorizationClient.Authorization()
+	authorizationInterface := authorizationClient.Authorization()
+	o.RoleBindingClient = authorizationInterface
+	o.SARClient = authorizationInterface.SubjectAccessReviews()
 
 	return nil
 }
@@ -133,6 +139,27 @@ func (o *NewProjectOptions) Run(useNodeSelector bool) error {
 		if err := adduser.AddRole(); err != nil {
 			fmt.Printf("%v could not be added to the %v role: %v\n", o.AdminUser, o.AdminRole, err)
 			errs = append(errs, err)
+		} else {
+			if err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
+				resp, err := o.SARClient.Create(&authorizationapi.SubjectAccessReview{
+					Action: authorizationapi.Action{
+						Namespace: o.ProjectName,
+						Verb:      "get",
+						Resource:  "projects",
+					},
+					User: o.AdminUser,
+				})
+				if err != nil {
+					return false, err
+				}
+				if !resp.Allowed {
+					return false, nil
+				}
+				return true, nil
+			}); err != nil {
+				fmt.Printf("%s is not able to get project %s with the %s role: %v\n", o.AdminUser, o.ProjectName, o.AdminRole, err)
+				errs = append(errs, err)
+			}
 		}
 	}
 

test/integration/login_test.go

@@ -47,10 +47,12 @@ func TestLogin(t *testing.T) {
 	if loginOptions.Username != username {
 		t.Fatalf("Unexpected user after authentication: %#v", loginOptions)
 	}
+	authorizationInterface := authorizationclient.NewForConfigOrDie(clusterAdminClientConfig).Authorization()
 
 	newProjectOptions := &newproject.NewProjectOptions{
 		ProjectClient:     projectclient.NewForConfigOrDie(clusterAdminClientConfig).Project(),
-		RoleBindingClient: authorizationclient.NewForConfigOrDie(clusterAdminClientConfig),
+		RoleBindingClient: authorizationInterface,
+		SARClient:         authorizationInterface.SubjectAccessReviews(),
 		ProjectName:       project,
 		AdminRole:         bootstrappolicy.AdminRoleName,
 		AdminUser:         username,

test/integration/project_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/openshift/origin/pkg/oc/admin/policy"
 	projectapi "github.com/openshift/origin/pkg/project/apis/project"
 	projectclient "github.com/openshift/origin/pkg/project/generated/internalclientset"
+	projectinternalversion "github.com/openshift/origin/pkg/project/generated/internalclientset/typed/project/internalversion"
 	testutil "github.com/openshift/origin/test/util"
 	testserver "github.com/openshift/origin/test/util/server"
 )
@@ -467,25 +468,15 @@ func TestScopedProjectAccess(t *testing.T) {
 	}
 	waitForOnlyAdd("four", allWatch, t)
 
-	oneTwoProjects, err := oneTwoBobClient.Projects().List(metav1.ListOptions{})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if err := hasExactlyTheseProjects(oneTwoProjects, sets.NewString("one", "two")); err != nil {
+	if err := hasExactlyTheseProjects(oneTwoBobClient.Projects(), sets.NewString("one", "two")); err != nil {
 		t.Error(err)
 	}
-	twoThreeProjects, err := twoThreeBobClient.Projects().List(metav1.ListOptions{})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if err := hasExactlyTheseProjects(twoThreeProjects, sets.NewString("two", "three")); err != nil {
+
+	if err := hasExactlyTheseProjects(twoThreeBobClient.Projects(), sets.NewString("two", "three")); err != nil {
 		t.Error(err)
 	}
-	allProjects, err := allBobClient.Projects().List(metav1.ListOptions{})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if err := hasExactlyTheseProjects(allProjects, sets.NewString("one", "two", "three", "four")); err != nil {
+
+	if err := hasExactlyTheseProjects(allBobClient.Projects(), sets.NewString("one", "two", "three", "four")); err != nil {
 		t.Error(err)
 	}
 
@@ -589,15 +580,11 @@ func TestInvalidRoleRefs(t *testing.T) {
 	}
 
 	// Make sure bob still sees his project (and only his project)
-	if projects, err := projectclient.NewForConfigOrDie(bobConfig).Projects().List(metav1.ListOptions{}); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	} else if hasErr := hasExactlyTheseProjects(projects, sets.NewString("foo")); hasErr != nil {
+	if hasErr := hasExactlyTheseProjects(projectclient.NewForConfigOrDie(bobConfig).Projects(), sets.NewString("foo")); hasErr != nil {
 		t.Error(hasErr)
 	}
 	// Make sure alice still sees her project (and only her project)
-	if projects, err := projectclient.NewForConfigOrDie(aliceConfig).Projects().List(metav1.ListOptions{}); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	} else if hasErr := hasExactlyTheseProjects(projects, sets.NewString("bar")); hasErr != nil {
+	if hasErr := hasExactlyTheseProjects(projectclient.NewForConfigOrDie(aliceConfig).Projects(), sets.NewString("bar")); hasErr != nil {
 		t.Error(hasErr)
 	}
 	// Make sure cluster admin still sees all projects
@@ -614,14 +601,26 @@ func TestInvalidRoleRefs(t *testing.T) {
 	}
 }
 
-func hasExactlyTheseProjects(list *projectapi.ProjectList, projects sets.String) error {
-	if len(list.Items) != len(projects) {
-		return fmt.Errorf("expected %v, got %v", projects, list.Items)
-	}
-	for _, project := range list.Items {
-		if !projects.Has(project.Name) {
-			return fmt.Errorf("expected %v, got %v", projects, list.Items)
+func hasExactlyTheseProjects(lister projectinternalversion.ProjectResourceInterface, projects sets.String) error {
+	var lastErr error
+	if err := wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) {
+		list, err := lister.List(metav1.ListOptions{})
+		if err != nil {
+			return false, err
 		}
+		if len(list.Items) != len(projects) {
+			lastErr = fmt.Errorf("expected %v, got %v", projects.List(), list.Items)
+			return false, nil
+		}
+		for _, project := range list.Items {
+			if !projects.Has(project.Name) {
+				lastErr = fmt.Errorf("expected %v, got %v", projects.List(), list.Items)
+				return false, nil
+			}
+		}
+		return true, nil
+	}); err != nil {
+		return fmt.Errorf("hasExactlyTheseProjects failed with %v and %v", err, lastErr)
 	}
 	return nil
 }

test/util/server/server.go

@@ -617,10 +617,12 @@ func CreateNewProject(clientConfig *restclient.Config, projectName, adminUser st
 	if err != nil {
 		return nil, nil, err
 	}
+	authorizationInterface := authorizationClient.Authorization()
 
 	newProjectOptions := &newproject.NewProjectOptions{
 		ProjectClient:     projectClient,
-		RoleBindingClient: authorizationClient.Authorization(),
+		RoleBindingClient: authorizationInterface,
+		SARClient:         authorizationInterface.SubjectAccessReviews(),
 		ProjectName:       projectName,
 		AdminRole:         bootstrappolicy.AdminRoleName,
 		AdminUser:         adminUser,