wait for group cache in templateinstance tests

pkg/template/registry/templateinstance/strategy.go

@@ -3,7 +3,9 @@ package templateinstance
 import (
 	"errors"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	kutilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
@@ -96,8 +98,34 @@ func (s *templateInstanceStrategy) ValidateUpdate(ctx apirequest.Context, obj, o
 		return field.ErrorList{field.InternalError(field.NewPath(""), errors.New("user not found in context"))}
 	}
 
-	templateInstance := obj.(*templateapi.TemplateInstance)
-	oldTemplateInstance := old.(*templateapi.TemplateInstance)
+	// Decode Spec.Template.Objects on both obj and old to Unstructureds.  This
+	// allows detectection of at least some cases where the Objects are
+	// semantically identical, but the serialisations have been jumbled up.  One
+	// place where this happens is in the garbage collector, which uses
+	// Unstructureds via the dynamic client.
+
+	objcopy, err := kapi.Scheme.DeepCopy(obj)
+	if err != nil {
+		return field.ErrorList{field.InternalError(field.NewPath(""), err)}
+	}
+	templateInstance := objcopy.(*templateapi.TemplateInstance)
+
+	errs := runtime.DecodeList(templateInstance.Spec.Template.Objects, unstructured.UnstructuredJSONScheme)
+	if len(errs) != 0 {
+		return field.ErrorList{field.InternalError(field.NewPath(""), kutilerrors.NewAggregate(errs))}
+	}
+
+	oldcopy, err := kapi.Scheme.DeepCopy(old)
+	if err != nil {
+		return field.ErrorList{field.InternalError(field.NewPath(""), err)}
+	}
+	oldTemplateInstance := oldcopy.(*templateapi.TemplateInstance)
+
+	errs = runtime.DecodeList(oldTemplateInstance.Spec.Template.Objects, unstructured.UnstructuredJSONScheme)
+	if len(errs) != 0 {
+		return field.ErrorList{field.InternalError(field.NewPath(""), kutilerrors.NewAggregate(errs))}
+	}
+
 	allErrs := validation.ValidateTemplateInstanceUpdate(templateInstance, oldTemplateInstance)
 	allErrs = append(allErrs, s.validateImpersonationUpdate(templateInstance, oldTemplateInstance, user)...)
 

test/extended/templates/templateinstance_cross_namespace.go

@@ -112,28 +112,24 @@ var _ = g.Describe("[Conformance][templates] templateinstance cross-namespace te
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		g.By("deleting the templateinstance")
-		err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, nil)
+		foreground := metav1.DeletePropagationForeground
+		err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, &metav1.DeleteOptions{PropagationPolicy: &foreground})
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		// wait for garbage collector to do its thing
-		err = wait.Poll(time.Second, time.Minute, func() (bool, error) {
+		err = wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
 			_, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Get(templateinstance.Name, metav1.GetOptions{})
-			if err == nil || !kerrors.IsNotFound(err) {
-				return false, err
+			if kerrors.IsNotFound(err) {
+				return true, nil
 			}
-
-			_, err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Get("secret1", metav1.GetOptions{})
-			if err == nil || !kerrors.IsNotFound(err) {
-				return false, err
-			}
-
-			_, err = cli.KubeClient().CoreV1().Secrets(cli2.Namespace()).Get("secret2", metav1.GetOptions{})
-			if err == nil || !kerrors.IsNotFound(err) {
-				return false, err
-			}
-
-			return true, nil
+			return false, err
 		})
 		o.Expect(err).NotTo(o.HaveOccurred())
+
+		_, err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Get("secret1", metav1.GetOptions{})
+		o.Expect(kerrors.IsNotFound(err)).To(o.BeTrue())
+
+		_, err = cli.KubeClient().CoreV1().Secrets(cli2.Namespace()).Get("secret2", metav1.GetOptions{})
+		o.Expect(kerrors.IsNotFound(err)).To(o.BeTrue())
 	})
 })

test/extended/templates/templateinstance_impersonation.go

@@ -1,13 +1,16 @@
 package templates
 
 import (
+	"time"
+
 	g "github.com/onsi/ginkgo"
 	o "github.com/onsi/gomega"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/wait"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/retry"
 
@@ -53,17 +56,91 @@ var _ = g.Describe("[Conformance][templates] templateinstance impersonation test
 	)
 
 	g.BeforeEach(func() {
-		var err error
-
 		adminuser = createUser(cli, "adminuser", bootstrappolicy.AdminRoleName)
 		impersonateuser = createUser(cli, "impersonateuser", bootstrappolicy.EditRoleName)
 		impersonatebygroupuser = createUser(cli, "impersonatebygroupuser", bootstrappolicy.EditRoleName)
-		impersonategroup = createGroup(cli, "impersonategroup", bootstrappolicy.EditRoleName)
-		addUserToGroup(cli, impersonatebygroupuser.Name, impersonategroup.Name)
 		edituser1 = createUser(cli, "edituser1", bootstrappolicy.EditRoleName)
 		edituser2 = createUser(cli, "edituser2", bootstrappolicy.EditRoleName)
 		viewuser = createUser(cli, "viewuser", bootstrappolicy.ViewRoleName)
 
+		impersonategroup = createGroup(cli, "impersonategroup", bootstrappolicy.EditRoleName)
+		addUserToGroup(cli, impersonatebygroupuser.Name, impersonategroup.Name)
+
+		// additional plumbing to enable impersonateuser to impersonate edituser1
+		role, err := cli.AdminAuthorizationClient().Authorization().Roles(cli.Namespace()).Create(&authorizationapi.Role{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "impersonater",
+			},
+			Rules: []authorizationapi.PolicyRule{
+				{
+					Verbs:     sets.NewString("assign"),
+					APIGroups: []string{templateapi.GroupName},
+					Resources: sets.NewString("templateinstances"),
+				},
+			},
+		})
+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		_, err = cli.AdminAuthorizationClient().Authorization().RoleBindings(cli.Namespace()).Create(&authorizationapi.RoleBinding{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "impersonater-binding",
+			},
+			RoleRef: kapi.ObjectReference{
+				Name:      role.Name,
+				Namespace: cli.Namespace(),
+			},
+			Subjects: []kapi.ObjectReference{
+				{
+					Kind: authorizationapi.UserKind,
+					Name: impersonateuser.Name,
+				},
+				{
+					Kind: authorizationapi.GroupKind,
+					Name: impersonategroup.Name,
+				},
+			},
+		})
+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		// I think we get flakes when the group cache hasn't yet noticed the
+		// new group membership made above.  Wait until all it looks like
+		// all the users above have access to the namespace as expected.
+		err = wait.PollImmediate(time.Second, 30*time.Second, func() (done bool, err error) {
+			for _, user := range []*userapi.User{adminuser, impersonateuser, impersonatebygroupuser, edituser1, edituser2, viewuser} {
+				cli.ChangeUser(user.Name)
+				sar, err := cli.AuthorizationClient().Authorization().LocalSubjectAccessReviews(cli.Namespace()).Create(&authorizationapi.LocalSubjectAccessReview{
+					Action: authorizationapi.Action{
+						Verb:     "get",
+						Resource: "pods",
+					},
+				})
+				if err != nil {
+					return false, err
+				}
+				if !sar.Allowed {
+					return false, nil
+				}
+			}
+
+			cli.ChangeUser(impersonatebygroupuser.Name)
+			sar, err := cli.AuthorizationClient().Authorization().LocalSubjectAccessReviews(cli.Namespace()).Create(&authorizationapi.LocalSubjectAccessReview{
+				Action: authorizationapi.Action{
+					Verb:     "assign",
+					Group:    templateapi.GroupName,
+					Resource: "templateinstances",
+				},
+			})
+			if err != nil {
+				return false, err
+			}
+			if !sar.Allowed {
+				return false, nil
+			}
+
+			return true, nil
+		})
+		o.Expect(err).NotTo(o.HaveOccurred())
+
 		dummytemplateinstance = &templateapi.TemplateInstance{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "test",
@@ -141,42 +218,6 @@ var _ = g.Describe("[Conformance][templates] templateinstance impersonation test
 				hasUpdateStatusPermission: false,
 			},
 		}
-
-		// additional plumbing to enable impersonateuser to impersonate edituser1
-		role, err := cli.AdminAuthorizationClient().Authorization().Roles(cli.Namespace()).Create(&authorizationapi.Role{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "impersonater",
-			},
-			Rules: []authorizationapi.PolicyRule{
-				{
-					Verbs:     sets.NewString("assign"),
-					APIGroups: []string{templateapi.GroupName},
-					Resources: sets.NewString("templateinstances"),
-				},
-			},
-		})
-		o.Expect(err).NotTo(o.HaveOccurred())
-
-		_, err = cli.AdminAuthorizationClient().Authorization().RoleBindings(cli.Namespace()).Create(&authorizationapi.RoleBinding{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "impersonater-binding",
-			},
-			RoleRef: kapi.ObjectReference{
-				Name:      role.Name,
-				Namespace: cli.Namespace(),
-			},
-			Subjects: []kapi.ObjectReference{
-				{
-					Kind: authorizationapi.UserKind,
-					Name: impersonateuser.Name,
-				},
-				{
-					Kind: authorizationapi.GroupKind,
-					Name: impersonategroup.Name,
-				},
-			},
-		})
-		o.Expect(err).NotTo(o.HaveOccurred())
 	})
 
 	g.AfterEach(func() {

test/extended/templates/templateinstance_security.go

@@ -273,8 +273,20 @@ var _ = g.Describe("[Conformance][templates] templateinstance security tests", f
 				o.Expect(templateinstance.HasCondition(test.expectCondition, kapi.ConditionTrue)).To(o.Equal(true))
 				o.Expect(test.checkOK(test.namespace)).To(o.BeTrue())
 
-				err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, nil)
+				foreground := metav1.DeletePropagationForeground
+				err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, &metav1.DeleteOptions{PropagationPolicy: &foreground})
 				o.Expect(err).NotTo(o.HaveOccurred())
+
+				// wait for garbage collector to do its thing
+				err = wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
+					_, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Get(templateinstance.Name, metav1.GetOptions{})
+					if kerrors.IsNotFound(err) {
+						return true, nil
+					}
+					return false, err
+				})
+				o.Expect(err).NotTo(o.HaveOccurred())
+
 				err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Delete(secret.Name, nil)
 				o.Expect(err).NotTo(o.HaveOccurred())
 			}