CLOUDP-280230: Network peering translation layer #1884
Conversation
Including fuzzing based conversion testing Signed-off-by: jose.vazquez <[email protected]>
josvazg
force-pushed
the
CLOUDP-280230/net-peering-crd
branch
from
78de4b3
to
23da286
Compare
| VnetName: pointer.SetOrNil(peer.VNetName, ""), | ||
| } | ||
| default: | ||
| panic(fmt.Errorf("unsupported provider %q", peer.ProviderName)) |
There was a problem hiding this comment.
i am sceptical this should panic 🤔 are we really considering this being a programmatic error?
There was a problem hiding this comment.
I am on the fence as well. There are 2 ways to hit this:
- Our validation fails to filter the provider name in the CRD.
- Atlas sends an incorrect value.
For the first one we could panic, is our fault. But for the second, I guess we should error out instead.
There was a problem hiding this comment.
@josvazg given this is driven by user input or Atlas responses I am in strong favor of returning an error.
A panic is valid if for we wire things incorrectly internally, i.e. use the "wrong indexer name" or something like that. But anything that arrives "via wire" from the user or Atlas to AKO shouldn't panic unless there are very very strong reasons.
There was a problem hiding this comment.
Our validation fails to filter the provider name in the CRD.
If we apply the rule I mentioned above this shouldn't panic as the provider name is returned from Atlas, aka it arrived externally to us.
There was a problem hiding this comment.
Atlas sends an incorrect value.
same rule as above. This is Atlas output, not an internal wiring error, it shouldn't panic.
There was a problem hiding this comment.
I will introduce an error return value and change all the code accordingly.
All Submissions: