Hi @PuneetPunamiya! Thanks for reporting this.

An empty config value means the default value is loaded. The config values are documented here. The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

Chains uses the mentioned annotation to indicate that it is done processing the resource. So if it sees a resource with that annotation, then it just ignores it. Otherwise, it may constantly keep retrying. signed is probably not the best name for this though.

I think the question here is how should Chains behave if it is misconfigured. Should it stay out of the way, or should it make this obvious to users creating TaskRun and PipelineRun resources? Or maybe, the Chains controller should have some sort of config validation at start up, making it obvious to whoever is administrating the controller.

Thanks @lcarva for the response 👍🏻

An empty config value means the default value is loaded.

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

So if I'm not getting you wrong, we do have signing-secret created once chains is installed, but I think what you meant is the signing-secret was not updated/configured with the cosign cli and that is why it gives the following error. Also once we update the keys by executing cosign generate .. command, the signing fails 👍🏻

I think the question here is how should Chains behave if it is misconfigured.

Yes exactly, I purposely added no config values from whic are documented here assuming the signing should fail

the Chains controller should have some sort of config validation at start up, making it obvious to whoever is administrating the controller.

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

Do you mean here? If so, +1.

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

A warning may be easy to miss. Is preventing the controller from starting (by crashing it?) too heavy handed?

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

Do you mean here? If so, +1.

Yeah, I meant here only

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

A warning may be easy to miss. Is preventing the controller from starting (by crashing it?) too heavy handed?

How about erroring out in the controller logs and controller not proceeding further steps instead of crashing it ?

An empty config value means the default value is loaded. The config values are documented here. The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

Chains uses the mentioned annotation to indicate that it is done processing the resource. So if it sees a resource with that annotation, then it just ignores it. Otherwise, it may constantly keep retrying. signed is probably not the best name for this though.

@lcarva i agree the signed is not the best name to indicate the processing of that resource; like you said, to a user chains.tekton.dev/signed=true indicates that chains was able to successfully sign the resource, hence this is very misleading.

what's the downside of setting chains.tekton.dev/signed=false/failed in this case?

what's the downside of setting chains.tekton.dev/signed=false/failed in this case?

Setting it to "failed" is probably the right behavior. It will be challenging to get it right as we need to distinguish from use cases where there's just nothing to sign, e.g. #458.

cc @wlynch, @chitrangpatel

wdyt about specifying a list of values that accurately describe the state? e.g.:

• chains.tekton.dev/signed=true
• chains.tekton.dev/signed=failed
• chains.tekton.dev/signed=skipped

while i agree that chains.tekton.dev/signed should have multiple states as above ^ , but also in the absence of any keys (which means chains has nothing to sign with), the controller should not watch / process any taskruns/pipelineruns at all - why waste resources in going through each workload and add chains.tekton.dev/signed=skipped annotation

one data point to support this is #979 where we're already seeing slowness from chains controller due to needing to process a large number of tekton workloads - while that issue will be fixed differently, the chains controller should try to not watch workloads if not required, wdyt?

I agree with @concaf here. Another way to look at it is the different personas that rely on Chains.

There's the user who is running Pipelines/Tasks, and the admin who is responsible for configuring Tekton Chains. In some cases, these may be the same person, but think we should assume that they are separate people with different access levels.

If Chains is misconfigured, there's nothing the user can do. The admin is the one responsible. With this premise, if Chains is misconfigured, we should make that obvious to the admin, while also minimizing the impact to the user.

This is why I suggested earlier on that if Chains is misconfigured, it should just crash. This should effectively allow the admin to fix things and delay Chains from processing any *Run resources. Once the issue is resolved, Chains will catch up processing the *Run resources.

We discussed this in today's meeting. The overall consensus is to not proceed with the chains.tekton.dev/signed=skipped annotation approach. One of the promises of Chains is that it processes every resource in the cluster (if Chains is properly configured). This would violate that.

An approach that makes it obvious the controller is misconfigured is ideal, e.g. performing a basic config check at start up and crash if anything seems wrong (missing cosign key for example). This won't catch all the cases, but it's a starting point.

Additionally, we could create a mechanism that propagates errors back to the user. This could be done by using a new annotation or by adding a new status condition on the resource. This should bridge the gap of issues which are within the user's control to fix, e.g. malformatted linked secret.