Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 returned objects of reference type should be unchangeable #1851

Merged
merged 1 commit into from Apr 1, 2022
Merged

馃悰 returned objects of reference type should be unchangeable #1851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions pkg/cache/cache_test.go
View file
Open in desktop
Expand Up @@ -1245,6 +1245,40 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
Expect(sii).To(BeNil())
Expect(apierrors.IsTimeout(err)).To(BeTrue())
})

It("should be able not to change indexer values after indexing cluster-scope objects", func() {
By("creating the cache")
informer, err := cache.New(cfg, cache.Options{})
Expect(err).NotTo(HaveOccurred())

By("indexing the Namespace objects with fixed values before starting")
pod := &corev1.Namespace{}
indexerValues := []string{"a", "b", "c"}
fieldName := "fixedValues"
indexFunc := func(obj client.Object) []string {
return indexerValues
}
Expect(informer.IndexField(context.TODO(), pod, fieldName, indexFunc)).To(Succeed())

By("running the cache and waiting for it to sync")
go func() {
defer GinkgoRecover()
Expect(informer.Start(informerCacheCtx)).To(Succeed())
}()
Expect(informer.WaitForCacheSync(informerCacheCtx)).NotTo(BeFalse())

By("listing Namespaces with fixed indexer")
listObj := &corev1.NamespaceList{}
Expect(informer.List(context.Background(), listObj,
client.MatchingFields{fieldName: "a"})).To(Succeed())
Expect(listObj.Items).NotTo(BeZero())

By("verifying the indexing does not change fixed returned values")
Expect(indexerValues).Should(HaveLen(3))
Expect(indexerValues[0]).To(Equal("a"))
Expect(indexerValues[1]).To(Equal("b"))
Expect(indexerValues[2]).To(Equal("c"))
})
})
Context("with unstructured objects", func() {
It("should be able to get informer for the object", func() {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cache/informer_cache.go
View file
Open in desktop
Expand Up @@ -193,8 +193,8 @@ func indexByField(indexer Informer, field string, extractor client.IndexerFunc)
rawVals := extractor(obj)
var vals []string
if ns == "" {
// if we're not doubling the keys for the namespaced case, just re-use what was returned to us
vals = rawVals
// if we're not doubling the keys for the namespaced case, just create a new slice with same length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't understand. If you write a function that returns a pointer, you are not supposed to keep a reference to that pointer or its underlying value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why you are doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The story is, I compute and put the indexer values into the object status .status.indexerValues, so the extractor will be

func(obj client.Object) []string {
    someObject, _ := obj.(*group.ObjectKind)
    return someObject.Status.IndexerValues
}

As we know, the object from client cache should be unchangeable.

Just think another scene,

values := []string{"a", "b", "c"}
do(values)
doAgain(values)

Do we expect values will be changed by do? Then doAgain will get unexpected values.

Though slice(extractor return) is a pointer type here, but it is used because it contains immutable values, so I don't think it should be changed when being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slice(extractor return) here is more like a container containing immutable values(indexer values).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, can you then please add a test that covers this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, will do it ~

vals = make([]string, len(rawVals))
} else {
// if we need to add non-namespaced versions too, double the length
vals = make([]string, len(rawVals)*2)
Expand Down