From 1a072ae37be86f6163148886b85955628c056e37 Mon Sep 17 00:00:00 2001 From: Jim Minter Date: Fri, 20 Oct 2017 12:07:26 -0500 Subject: [PATCH 1/2] delete templateinstances in foreground where necessary in extended tests --- .../registry/templateinstance/strategy.go | 32 +++++++++- .../templateinstance_cross_namespace.go | 28 ++++----- .../templates/templateinstance_security.go | 60 ++++++++++++------- 3 files changed, 80 insertions(+), 40 deletions(-) diff --git a/pkg/template/registry/templateinstance/strategy.go b/pkg/template/registry/templateinstance/strategy.go index ea8940b22f88..b0e24a068c4c 100644 --- a/pkg/template/registry/templateinstance/strategy.go +++ b/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)...) diff --git a/test/extended/templates/templateinstance_cross_namespace.go b/test/extended/templates/templateinstance_cross_namespace.go index 25ce726ef934..439a7402ef2e 100644 --- a/test/extended/templates/templateinstance_cross_namespace.go +++ b/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()) }) }) diff --git a/test/extended/templates/templateinstance_security.go b/test/extended/templates/templateinstance_security.go index a59827daba29..c74801171895 100644 --- a/test/extended/templates/templateinstance_security.go +++ b/test/extended/templates/templateinstance_security.go @@ -77,28 +77,32 @@ var _ = g.Describe("[Conformance][templates] templateinstance security tests", f editgroup = createGroup(cli, "editgroup", bootstrappolicy.EditRoleName) addUserToGroup(cli, editbygroupuser.Name, editgroup.Name) - // 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, edituser, editbygroupuser} { - 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 + /* + // jminter: commenting this out for now in case it turns out to be superstition + + // 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, edituser, editbygroupuser} { + 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 + } } - } - return true, nil - }) - o.Expect(err).NotTo(o.HaveOccurred()) + return true, nil + }) + o.Expect(err).NotTo(o.HaveOccurred()) + */ }) g.AfterEach(func() { @@ -273,8 +277,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()) } From 9d8b6bec76c37505141e406c4d3b2619287368d6 Mon Sep 17 00:00:00 2001 From: Jim Minter Date: Mon, 23 Oct 2017 14:48:02 -0500 Subject: [PATCH 2/2] wait for group cache in templateinstance tests --- .../templateinstance_impersonation.go | 121 ++++++++++++------ .../templates/templateinstance_security.go | 46 +++---- 2 files changed, 102 insertions(+), 65 deletions(-) diff --git a/test/extended/templates/templateinstance_impersonation.go b/test/extended/templates/templateinstance_impersonation.go index 5af94c391271..1e843f7b95c4 100644 --- a/test/extended/templates/templateinstance_impersonation.go +++ b/test/extended/templates/templateinstance_impersonation.go @@ -1,6 +1,8 @@ package templates import ( + "time" + g "github.com/onsi/ginkgo" o "github.com/onsi/gomega" @@ -8,6 +10,7 @@ import ( 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() { diff --git a/test/extended/templates/templateinstance_security.go b/test/extended/templates/templateinstance_security.go index c74801171895..54160e46203f 100644 --- a/test/extended/templates/templateinstance_security.go +++ b/test/extended/templates/templateinstance_security.go @@ -77,32 +77,28 @@ var _ = g.Describe("[Conformance][templates] templateinstance security tests", f editgroup = createGroup(cli, "editgroup", bootstrappolicy.EditRoleName) addUserToGroup(cli, editbygroupuser.Name, editgroup.Name) - /* - // jminter: commenting this out for now in case it turns out to be superstition - - // 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, edituser, editbygroupuser} { - 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 - } + // 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, edituser, editbygroupuser} { + 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 } - return true, nil - }) - o.Expect(err).NotTo(o.HaveOccurred()) - */ + if !sar.Allowed { + return false, nil + } + } + return true, nil + }) + o.Expect(err).NotTo(o.HaveOccurred()) }) g.AfterEach(func() {