fix: validate resource kind. #40
Conversation
| if validation_request.request.kind.kind != apicore::Pod::KIND { | ||
| warn!(LOG_DRAIN, "Policy validates Pods only. Accepting resource"; "kind" => &validation_request.request.kind.kind); | ||
| return kubewarden::accept_request(); | ||
| } |
There was a problem hiding this comment.
Do we need that? If the passed object is not a Pod then the code on line 42 will fail
There was a problem hiding this comment.
Not really. While we are testing the docs, we discover that the parse the data as pod. Even if the resource passed in the unit test is not a pod kind. Therefore, the Err clause is never reached. It fails in the name validation. Considering the serde docs, as far as I can see, the Deserialize implementation just ignores fields missing or extra fields found and return a object with all data that it can find in the data being parsed.
There was a problem hiding this comment.
This change matches the specified behavior of the readme: "if a different type of resource is evaluated, it will be accepted.". But maybe the comment before the if could be clearer, or point that this is the behaviour we defined. Right now for a newcomer reusing the template, it may be understood as something that is needed to deal with k8s api in Rust, which is not.
There was a problem hiding this comment.
Ok, I take back my initial comment. I see what is happening with serde. I'm fine adding the check from above, but the comment is wrong. There's no test failing right now on the main branch, the failure you might have seen before was caused by something different.
I think the extra check can be added, and the comment should explain to the user that serde will not fail the deserialization if another k8s object is received. This is also done to play on the safe side, because a properly configured policy will receive only the events related to the resources it can understand.
There was a problem hiding this comment.
Fair enough, the comment make sense only for people following the tutorial.
Furthermore, the issue show up only when the user follows the writing policy tutorial. Because the test is not validating the right code. Take a look on this in new policy with no changes:
cargo test tests::accept_request_with_non_pod_resource -- --nocapture
Finished test [unoptimized + debuginfo] target(s) in 0.03s
Running unittests src/lib.rs (target/debug/deps/demo-1db451a0e87df990)
running 1 test
{"column":5,"file":"src/lib.rs","level":"info","line":33,"message":"starting validation","policy":"sample-policy"}
{"column":17,"file":"src/lib.rs","level":"info","line":53,"message":"accepting resource","policy":"sample-policy"}
test tests::accept_request_with_non_pod_resource ... okAs we can see in the output, the test is passing but the code is not returning in the right line. It's passing because the name of the resource is not consider invalid. But not because of the kind of the resource. During the tutorial we change the validation logic to consider the settings. After this change the test start to fail. Because the resource is consider as pod. That's why this validation is necessary.
In order to allow only pod resource in the template policy, it's necessary to check the resource kind before trying to parse. This is necessary because the parse function can be able to parse it even if the resource is not pod. This is necessary to improve the docs about writing policy in rust. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Description
In order to allow only pod resource in the template policy, it's necessary to check the resource kind before trying to parse. This is necessary because the parse function can be able to parse it even if the resource is not pod. This is necessary to improve the docs about writing policy in rust.