Should invalid blueprints be excluded from selection

[squeedee]

Would love some feedback on this as I'm unsure if it's a bug

If you have two supply chains on the cluster, and one is not Ready: True, should it still be considered for ‘selection’ of a workload?

If not: the workload selects on something that is valid, but the operator’s intention might have been for it to select for the currently broken supply chain

If it should: then we need to make sure we present the error in the workloads status (we do not right now, selection fails and the workload has an “Unknown” status

[scothis]

Good question.

I would tend to lean towards saying that the readiness of a blueprint should not impact selection, but should pause processing of the selected workload/deliverable with an appropriate status condition.

I lean this way because switching the blueprint can be dramatic. It's not uncommon for resource to briefly become unhealthy and then recover. The impact of pausing the processing of a workload is minor compared to the potentially large impact of deleting all of the stamped resources and creating new stamped resources. Impacts like:

• dropping build caches within a kpack Image
• dropping the last known good source if the latest source is failing tests (same with tests for images, etc)
• dropping the running workload in the inner-loop

[squeedee]

Sorry now I realize there is a choice here.

If the workload has a status.supplyChainRef, and that supply chain is not ready, we update the status of the workload to say so and re-queue.
If it doesn't - we select for the unambiguous "READY" supply chains?

[scothis]

failing on selector definitions

Can that be detected and rejected at admission time? That way it will never be a problem at run time. A value that can never be valid should be rejected via a validating webhook (or similar), while resources that could possibly become valid should be admitted.

If it doesn't - we select for the unambiguous "READY" supply chains?

I'm suggesting that selection should never be based on blueprint readiness to avoid the flip-flopping effect.

If the workload has a status.supplyChainRef, and that supply chain is not ready, we update the status of the workload to say so and re-queue

Trying to be clever about current state vs historical state often ends up backfiring.

[squeedee]

I wasnt thinking about the lifecycle; so the point you're making is that if it's not legal, then the old state stands. I hadn't made the connection. Definitely planned for it to be a validating web hook, but was still thinking in terms of the owner reconciler. Thanks!

[emmjohnson]

There are a few cases that we will never be able to reject at admission time. For example, SelectorMatchFields could have some key that does not exist. We do our best to validate the keys against ValidWorkloadPaths and ValidWorkloadPrefixes, but someone could still make this blow up at runtime -

cartographer/pkg/apis/v1alpha1/cluster_supply_chain_validations.go

Line 149 in bbc2410

err = validateFieldSelectorRequirements(selectors.SelectorMatchFields, ValidWorkloadPaths, ValidWorkloadPrefixes)

[squeedee]

So to be clear, we can match most validations to possible causes for a supply chain to cause the "Find Matching Supply Chain" process to choke up.

Question: Is this validation a reason to "blow up" or a reason to not select?

My thoughts:

• Invalid jsonpath syntax should be something we can validate, and thus reject if it appears during SC create/update. eg spec.[] is illegal, as is (probably)spec[{{}}]=-!
• JsonPath that does not reference a valid workload path, or include a valid workload prefix, can also be rejected in the webhook (if we don't test the JsonPath)?
• JsonPath that references a nonexistant node or a non-path is simply considered "Not Matching" so should not be a problem?

Am I missing any scenarios?