feat(source): migrate gateway sources to indexer-based filtering#6545
feat(source): migrate gateway sources to indexer-based filtering#6545ivankatliarchuk wants to merge 1 commit into
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Coverage Report for CI Build 28803821713Coverage increased (+0.5%) to 84.031%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
|
When reviewing this PR, I discovered two related issues. One directly on indexers, following #6445, on "/", confirmed with this test into func TestIndexerWithOptions_ClusterScopedLabelKeyLeadingSlash(t *testing.T) {
node := &corev1.Node{} // Node is cluster-scoped: GetNamespace() == ""
node.SetName("my-node")
node.SetLabels(map[string]string{"parent": "owner-node"})
// by-name branch: fixed in #6445 -> bare name, matches MetaNamespaceKeyFunc.
byName := IndexerWithOptions[*corev1.Node]()
keys, err := byName[IndexWithSelectors](node)
require.NoError(t, err)
assert.Equal(t, []string{"my-node"}, keys, "by-name branch correctly emits a bare name")
// by-label branch: NOT fixed -> leading slash from NamespacedName{Namespace:""}.String().
byLabel := IndexerWithOptions[*corev1.Node](IndexSelectorWithLabelKey("parent"))
keys, err = byLabel[IndexWithSelectors](node)
require.NoError(t, err)
assert.Equal(t, []string{"/owner-node"}, keys,
"by-label branch still prepends a slash for cluster-scoped objects; "+
"a bare \"owner-node\" (consistent with the by-name fix) is expected")
}The second is regression on filters, also following #6445, where invalid filters are silently discarded, confirmed with this test into func TestIngressSource_InvalidAnnotationFilterSilentlyMatchesAll(t *testing.T) {
t.Parallel()
// The invalid filter really is invalid: ParseFilter reports an error...
invalidFilter := "tier in (x y)" // set-based selector missing the comma
sel, err := annotations.ParseFilter(invalidFilter)
require.Error(t, err, "expected an invalid annotation filter to error at parse time")
require.Nil(t, sel, "ParseFilter returns a nil selector on invalid input")
// ...but store.go discards that error, so the source is built with a nil selector.
// Reproduce exactly what BuildWithConfig hands to NewIngressSource.
client := fake.NewClientset()
for i, ing := range createTestIngresses(3) {
maps.Copy(ing.Annotations, map[string]string{
annotations.HostnameKey: fmt.Sprintf("ing-%d.example.org", i),
annotations.TargetKey: fmt.Sprintf("target-%d.example.com", i),
})
_, err := client.NetworkingV1().Ingresses(ing.Namespace).Create(t.Context(), ing, metav1.CreateOptions{})
require.NoError(t, err)
}
src, err := NewIngressSource(t.Context(), client, &Config{
AnnotationFilter: sel, // nil, as produced by the discarded-error path in store.go
LabelFilter: labels.Everything(),
TemplateEngine: templatetest.MustEngine(t, "", "", "", false),
})
require.NoError(t, err)
endpoints, err := src.Endpoints(t.Context())
// Regression: the invalid filter neither errors nor filters — all 3 ingresses pass.
require.NoError(t, err, "invalid annotation filter no longer surfaces an error")
assert.Len(t, endpoints, 3, "invalid annotation filter silently matched every ingress")
} |
|
I'm not too sure that I fully follow what needs to be done |
|
I don't understand that test TestIngressSource_InvalidAnnotationFilterSilentlyMatchesAll. Not clear how behaviour is changed, it looks exactly same to me -> wrong annotation filter will not reach the service layer. If you passing no annotations filter, what is your expectation then? Nil annotation filter means there is no filter. You could not provide invalid annotation in production setup, not because of Line 132 in c6a922c The label selector has exactly same behaviour https://github.com/kubernetes-sigs/external-dns/pull/5222/changes#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261
Do you mean, you want instead Something like |

What does it do ?
Motivation
More