feat: Add ValidationPolicy & ValidationEvent

[karlkfi]

• Add ValidationPolicy: ExitEarly (default) - exit before apply/prune if any objects are invalid SkipInvalid - apply/prune valid objects & skip invalid ones
• Add ValidationEvent to be sent once for every invalid object or set of objects, when the SkipInvalid policy is selected. For ExitEarly, return an error for reverse compatibility.
• Add validation.Collector to simplify aggregating validation errors from multiple sources and extracting invalid object IDs.
• Add invalid objects to the TestContext so they can be retained in the inventory (only if already present). This primarily applies to invalid annotations and dependencies. Objects without name or kind should never be added to the inventory.
• Update Solver to use validation.Collector and filter invalid objects.
• Add e2e test for invalid objects.
• Update Printers to handle ValidationEvent
• Add ExternalDependencyError & InvalidAnnotationError to make it easier to handle and introspect validation errors.

fixes: https://github.com/kubernetes-sigs/cli-utils/issues/484

[karlkfi]

@seans3 The current existing behavior already exits early if the Validator errors. I don't understand why you think that's misguided. The code is littered with hundreds of places that assume that minimal validation has been done. It's not going to be a small change to fix that to skip invalid objects (which seems to be what you want).

What this PR primarily does is adds the ability to skip those invalid objects.

I also added more explicit depends-on & apply-time-mutation errors, but that's because these are client-side annotations and the server won't validate them. So if they're invalid and the applier continues without skipping them (the current behavior), then that's behaviorally incorrect. We should never completely ignore invalid depends-on & apply-time-mutation errors. We should either skip those invalid objects or exit early. Either way, the user needs to be notified. That's why I added the ValidationError, to make sure the user always gets notified, regardless of whether they chose SkipInvalid or ExitEarly.

Now that I've said that tho, I think the ApplyAll policy is probably only useful when not using depends-on or apply-time-mutation. Otherwise the behavior when they're invalid would be obviously incorrect. ApplyAll would basically be opting out of client-side validation and ignoring errors in client-side features. That's basically what you're championing when you're saying the applier should be more like kubectl and do less validation. The big difference is that kubectl doesn't have any client-side-only features like depends-on or apply-time-mutation.

[karlkfi]

I've made significant changes to the handling of dependency validation. Please re-review.

• Move dependency validation from Validator to SortObjs.
• Move SortObjs call from solver to applier/destroyer. This allows the resulting errors to be treated as validation errors.
• Modify SortObjs to return a MultiError so that all validation errors can be sent and invalid objects can be skipped or cause early exit.
• Add invalid objects to the TestContext so they can be retained in the inventory (only if already present). This primarily applies to invalid annotations and dependencies. Objects without name or kind should never be added to the inventory.
• Remove invalid objects from the applySet/pruneSet, rather than filtering and skipping them in the apply/prune/wait tasks. This reduces the number of events that are redundant now with validation events.
• Handle CyclicDependencyError as a validation error. It applies to multiple objects, instead of just one, like ValidationError.
• Handle validation of dependency errors in destroyer as well as the applier.
• Replace many -OrDie object commands that were panicing when an invalid object was found. Add ValidateObjMetadata to do the validation that the -OrDie functions were doing.
• Replace MultiValidationError with a more generic MultiError.
• Simplify ValidationError to optionally wrap MultiError.
• Modify ValidationError output to be easier to read.
• Add e2e test for invalid object handling.

I still have more tests to add.

[karlkfi]

Extracted replacement of -OrDie (panic) functions to https://github.com/kubernetes-sigs/cli-utils/pull/505

This should vastly reduce the number of files changed in this PR, once rebased.

[karlkfi]

This has been rebased on top of https://github.com/kubernetes-sigs/cli-utils/pull/505 and https://github.com/kubernetes-sigs/cli-utils/pull/506

[karlkfi]

Extracted the MultiError changes to https://github.com/kubernetes-sigs/cli-utils/pull/508

[karlkfi]

Ok, I did a big refactor to move the sorting bakc into the Solver. And got the tests finally passing again.

TODO:

• https://github.com/kubernetes-sigs/cli-utils/pull/511
• https://github.com/kubernetes-sigs/cli-utils/pull/510
• https://github.com/kubernetes-sigs/cli-utils/pull/512
• https://github.com/kubernetes-sigs/cli-utils/pull/513
• Add test case for InvSetTasks filtering invalid objects
• Add typed ExternalDependencyError error

[karlkfi]

/hold

[karlkfi]

/unhold

All TODOs finished. Ready for another review pass.

[mortent]

I'll remove the approval just to make sure @seans3 also has a chance to look at it before it merges. /approve cancel

[karlkfi]

I do not believe exiting early should be the default behavior. Even if this behavior exists now, it is incorrect and should be fixed to continue on error. Exiting early is also not consistent with the kubectl apply behavior which continues on error.

It's likely that ConfigSync and kpt will both initially use ExitEarly when adopting this new code. They may later switch to either hard coding SkipInvalid OR piping the option through to the user. Does that affect your default suggestion @seans3 ?

[karlkfi]

FWIW, I do not consider ExitEarly to a be a bug. Its a feature which arises from the desire to group related changes. Of course, Kubernetes doesn't have atomic applies or groups, but you can at least avoid applying a Deployment if its Secret, ServiceAccount, or ConfigMap is invalid. This behavior is highly desired when you take the time to granularly group your objects.

On the other hand, SkipInvalid is attractive when you haven't or don't intend to group your objects. This comes up regularly when multiple teams share a single config repo or delegate config management to a central SRE/DevOps/Ops team.

I'm ok with either being default. I don't think it really matters. There are pros and cons on both sides.

• ExitEarly is reverse compatible, which is desirable for existing kpt users because they're releasing beta versions and breaking changes aren't being released as major versions yet.
• SkipInvalid might be more desirable to new kpt users who are more familiar with kubectl.
• Any wrapping client can select their own default... So as long as we communicate breaking changes, I'm not against them.

I'm biasing slightly towards reverse compatibility here, but if anyone else wants to chime in with a 3rd vote I don't mind making the change.

[mortent]

I would probably prefer that we keep the current behavior here (so ExitEarly is the default), but I don't feel strongly about it.

[haiyanmeng]

For Config Sync users, the default should be ExitEarly, which is closer to the existing validation behavior in Config Sync.

Supporting the SkipInvalid validation policy would require lots of work from Config Sync. Today, Config Sync validates the configs declared in a Git repo, and only sends the configs to cli-utils to apply after the configs pass all the validations (there are about several dozens of validations in Config Sync).

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karlkfi, seans3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cli-utils/blob/master/OWNERS)~~ [seans3] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment