Skip to content

Commit 8e73380

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #16979 from jim-minter/issue16775-2
Automatic merge from submit-queue. delete templateinstances in foreground where necessary in extended tests fixes #16775 (2nd attempt) Proposing to comment out the previous "fix" in case it's superstition
2 parents f10a493 + 1a072ae commit 8e73380

File tree

3 files changed

+80
-40
lines changed

3 files changed

+80
-40
lines changed

‎pkg/template/registry/templateinstance/strategy.go

+30-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ package templateinstance
33
import (
44
"errors"
55

6+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
67
"k8s.io/apimachinery/pkg/runtime"
8+
kutilerrors "k8s.io/apimachinery/pkg/util/errors"
79
"k8s.io/apimachinery/pkg/util/validation/field"
810
"k8s.io/apiserver/pkg/authentication/user"
911
apirequest "k8s.io/apiserver/pkg/endpoints/request"
@@ -96,8 +98,34 @@ func (s *templateInstanceStrategy) ValidateUpdate(ctx apirequest.Context, obj, o
9698
return field.ErrorList{field.InternalError(field.NewPath(""), errors.New("user not found in context"))}
9799
}
98100

99-
templateInstance := obj.(*templateapi.TemplateInstance)
100-
oldTemplateInstance := old.(*templateapi.TemplateInstance)
101+
// Decode Spec.Template.Objects on both obj and old to Unstructureds. This
102+
// allows detectection of at least some cases where the Objects are
103+
// semantically identical, but the serialisations have been jumbled up. One
104+
// place where this happens is in the garbage collector, which uses
105+
// Unstructureds via the dynamic client.
106+
107+
objcopy, err := kapi.Scheme.DeepCopy(obj)
108+
if err != nil {
109+
return field.ErrorList{field.InternalError(field.NewPath(""), err)}
110+
}
111+
templateInstance := objcopy.(*templateapi.TemplateInstance)
112+
113+
errs := runtime.DecodeList(templateInstance.Spec.Template.Objects, unstructured.UnstructuredJSONScheme)
114+
if len(errs) != 0 {
115+
return field.ErrorList{field.InternalError(field.NewPath(""), kutilerrors.NewAggregate(errs))}
116+
}
117+
118+
oldcopy, err := kapi.Scheme.DeepCopy(old)
119+
if err != nil {
120+
return field.ErrorList{field.InternalError(field.NewPath(""), err)}
121+
}
122+
oldTemplateInstance := oldcopy.(*templateapi.TemplateInstance)
123+
124+
errs = runtime.DecodeList(oldTemplateInstance.Spec.Template.Objects, unstructured.UnstructuredJSONScheme)
125+
if len(errs) != 0 {
126+
return field.ErrorList{field.InternalError(field.NewPath(""), kutilerrors.NewAggregate(errs))}
127+
}
128+
101129
allErrs := validation.ValidateTemplateInstanceUpdate(templateInstance, oldTemplateInstance)
102130
allErrs = append(allErrs, s.validateImpersonationUpdate(templateInstance, oldTemplateInstance, user)...)
103131

‎test/extended/templates/templateinstance_cross_namespace.go

+12-16
Original file line numberDiff line numberDiff line change
@@ -112,28 +112,24 @@ var _ = g.Describe("[Conformance][templates] templateinstance cross-namespace te
112112
o.Expect(err).NotTo(o.HaveOccurred())
113113

114114
g.By("deleting the templateinstance")
115-
err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, nil)
115+
foreground := metav1.DeletePropagationForeground
116+
err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, &metav1.DeleteOptions{PropagationPolicy: &foreground})
116117
o.Expect(err).NotTo(o.HaveOccurred())
117118

118119
// wait for garbage collector to do its thing
119-
err = wait.Poll(time.Second, time.Minute, func() (bool, error) {
120+
err = wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
120121
_, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Get(templateinstance.Name, metav1.GetOptions{})
121-
if err == nil || !kerrors.IsNotFound(err) {
122-
return false, err
122+
if kerrors.IsNotFound(err) {
123+
return true, nil
123124
}
124-
125-
_, err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Get("secret1", metav1.GetOptions{})
126-
if err == nil || !kerrors.IsNotFound(err) {
127-
return false, err
128-
}
129-
130-
_, err = cli.KubeClient().CoreV1().Secrets(cli2.Namespace()).Get("secret2", metav1.GetOptions{})
131-
if err == nil || !kerrors.IsNotFound(err) {
132-
return false, err
133-
}
134-
135-
return true, nil
125+
return false, err
136126
})
137127
o.Expect(err).NotTo(o.HaveOccurred())
128+
129+
_, err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Get("secret1", metav1.GetOptions{})
130+
o.Expect(kerrors.IsNotFound(err)).To(o.BeTrue())
131+
132+
_, err = cli.KubeClient().CoreV1().Secrets(cli2.Namespace()).Get("secret2", metav1.GetOptions{})
133+
o.Expect(kerrors.IsNotFound(err)).To(o.BeTrue())
138134
})
139135
})

‎test/extended/templates/templateinstance_security.go

+38-22
Original file line numberDiff line numberDiff line change
@@ -77,28 +77,32 @@ var _ = g.Describe("[Conformance][templates] templateinstance security tests", f
7777
editgroup = createGroup(cli, "editgroup", bootstrappolicy.EditRoleName)
7878
addUserToGroup(cli, editbygroupuser.Name, editgroup.Name)
7979

80-
// I think we get flakes when the group cache hasn't yet noticed the
81-
// new group membership made above. Wait until all it looks like
82-
// all the users above have access to the namespace as expected.
83-
err := wait.PollImmediate(time.Second, 30*time.Second, func() (done bool, err error) {
84-
for _, user := range []*userapi.User{adminuser, edituser, editbygroupuser} {
85-
cli.ChangeUser(user.Name)
86-
sar, err := cli.AuthorizationClient().Authorization().LocalSubjectAccessReviews(cli.Namespace()).Create(&authorizationapi.LocalSubjectAccessReview{
87-
Action: authorizationapi.Action{
88-
Verb: "get",
89-
Resource: "pods",
90-
},
91-
})
92-
if err != nil {
93-
return false, err
94-
}
95-
if !sar.Allowed {
96-
return false, nil
80+
/*
81+
// jminter: commenting this out for now in case it turns out to be superstition
82+
83+
// I think we get flakes when the group cache hasn't yet noticed the
84+
// new group membership made above. Wait until all it looks like
85+
// all the users above have access to the namespace as expected.
86+
err := wait.PollImmediate(time.Second, 30*time.Second, func() (done bool, err error) {
87+
for _, user := range []*userapi.User{adminuser, edituser, editbygroupuser} {
88+
cli.ChangeUser(user.Name)
89+
sar, err := cli.AuthorizationClient().Authorization().LocalSubjectAccessReviews(cli.Namespace()).Create(&authorizationapi.LocalSubjectAccessReview{
90+
Action: authorizationapi.Action{
91+
Verb: "get",
92+
Resource: "pods",
93+
},
94+
})
95+
if err != nil {
96+
return false, err
97+
}
98+
if !sar.Allowed {
99+
return false, nil
100+
}
97101
}
98-
}
99-
return true, nil
100-
})
101-
o.Expect(err).NotTo(o.HaveOccurred())
102+
return true, nil
103+
})
104+
o.Expect(err).NotTo(o.HaveOccurred())
105+
*/
102106
})
103107

104108
g.AfterEach(func() {
@@ -273,8 +277,20 @@ var _ = g.Describe("[Conformance][templates] templateinstance security tests", f
273277
o.Expect(templateinstance.HasCondition(test.expectCondition, kapi.ConditionTrue)).To(o.Equal(true))
274278
o.Expect(test.checkOK(test.namespace)).To(o.BeTrue())
275279

276-
err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, nil)
280+
foreground := metav1.DeletePropagationForeground
281+
err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Delete(templateinstance.Name, &metav1.DeleteOptions{PropagationPolicy: &foreground})
277282
o.Expect(err).NotTo(o.HaveOccurred())
283+
284+
// wait for garbage collector to do its thing
285+
err = wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
286+
_, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Get(templateinstance.Name, metav1.GetOptions{})
287+
if kerrors.IsNotFound(err) {
288+
return true, nil
289+
}
290+
return false, err
291+
})
292+
o.Expect(err).NotTo(o.HaveOccurred())
293+
278294
err = cli.KubeClient().CoreV1().Secrets(cli.Namespace()).Delete(secret.Name, nil)
279295
o.Expect(err).NotTo(o.HaveOccurred())
280296
}

0 commit comments

Comments
 (0)