search

kubernetes-sigs / cli-utils

This repo contains binaries that built from libraries in cli-runtime.
Apache License 2.0
155 stars 78 forks source link

fix: Don't panic when ObjMetadata is invalid (missing kind/name) #505

Closed karlkfi closed 2 years ago

karlkfi commented 2 years ago

This PR removes a panic that was duplicating logic contained in the object.Validator.

This change paves the way for us to skip invalid objects, instead of exiting early: https://github.com/kubernetes-sigs/cli-utils/pull/488

mortent commented 2 years ago

I agree that being able to continue applying other resources even if one is invalid is the right direction.

Removing the ToObjMetaOrDie functions and allowing invalid resources to pass through the initial validation means that we can no longer assume that ObjMetadata instances are valid. But in some cases, such as updating the inventory list, we do want to make sure that the data we write to the cluster is valid. My understanding is that we will use the ValidateObjMetadata function when we need to guard against invalid ObjMetadata object. Is that correct? And do we have a good idea about where we need to do this check?

The ValidateObjMetadata function currently doesn't verify the namespace of the resource, i.e. make sure that cluster-scoped resources doesn't have the namespace set and that namespace-scoped resources do. Maybe not in this PR, but we should probably validate this, as getting it wrong can lead to problems. The challenge is that we need to RESTMapper in order to check the namespacability of a resource with additional complexity if the type doesn't exist in the cluster yet.

karlkfi commented 2 years ago

Removing the ToObjMetaOrDie functions and allowing invalid resources to pass through the initial validation means that we can no longer assume that ObjMetadata instances are valid.

This isn't what's happening. Everything goes through the Validator before being applied, and the Validator checks the kind and name the same way CreateObjMetadata did. So the ObjMetadata will continue to be valid after validation.

The ValidateObjMetadata function currently doesn't verify the namespace of the resource, i.e. make sure that cluster-scoped resources doesn't have the namespace set and that namespace-scoped resources do. Maybe not in this PR, but we should probably validate this, as getting it wrong can lead to problems. The challenge is that we need to RESTMapper in order to check the namespacability of a resource with additional complexity if the type doesn't exist in the cluster yet.

The Validator also checks the namespace.

In fact, digging into the places that ValidateObjMetadata is used, we can probably just delete it. It's completely redundant.

karlkfi commented 2 years ago

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

Once rebased, this PR will just be the OrDie changes to avoid panic.

mortent commented 2 years ago

Removing the ToObjMetaOrDie functions and allowing invalid resources to pass through the initial validation means that we can no longer assume that ObjMetadata instances are valid.

This isn't what's happening. Everything goes through the Validator before being applied, and the Validator checks the kind and name the same way CreateObjMetadata did. So the ObjMetadata will continue to be valid after validation.

Yeah, I understand that we aren't removing any of the validation in this PR. But my understanding was that we want to eventually relax the early validation so we can continue applying the valid resources even if others fail validation (and I think that is a good thing). Is the idea that we will filter out any invalid resources early on even in this case, so we know that we will never end up with ObjMetadata created from resources that failed validation (seems so)? The primary area I can think of where it might be problematic with ObjMetadata that are created from resources that didn't pass validation is that we use those to populate the inventory.

karlkfi commented 2 years ago

The primary area I can think of where it might be problematic with ObjMetadata that are created from resources that didn't pass validation is that we use those to populate the inventory.

@mortent In this PR, validation is still required and causes early exit. So invalid objects won't make it into the inventory.

The next PR (https://github.com/kubernetes-sigs/cli-utils/pull/488) adds the option for SkipInvalid, instead of ExitEarly, and then also defends against adding invalid objects into the Inventory by adding them to the TaskContext and checking the TaskContext in the inventory set task.

karlkfi commented 2 years ago

Rebased on top of https://github.com/kubernetes-sigs/cli-utils/pull/506

mortent commented 2 years ago

/approve

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, karlkfi, mortent

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)~~ [mortent] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment