fix(MeshTrace): invalid sampling default values

[michaelbeaumont]

Checklist prior to review

• Link to relevant issue as well as docs and UI issues --
• This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
• Tests (Unit test, E2E tests, manual test on universal and k8s) --
  • Don't forget ci/ labels to run additional/fewer tests
• Do you need to update UPGRADE.md? --
• Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label) --

Changelog: skip

[slonka]

Don't we need to add a test to prevent re-introducing this? How did this pass validation tests the first time?

ok so the original PR never changed the tests to check if it work 👀

kuma/pkg/plugins/policies/meshtrace/api/v1alpha1/validator_test.go

Line 49 in 5d0f064

overall: 80

#8557

[michaelbeaumont]

Don't we need to add a test to prevent re-introducing this? How did this pass validation tests the first time?

How could we ever reintroduce this?

So would we have validation that sets every object field to the empty value to test if the default values of that field's fields are correct?

[michaelbeaumont]

ok so the original PR never changed the tests to check if it work 👀

In particular this only shows up if sampling: is set but not all the fields are set. (and only on Kubernetes)

[slonka]

So would we have validation that sets every object field to the empty value to test if the default values of that field's fields are correct?

That will also be beneficial. It's just that if we change the default from "100" to "100%" then we NEED to change the unit tests as well.

[michaelbeaumont]

It's just that if we change the default from "100" to "100%" then we NEED to change the unit tests as well.

What exactly would the test test, just that the default values we enter aren't invalid?

Note this also only shows up with Kubernetes because of #6070

[slonka]

@michaelbeaumont can you add an error case with explicit "80%"?

[michaelbeaumont]

@michaelbeaumont can you add an error case with explicit "80%"?

What would that test?

[slonka]

What would that test?

well exactly the problem that we hit 🤔 EDIT: I agree that now it's of not that much value but it would've been useful in the past

[github-actions]

backporting to release-2.8 with action

backporting to release-2.7 with action
backporting to release-2.6 with action
backporting to release-2.5 with action