Skip to content

krt: add assertions#56510

Merged
istio-testing merged 1 commit intoistio:masterfrom
howardjohn:pilot/various-httproute-fixes
Jun 4, 2025
Merged

krt: add assertions#56510
istio-testing merged 1 commit intoistio:masterfrom
howardjohn:pilot/various-httproute-fixes

Conversation

@howardjohn
Copy link
Copy Markdown
Member

@howardjohn howardjohn commented Jun 4, 2025

Krt places a number of un-checked requirements on collections.

This PR introduces a new knob to turn on assertions to be used during
development. IT IS OFF for production.

For the record, I had two tests that fail these assertions:

func TestCollectionInvalid(t *testing.T) {
	//return
	stop := test.NewStop(t)
	opts := testOptions(t)
	c := kube.NewFakeClient()
	kpc := kclient.New[*corev1.Pod](c)
	pc := clienttest.Wrap(t, kpc)
	pods := krt.WrapClient[*corev1.Pod](kpc, opts.WithName("Pods")...)
	c.RunAndWait(stop)
	BadPods := krt.NewCollection(pods, func(ctx krt.HandlerContext, i *corev1.Pod) *SimplePod {
		return &SimplePod{
			// Ignore namespace, so we will have 2 inputs map to one output.
			Named: Named{Name: i.Name},
			IP:    i.Status.PodIP,
		}
	}, opts.WithName("Broken")...)
	BadPods.Register(func(o krt.Event[SimplePod]) {
		log.Errorf("howardjohn: %v %v %v", o.Event, o.Old, o.New)
	})

	assert.Equal(t, fetcherSorted(BadPods)(), nil)
	pod := &corev1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			Name:      "name",
			Namespace: "namespace",
		},
		Status: corev1.PodStatus{PodIP: "1.2.3.4"},
	}
	pc.CreateOrUpdateStatus(pod)
	assert.EventuallyEqual(t, fetcherSorted(BadPods), []SimplePod{{Named{Name: "name"}, Labeled{}, "1.2.3.4"}})

	pod2 := &corev1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			Name:      "name",
			Namespace: "namespace2",
		},
		Status: corev1.PodStatus{PodIP: "1.2.3.5"},
	}
	pc.Create(pod2)
	assert.EventuallyEqual(t, fetcherSorted(BadPods), []SimplePod{
		{Named{Name: "name"}, Labeled{}, "1.2.3.4"},
	})
	time.Sleep(time.Millisecond*50)

	pc.Delete("name", "namespace2")
	assert.EventuallyEqual(t, fetcherSorted(BadPods), []SimplePod{
		{Named{Name: "name"}, Labeled{}, "1.2.3.4"},
	})
	time.Sleep(time.Millisecond*50)
}


func TestIndexMerge(t *testing.T) {
	return
	opts := testOptions(t)
	dumpOnFailure(t, opts.Debugger())
	c := kube.NewFakeClient()
	kpc := kclient.New[*corev1.ConfigMap](c)
	cmt := clienttest.Wrap(t, kpc)
	cms := krt.WrapClient[*corev1.ConfigMap](kpc, opts.WithName("ConfigMaps")...)
	index := krt.NewIndex[string, *corev1.ConfigMap](cms, "key", func(o *corev1.ConfigMap) []string {
		return []string{o.Data["key"]}
	})
	c.RunAndWait(opts.Stop())
	merged := krt.SafeMerge(index, func(ctx krt.HandlerContext, objects []*corev1.ConfigMap) **corev1.ConfigMap {
		log.Errorf("howardjohn: compute %v configmaps", len(objects))
		cms := slices.SortBy(objects, (*corev1.ConfigMap).GetName)
		if len(cms) == 0 {
			return nil
		}
		base := cms[0].DeepCopy()
		for _, cm := range cms[1:] {
			base.Data["value"] = base.Data["value"] + "," + cm.Data["value"]
		}
		return &base
	})
	//merged := krt.NewCollection(index.AsCollection(),
	//	func(ctx krt.HandlerContext, io krt.IndexObject[string, *corev1.ConfigMap]) **corev1.ConfigMap {
	//		log.Errorf("howardjohn: COMPUTE %v, %v", io.Key, len(io.Objects))
	//		cms := slices.SortBy(io.Objects, (*corev1.ConfigMap).GetName)
	//		if len(cms) == 0 {
	//			return nil
	//		}
	//		base := cms[0].DeepCopy()
	//		for _, cm := range cms[1:] {
	//			base.Data["value"] = base.Data["value"] + "," + cm.Data["value"]
	//		}
	//		return &base
	//	}, opts.WithName("merged")...)
	merged.Register(func(o krt.Event[*corev1.ConfigMap]) {
		log.Errorf("howardjohn: %v %v", o.Items(), krt.GetKey(o.Items()[0]))
	})

	tt := assert.NewTracker[string](t)
	index.AsCollection().Register(TrackerHandler[krt.IndexObject[string, *corev1.ConfigMap]](tt))

	assertion := func(name, key, value string) {
		t.Helper()
		assert.EventuallyEqual(t, func() *corev1.ConfigMap {
			v := merged.GetKey(name)
			log.Errorf("howardjohn: get key %v %v", name, v)
			log.Errorf("howardjohn: list %v", merged.List())
			if len(merged.List()) > 0 {
				log.Errorf("howardjohn: list key %v", krt.GetKey(merged.List()[0]))
			}
			return ptr.Flatten(v)
		}, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: name}, Data: map[string]string{"key": key, "value": value}})
	}

	cmt.CreateOrUpdate(&corev1.ConfigMap{
		ObjectMeta: metav1.ObjectMeta{Name: "a"},
		Data:       map[string]string{"key": "key1", "value": "valuea"},
	})
	assertion("a", "key1", "valuea")
	tt.WaitOrdered("add/key1")

	cmt.CreateOrUpdate(&corev1.ConfigMap{
		ObjectMeta: metav1.ObjectMeta{Name: "b"},
		Data:       map[string]string{"key": "key1", "value": "valueb"},
	})
	assertion("a", "key1", "valuea,valueb")
	tt.WaitOrdered("add/key1")

	cmt.CreateOrUpdate(&corev1.ConfigMap{
		ObjectMeta: metav1.ObjectMeta{Name: "b"},
		Data:       map[string]string{"key": "key2", "value": "valueb"},
	})
	assertion("a", "key1", "valuea")
	assertion("b", "key2", "valueb")
	tt.WaitUnordered("add/key2", "add/key1")

	cmt.CreateOrUpdate(&corev1.ConfigMap{
		ObjectMeta: metav1.ObjectMeta{Name: "a"},
		Data:       map[string]string{"key": "key2", "value": "valuea"},
	})
	assertion("a", "key2", "valuea,valueb")
	tt.WaitUnordered("delete/key1", "add/key2")

}

Additionally, Itried to make a wrapper to allow safe merging:


type WithKeyPrefix[T any] struct {
	KeyPrefix string
	Inner T
}

func (w WithKeyPrefix[T]) ResourceName() string {
	return w.KeyPrefix + "/" + GetKey(w.Inner)
}

func SafeMerge[K comparable, O any](
	idx Index[K, O],
	f func(ctx HandlerContext, objects []O) *O,
	opts ...CollectionOption,
) Collection[O] {
	base := NewCollection(idx.AsCollection(), func(ctx HandlerContext, i IndexObject[K, O]) *WithKeyPrefix[O] {
		merged := f(ctx, i.Objects)
		prefix := toString(i.Key)
		if merged == nil {
			return nil
		}
		return &WithKeyPrefix[O]{
			KeyPrefix: prefix,
			Inner: *merged,
		}
	}, opts...)
	mapped := MapCollection(base, func(t WithKeyPrefix[O]) O {
		return t.Inner
	})
	return mapped
}

However, this does not work due to MapCollection requiring the input and output key to be the same. I added an assertion to verify this.

Krt places a number of un-checked requirements on collections.

This PR introduces a new knob to turn on assertions to be used during
development. IT IS OFF for production
@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Jun 4, 2025
@istio-testing
Copy link
Copy Markdown
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2025
@howardjohn howardjohn marked this pull request as ready for review June 4, 2025 16:39
@howardjohn howardjohn requested a review from a team as a code owner June 4, 2025 16:39
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 4, 2025
@howardjohn howardjohn added the cherrypick/release-1.26 Set this label on a PR to auto-merge it to the release-1.26 branch label Jun 4, 2025
@istio-testing
Copy link
Copy Markdown
Collaborator

@howardjohn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integ-ambient-mc_istio b8f2a10 link false /test integ-ambient-mc
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

)

// EnableAssertions, if true, will enable assertions. These typically are violations of the krt collection requirements.
const EnableAssertions = false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How/where does one enable these assertions?

@istio-testing istio-testing merged commit d8748b7 into istio:master Jun 4, 2025
29 of 30 checks passed
@istio-testing
Copy link
Copy Markdown
Collaborator

In response to a cherrypick label: #56510 failed to apply on top of branch "release-1.26":

Applying: krt: add assertions
Using index info to reconstruct a base tree...
M	pkg/kube/krt/collection.go
M	pkg/kube/krt/collection_test.go
A	pkg/kube/krt/map.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/kube/krt/map.go deleted in HEAD and modified in krt: add assertions. Version krt: add assertions of pkg/kube/krt/map.go left in tree.
Auto-merging pkg/kube/krt/collection_test.go
CONFLICT (content): Merge conflict in pkg/kube/krt/collection_test.go
Auto-merging pkg/kube/krt/collection.go
CONFLICT (content): Merge conflict in pkg/kube/krt/collection.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 krt: add assertions

@istio-testing
Copy link
Copy Markdown
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #56516

@keithmattix keithmattix mentioned this pull request Jun 5, 2025
4 tasks
howardjohn added a commit to howardjohn/istio that referenced this pull request Jun 13, 2025
Krt places a number of un-checked requirements on collections.

This PR introduces a new knob to turn on assertions to be used during
development. IT IS OFF for production

(cherry picked from commit d8748b7)
istio-testing pushed a commit that referenced this pull request Jun 16, 2025
Krt places a number of un-checked requirements on collections.

This PR introduces a new knob to turn on assertions to be used during
development. IT IS OFF for production

(cherry picked from commit d8748b7)
fjglira pushed a commit to fjglira/istio that referenced this pull request Sep 26, 2025
* upstream/master: (28 commits)
  Automator: update common-files@master in istio/istio@master (istio#56545)
  Automator: update proxy@master in istio/istio@master (istio#56544)
  Automator: update go-control-plane in istio/istio@master (istio#56543)
  Automator: update proxy@master in istio/istio@master (istio#56540)
  Automator: update ztunnel@master in istio/istio@master (istio#56532)
  Ambient: In ambient index, filter configs by revision (istio#56477)
  Automator: update istio/client-go@master dependency in istio/istio@master (istio#56539)
  Automator: update proxy@master in istio/istio@master (istio#56538)
  Automator: update common-files@master in istio/istio@master (istio#56537)
  optimization: allow for lazy sidecar initialization (istio#47221)
  static collection eager indexes (istio#56530)
  fix typo in flag (istio#56534)
  feat: enable support for proxy protocol on status port (istio#55986)
  remove finding of pods by IP (istio#56502)
  Automator: update proxy@master in istio/istio@master (istio#56528)
  migrate file monitor to krt (istio#55970)
  Automator: update istio/client-go@master dependency in istio/istio@master (istio#56525)
  Automator: update ztunnel@master in istio/istio@master (istio#56518)
  Fix crash in merging http routes (istio#56499)
  krt: add assertions (istio#56510)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherrypick/release-1.26 Set this label on a PR to auto-merge it to the release-1.26 branch release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants