[pluggable-validation / 2]: implement core adjustment validation. #170
Conversation
| } | ||
|
|
||
| func (p *plugin) ValidateContainerAdjustment(ctx context.Context, req *ValidateContainerAdjustmentRequest) error { | ||
| if !p.events.IsSet(Event_VALIDATE_CONTAINER_ADJUSTMENT) { |
There was a problem hiding this comment.
nit: Could we make use of isContainerAdjustmentValidator() above since we're doing the same thing?
There was a problem hiding this comment.
Here, I'd prefer to keep, in that regard, the boilerplate to forward the request to the plugin identical to the other boilerplates. The is*-checker is used in a different context, where it is more readable to hide the test behind a function with a self-explanatory name.
There was a problem hiding this comment.
If there is a strong feeling against having both, then I think I'd rather get rid of isContainerAdjustmentValidator() and write it out in the other context.
klihub
force-pushed
the
devel/pluggable-validation/implement-validation
branch
4 times, most recently
from
8dbb423 to
66e2aa6
Compare
samuelkarp
left a comment
samuelkarp
left a comment
There was a problem hiding this comment.
This mostly looks good, just one comment about validator plugin invocation order and also not going to approve this until after #169 is merged.
klihub
force-pushed
the
devel/pluggable-validation/implement-validation
branch
from
66e2aa6 to
dc7034b
Compare
|
@klihub Can you rebase this branch on |
|
|
||
| for _, p := range r.validators { | ||
| wg.Add(1) | ||
| go func(p *plugin) { |
There was a problem hiding this comment.
Just a note for the future that we might want to revisit this slightly and think about whether we want to limit the concurrency a bit; might become an issue if there are many validator plugins. No need to change as part of this PR.
There was a problem hiding this comment.
Are there any places where we might need to update comments to explain that validation happens in parallel? (Just wondering if customers might try creating plugins with low
plugin_idxand wonder why they don't receive priority)
@chrishenzie Yes, I think it will be a good idea to emphasize in the documentation that during the validation phase there is no 'plugin ordering by index' as validating plugins are run in parallel.
But regarding users wondering "why they don't get priority", I think they should not. Any presumed ordering/priority would be relevant only if it was possible to force validation both to fail and to succeed. But our semantics is deliberately such that the latter is not possible.
There was a problem hiding this comment.
true and users of the api may want ordering regardless of our scheme... E.g. I will only approve after ___ approval, if my check is also valid. Allocate claim foo before claim foo'...
There was a problem hiding this comment.
typically one would have a set operating in parallel at a level where the higher levels must finish first? built-in, set-a.. set b with a dependency in set-a..
There was a problem hiding this comment.
E.g. I will only approve after ___ approval, if my check is also valid
The validations are a logical AND, so if any fails the container is rejected. I don't think you can model "I will only approve after $foo approval" since if $foo rejects the container is rejected anyway, and if $foo succeeds then you'd also succeed your validation?
Implement pluggable container adjustment validation. When validator plugins are present, use them to validate the collected adjustments, failing container creation if any validation fails. For adjustment validation plugins receive the pod, the pristing un- adjusted container, the collected container adjustments, information about which plugins adjusted what container parameters, and the list of plugins consulted for the adjustments. The plugin can then choose to accept or reject the adjustments. Accepting or rejecting adjustments are transactional. Either all or none of the adjustments are accepted, together with the container creation request. IOW, rejecting an adjustment results in a failed container creation request. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/pluggable-validation/implement-validation
branch
from
dc7034b to
e9f95d0
Compare
@samuelkarp @chrishenzie Okay, so while adding test cases for the default validator in #171 it became obvious that running validation with multiple plugins and at least one of them being a builtin one (the default validator) is not parallel/goroutine safe. But the reason is not protobuf itself. Rather it is simply that in the current implementation a query/lookup of a field owner of a particular container is (counterintuitively) not a read-only operation if the container has not been adjusted at all. In that case it has no FieldOwners entry at all and that then gets implicitly created during the lookup (with a write to a map). I fixed this in a separate commit in #171 prior to adding the test cases. |
4 participants
This PR is stacked on #169.
This PR is the second in a stack which splits up #163 into multiple PRs, which collectively implement configurable restrictions using the proposed pluggable validation API and a builtin default validator plugin.
This PR defines the validation API and implements the core validation logic for container adjustments.
/cc @chrishenzie