feat(MeshHTTPRoute): add basic validation

[michaelbeaumont]

Checklist prior to review

• Link to docs PR or issue -- none
• Link to UI issue or PR -- none
• Is the issue worked on linked? -- MeshHTTPRoute basic api implementation #5470
• The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
• The PR will work for both Linux and Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
• Unit Tests -- none
• E2E Tests --
• Manual Universal Tests --
• Manual Kubernetes Tests --
• Do you need to update UPGRADE.md? --
• Does it need to be backported according to the backporting policy? --
• Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

Changelog: feat(policy): implement MeshHTTPRoute policy

[jakubdyszkiewicz]

Please add the tests

[michaelbeaumont]

@jakubdyszkiewicz maybe that validation runs at all, but IMO there isn't validation complicated enough to write tests for yet. I'll just end up with a list of kinds in a test file that matches the declared list of kinds in the validation file. Might as well just make sure the one list is correct.

[jakubdyszkiewicz]

IMO there isn't validation complicated enough to write tests for yet

I disagree. A restricted set of kinds on targetRef + to is an important part of the policy validation. I don't see why this unreasonable test.

[michaelbeaumont]

I disagree. A restricted set of kinds on targetRef + to is an important part of the policy validation. I don't see why this unreasonable test.

Right but how do you check that the test is correct in the first place? You need to read the list of kinds that are listed in the tests. But there's already a perfectly readable, declarative list of kinds in the validation code, you can just read that instead when reviewing. There's no point to such a test IMO, it's not actually any safer.

[michaelbeaumont]

@jakubdyszkiewicz are there enough tests in this PR in your opinion?

[slonka]

Right but how do you check that the test is correct in the first place? You need to read the list of kinds that are listed in the tests.

I wouldn't do it that way. I would go to MADR to check if there is an entry about which targetRefs are supported (maybe re-think if that entry is correct). Testing it is a bit much but because we have general tests for targetRef and the supported list for MeshHTTPRoute is (as you said) perfectly readable but I would just make sure that madr matches what is in the validator.

[slonka]

Looks ok, one thing I noticed looking around the code is that weight is defined as "int" but I think it should be >0

[jakubdyszkiewicz]

I'd also add an element in to, but yes. It's enough. The helper is also pretty cool :)

[michaelbeaumont]

Looks ok, one thing I noticed looking around the code is that weight is defined as "int" but I think it should be >0

I think we want to allow weights of 0 for turning off and on a backend but yeah actually it should be a uint

[michaelbeaumont]

Waiting on #5652

[michaelbeaumont]

Right but how do you check that the test is correct in the first place? You need to read the list of kinds that are listed in the tests.

I wouldn't do it that way. I would go to MADR to check if there is an entry about which targetRefs are supported (maybe re-think if that entry is correct). Testing it is a bit much but because we have general tests for targetRef and the supported list for MeshHTTPRoute is (as you said) perfectly readable

I mean if you had exhaustive tests for the supported kinds, you'd need to read which ones are in the tests to make sure the tests are correct. Which ones are correct you'd know from reading the MADR.

but I would just make sure that madr matches what is in the validator.

Which is what I mean by skipping the exhaustive tests entirely