Add methods for dynamic target matching #6
Conversation
This extends the idea of the NamespaceSelector to other types. The FakePolicy tests demonstrate that Target can be used in a cluster- scoped manner; it would be easy to restrict it to a specific namespace. Using NamespaceSelector and Target to limit the policy to specific resources in specific namespaces is possible, but might require a loop over all matched namespaces, with a namespaced call in each. With the new fields in the FakePolicy, one of the format verification test cases was unnecessary. Signed-off-by: Justin Kulikauskas <[email protected]>
The new method does not use a dynamic client, which allows it to utilize the read cache that is configured by default on the controller-runtime clients. This can improve the speed of the controller, while also reducing load on the cluster's API server. The FakePolicy controller demonstrates (and tests) how to implement the new ResourceList interface, necessary because the `client.ObjectList` interface does not provide any access to the list's items. This is a partial implementation, which does not allow restricting the matches by namespace (that is coming next). Signed-off-by: Justin Kulikauskas <[email protected]>
GetMatches and GetMatchesDynamic are now able to be namespaced by the Target. Previously, only GetMatchesDynamic could be namespaced, and it had to be done by the user when creating the ResourceInterface. This seems to be a more ergonomic solution. The tests for the namespaced Targets are mostly duplicates of the tests for the non-namespaced Targets; some refactoring allowed them to be re-used without copying them, with only some small adjustments. Signed-off-by: Justin Kulikauskas <[email protected]>
This reduces some duplicated logic, and provides another example for users of how to implement the ResourceList interface. Signed-off-by: Justin Kulikauskas <[email protected]>
PR Review
✨ Review tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (l fakeObjList) SetResourceVersion(version string) { | ||
| } |
There was a problem hiding this comment.
Suggestion: The method SetResourceVersion in fakeObjList and fakeObj should set the resource version to the provided string. Currently, it does nothing, which might not be the intended behavior. [bug]
| func (l fakeObjList) SetResourceVersion(version string) { | |
| } | |
| func (l *fakeObjList) SetResourceVersion(version string) { | |
| *l = fakeObjList(version) | |
| } |
| func (l fakeObjList) DeepCopyObject() runtime.Object { | ||
| return l | ||
| } |
There was a problem hiding this comment.
Suggestion: The method DeepCopyObject in fakeObjList and fakeObj should return a deep copy of the object instead of returning the receiver directly. This prevents modifications to the copy from affecting the original object. [bug]
| func (l fakeObjList) DeepCopyObject() runtime.Object { | |
| return l | |
| } | |
| func (l fakeObjList) DeepCopyObject() runtime.Object { | |
| copy := l | |
| return copy | |
| } |
| func (l fakeObjList) SetRemainingItemCount(c *int64) { | ||
| } |
There was a problem hiding this comment.
Suggestion: The method SetRemainingItemCount in fakeObjList should set the remaining item count to the provided pointer. Currently, it does nothing, which might not be the intended behavior. [bug]
| func (l fakeObjList) SetRemainingItemCount(c *int64) { | |
| } | |
| func (l *fakeObjList) SetRemainingItemCount(c *int64) { | |
| // Assuming there's a field to store this value, which should be added to the struct. | |
| // l.remainingItemCount = c | |
| } |
api/v1beta1/target.go
Outdated
| // ReflectiveResourceList implements ResourceList for the wrapped client.ObjectList, by using | ||
| // reflection. The wrapped list must have an Items field, with a slice of items which satisfy the | ||
| // client.Object interface - most types which satisfy client.ObjectList seem to follow this | ||
| // convention. Using this type is not recommended: implementing ResourceList yourself will generally | ||
| // lead to better performance. | ||
| type ReflectiveResourceList struct { |
There was a problem hiding this comment.
Suggestion: The ReflectiveResourceList uses reflection which can be error-prone and inefficient. Consider implementing specific ResourceList types for known ObjectList types to avoid reflection and improve performance. [enhancement]
| // ReflectiveResourceList implements ResourceList for the wrapped client.ObjectList, by using | |
| // reflection. The wrapped list must have an Items field, with a slice of items which satisfy the | |
| // client.Object interface - most types which satisfy client.ObjectList seem to follow this | |
| // convention. Using this type is not recommended: implementing ResourceList yourself will generally | |
| // lead to better performance. | |
| type ReflectiveResourceList struct { | |
| // Example for a specific implementation for a PodList | |
| type PodResourceList struct { | |
| PodList *corev1.PodList | |
| } | |
| func (l *PodResourceList) Items() ([]client.Object, error) { | |
| items := make([]client.Object, len(l.PodList.Items)) | |
| for i, pod := range l.PodList.Items { | |
| items[i] = &pod | |
| } | |
| return items, nil | |
| } | |
| func (l *PodResourceList) ObjectList() client.ObjectList { | |
| return l.PodList | |
| } |
There was a problem hiding this comment.
Did you just re-word the warnings in my docstring as a suggestion for me? 😆 I mean, it's true, but...
| include, err = filepath.Match(string(includePattern), name) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error parsing 'include' pattern '%s': %w", string(includePattern), err) | ||
| } |
There was a problem hiding this comment.
Suggestion: The error handling in match function should provide more context about the error, especially which pattern caused the error. This will help in debugging issues with pattern matching. [enhancement]
| include, err = filepath.Match(string(includePattern), name) | |
| if err != nil { | |
| return false, fmt.Errorf("error parsing 'include' pattern '%s': %w", string(includePattern), err) | |
| } | |
| include, err = filepath.Match(string(includePattern), name) | |
| if err != nil { | |
| return false, fmt.Errorf("error parsing 'include' pattern '%s' for name '%s': %w", string(includePattern), name, err) | |
| } |
| By("Creating sample configmaps") | ||
| for _, cm := range sampleConfigMaps { | ||
| ns, name, _ := strings.Cut(cm, "/") | ||
| cmObj := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: ns, | ||
| Labels: map[string]string{"sample": name}, | ||
| }, | ||
| Data: map[string]string{"foo": "bar"}, | ||
| } | ||
| Expect(cleanlyCreate(cmObj)).To(Succeed()) |
There was a problem hiding this comment.
Suggestion: To improve the efficiency of the test setup, consider using a batch creation approach for ConfigMaps if the API supports it, instead of creating them one by one inside a loop. This reduces the number of API calls and can significantly speed up the test setup phase. [performance]
| By("Creating sample configmaps") | |
| for _, cm := range sampleConfigMaps { | |
| ns, name, _ := strings.Cut(cm, "/") | |
| cmObj := &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| Namespace: ns, | |
| Labels: map[string]string{"sample": name}, | |
| }, | |
| Data: map[string]string{"foo": "bar"}, | |
| } | |
| Expect(cleanlyCreate(cmObj)).To(Succeed()) | |
| // Example of batch creation, adjust based on actual API capabilities | |
| var configMaps []*corev1.ConfigMap | |
| for _, cm := range sampleConfigMaps { | |
| ns, name, _ := strings.Cut(cm, "/") | |
| cmObj := &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| Namespace: ns, | |
| Labels: map[string]string{"sample": name}, | |
| }, | |
| Data: map[string]string{"foo": "bar"}, | |
| } | |
| configMaps = append(configMaps, cmObj) | |
| } | |
| Expect(batchCreateConfigMaps(configMaps)).To(Succeed()) |
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" |
There was a problem hiding this comment.
Suggestion: It's recommended to avoid using dot imports as they can pollute the namespace and make it harder to understand where identifiers come from. Consider importing the packages with a namespace and using them explicitly. [best practice]
| . "github.com/onsi/ginkgo/v2" | |
| . "github.com/onsi/gomega" | |
| ginkgo "github.com/onsi/ginkgo/v2" | |
| gomega "github.com/onsi/gomega" |
| Eventually(func(g Gomega) { | ||
| foundPolicy := fakev1beta1.FakePolicy{} | ||
| g.Expect(k8sClient.Get(ctx, getNamespacedName(&policy), &foundPolicy)).To(Succeed()) | ||
| g.Expect(foundPolicy.Status.SelectionComplete).To(BeTrue()) | ||
| g.Expect(foundPolicy.Status.DynamicSelectedConfigMaps).To(ConsistOf(desiredMatches)) | ||
| g.Expect(foundPolicy.Status.ClientSelectedConfigMaps).To(ConsistOf(desiredMatches)) | ||
| g.Expect(foundPolicy.Status.SelectionError).To(Equal(selErr)) |
There was a problem hiding this comment.
Suggestion: The use of Eventually should include a timeout to avoid indefinitely waiting for a condition that may never be met, which can hang the test suite. [best practice]
| Eventually(func(g Gomega) { | |
| foundPolicy := fakev1beta1.FakePolicy{} | |
| g.Expect(k8sClient.Get(ctx, getNamespacedName(&policy), &foundPolicy)).To(Succeed()) | |
| g.Expect(foundPolicy.Status.SelectionComplete).To(BeTrue()) | |
| g.Expect(foundPolicy.Status.DynamicSelectedConfigMaps).To(ConsistOf(desiredMatches)) | |
| g.Expect(foundPolicy.Status.ClientSelectedConfigMaps).To(ConsistOf(desiredMatches)) | |
| g.Expect(foundPolicy.Status.SelectionError).To(Equal(selErr)) | |
| Eventually(func(g Gomega) { | |
| foundPolicy := fakev1beta1.FakePolicy{} | |
| g.Expect(k8sClient.Get(ctx, getNamespacedName(&policy), &foundPolicy)).To(Succeed()) | |
| g.Expect(foundPolicy.Status.SelectionComplete).To(BeTrue()) | |
| g.Expect(foundPolicy.Status.DynamicSelectedConfigMaps).To(ConsistOf(desiredMatches)) | |
| g.Expect(foundPolicy.Status.ClientSelectedConfigMaps).To(ConsistOf(desiredMatches)) | |
| g.Expect(foundPolicy.Status.SelectionError).To(Equal(selErr)) | |
| }, "30s").Should(Succeed()) # Example timeout of 30 seconds |
There was a problem hiding this comment.
If not provided, Eventually uses a default timeout (I think 1 second)
| allConfigMaps := append(defaultConfigMaps, sampleConfigMaps...) | ||
|
|
||
| beforeFunc := func() { | ||
| By("Creating sample configmaps") | ||
| for _, cm := range sampleConfigMaps { | ||
| ns, name, _ := strings.Cut(cm, "/") | ||
| cmObj := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: ns, | ||
| Labels: map[string]string{"sample": name}, | ||
| }, | ||
| Data: map[string]string{"foo": "bar"}, | ||
| } | ||
| Expect(cleanlyCreate(cmObj)).To(Succeed()) | ||
| } | ||
|
|
||
| By("Ensuring the allConfigMaps list is correct") | ||
| // constructing the default / allConfigMaps lists is complicated because of how ginkgo | ||
| // runs the table tests... this seems better than other workarounds. | ||
| cmList := corev1.ConfigMapList{} | ||
| Expect(k8sClient.List(ctx, &cmList)).To(Succeed()) | ||
|
|
||
| foundCM := make([]string, len(cmList.Items)) | ||
| for i, cm := range cmList.Items { | ||
| foundCM[i] = cm.GetNamespace() + "/" + cm.GetName() | ||
| } | ||
|
|
||
| Expect(allConfigMaps).To(ConsistOf(foundCM)) |
There was a problem hiding this comment.
Suggestion: To ensure that the allConfigMaps list is correctly populated before it's used in assertions, consider adding a check to ensure that sampleConfigMaps and defaultConfigMaps are not empty. This prevents false positives in tests where the configuration might not be loaded as expected. [enhancement]
| allConfigMaps := append(defaultConfigMaps, sampleConfigMaps...) | |
| beforeFunc := func() { | |
| By("Creating sample configmaps") | |
| for _, cm := range sampleConfigMaps { | |
| ns, name, _ := strings.Cut(cm, "/") | |
| cmObj := &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| Namespace: ns, | |
| Labels: map[string]string{"sample": name}, | |
| }, | |
| Data: map[string]string{"foo": "bar"}, | |
| } | |
| Expect(cleanlyCreate(cmObj)).To(Succeed()) | |
| } | |
| By("Ensuring the allConfigMaps list is correct") | |
| // constructing the default / allConfigMaps lists is complicated because of how ginkgo | |
| // runs the table tests... this seems better than other workarounds. | |
| cmList := corev1.ConfigMapList{} | |
| Expect(k8sClient.List(ctx, &cmList)).To(Succeed()) | |
| foundCM := make([]string, len(cmList.Items)) | |
| for i, cm := range cmList.Items { | |
| foundCM[i] = cm.GetNamespace() + "/" + cm.GetName() | |
| } | |
| Expect(allConfigMaps).To(ConsistOf(foundCM)) | |
| allConfigMaps := append(defaultConfigMaps, sampleConfigMaps...) | |
| Expect(defaultConfigMaps).NotTo(BeEmpty()) | |
| Expect(sampleConfigMaps).NotTo(BeEmpty()) | |
| Expect(allConfigMaps).To(ConsistOf(foundCM)) |
| targetConfigMaps: | ||
| matchExpressions: | ||
| - key: sample | ||
| operator: Exists |
There was a problem hiding this comment.
Suggestion: It's recommended to specify values or value when using the Exists operator in matchExpressions to ensure clarity and explicit behavior, even though it might be optional depending on the context. [enhancement]
| targetConfigMaps: | |
| matchExpressions: | |
| - key: sample | |
| operator: Exists | |
| targetConfigMaps: | |
| matchExpressions: | |
| - key: sample | |
| operator: Exists | |
| values: ["desired_value_here"] |
| // provided Interface is already namespaced, the namespace of the Interface will be used (possibly | ||
| // overriding the namespace specified in the Target). | ||
| // | ||
| // NOTE: unlike the NamespaceSelector, an empty Target will match *all* resources on the cluster. |
There was a problem hiding this comment.
This is something I need to reconsider
There was a problem hiding this comment.
Careful documentation will be the best approach. The behavior of selectors is often confusing to me (and I've read through it on various occasions). Nil selectors might select nothing? But empty selectors select everything. It's like they're deliberately trying to confuse me. So we will treat nil as empty, and if a user wants to select nothing, they can Exclude: "*".
| matchingNames := make([]string, 0, len(set)) | ||
| for ns := range set { | ||
| matchingNames = append(matchingNames, ns) | ||
| } | ||
|
|
||
| return matchingNames, nil |
There was a problem hiding this comment.
This is a good idea. Not in this test function specifically, but more generally in the GetMatches and GetMatchesDynamic methods.
| Eventually(func(g Gomega) { | ||
| foundPolicy := fakev1beta1.FakePolicy{} | ||
| g.Expect(k8sClient.Get(ctx, getNamespacedName(&policy), &foundPolicy)).To(Succeed()) | ||
| g.Expect(foundPolicy.Status.SelectionComplete).To(BeTrue()) | ||
| g.Expect(foundPolicy.Status.DynamicSelectedConfigMaps).To(ConsistOf(desiredMatches)) | ||
| g.Expect(foundPolicy.Status.ClientSelectedConfigMaps).To(ConsistOf(desiredMatches)) | ||
| g.Expect(foundPolicy.Status.SelectionError).To(Equal(selErr)) |
There was a problem hiding this comment.
If not provided, Eventually uses a default timeout (I think 1 second)
|
/describe |
TitleAdd methods for dynamic target matching User descriptionThe main idea was to extend the nice name + label matching things we have for Namespaces to other types. The individual commit messages have much more context. Typeenhancement, tests Description
Changes walkthrough
|
AI Suggestions Score
|
This type implements ResourceList using reflection, so that consumers don't need to implement the methods themselves. FakePolicyController tests verify that the behavior of using this implementation matches the behavior of the bespoke implementation. Signed-off-by: Justin Kulikauskas <[email protected]>
Signed-off-by: Justin Kulikauskas <[email protected]>
Signed-off-by: Justin Kulikauskas <[email protected]>
JustinKuli
force-pushed
the
updated-dyn-target
branch
from
a65fcc0 to
b888d94
Compare
The main idea was to extend the nice name + label matching things we have for Namespaces to other types. The individual commit messages have much more context.