Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chains section in TektonConfig gets override with default #2160

Closed
gbenhaim opened this issue May 19, 2024 · 2 comments · Fixed by #2269
Closed

Chains section in TektonConfig gets override with default #2160

gbenhaim opened this issue May 19, 2024 · 2 comments · Fixed by #2269
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@gbenhaim
Copy link

Expected Behavior

User provided configuration for Tekton Chains should be preserved.

Actual Behavior

When creating a TektonConfig with user provided configurations for chains, the operator overrides the values in the TektonConfig.

Example config the gets overriden:

---
apiVersion: operator.tekton.dev/v1alpha1
kind: TektonConfig
metadata:
  name: config
spec:
  profile: basic
  targetNamespace: tekton-pipelines
  pruner:
    resources:
    - pipelinerun
    - taskrun
    keep: 100
    schedule: "0 8 * * *"
  chain:
    artifacts.oci.storage: oci
    artifacts.pipelinerun.format: in-toto
    artifacts.pipelinerun.storage: oci
    artifacts.taskrun.format: in-toto
    artifacts.taskrun.storage: ""
    transparency.enabled: "false"

Steps to Reproduce the Problem

  1. Install the Tekton operator
  2. Create a TektonConfig with custom values for chains (for example the config above)

Additional Info

  • Kubernetes version:

    Output of kubectl version:

Server Version: version.Info{Major:"1", Minor:"29", GitVersion:"v1.29.2", GitCommit:"4b8e819355d791d96b7e9d9efe4cbafae2311c88", GitTreeState:"clean", BuildDate:"2024-02-14T22:24:00Z", GoVersion:"go1.21.7", Compiler:"gc", Platform:"linux/amd64"}

  • Tekton Pipeline version:

    Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

Client version: 0.33.0
Chains version: v0.20.0
Pipeline version: v0.56.1
Triggers version: v0.26.1
Operator version: v0.70.0

I think that the issue is that the pipelines operator runs its pre-upgrade logic after the tetkon config is created, but before it creates the TektonChains CR (https://github.com/tektoncd/operator/blob/main/pkg/reconciler/shared/tektonconfig/upgrade/pre_upgrade.go#L33), because of that the default values of chains configuration are written to the tekton config.
I saw the the pre-upgrade logic is running by turning on debug logging:

{"level":"debug","logger":"tekton-operator-lifecycle.upgrade","caller":"upgrade/upgrade.go:89","msg":"executing pre upgrade functions","commit":"2014719-dirty","knative.dev/pod":"tekton-operator-54f9b585dd-xlv8f","knative.dev/controller":"github.com.tektoncd.operator.pkg.reconciler.shared.tektonconfig.Reconciler","knative.dev/kind":"operator.tekton.dev.TektonConfig","knative.dev/traceid":"e29a3087-faa9-47f9-9db6-2d068254dbf5","knative.dev/key":"config","numberOfFunctions":2}
{"level":"debug","logger":"tekton-operator-lifecycle.upgrade","caller":"upgrade/upgrade.go:105","msg":"completed pre upgrade execution","commit":"2014719-dirty","knative.dev/pod":"tekton-operator-54f9b585dd-xlv8f","knative.dev/controller":"github.com.tektoncd.operator.pkg.reconciler.shared.tektonconfig.Reconciler","knative.dev/kind":"operator.tekton.dev.TektonConfig","knative.dev/traceid":"e29a3087-faa9-47f9-9db6-2d068254dbf5","knative.dev/key":"config"}

https://github.com/tektoncd/operator/blob/main/pkg/reconciler/shared/tektonconfig/upgrade/upgrade.go#L34
https://github.com/tektoncd/operator/blob/main/pkg/reconciler/shared/tektonconfig/upgrade/upgrade.go#L89

@gbenhaim gbenhaim added the kind/bug Categorizes issue or PR as related to a bug. label May 19, 2024
@PuneetPunamiya
Copy link
Member

So I used the above configuration for chains you mentioned in the issue and tested the upgrade from Operator version: v0.70.x to v0.71.x, i.e.

  chain:
    artifacts.oci.storage: oci
    artifacts.pipelinerun.format: in-toto
    artifacts.pipelinerun.storage: oci
    artifacts.taskrun.format: in-toto
    artifacts.taskrun.storage: ""
    transparency.enabled: "false"

With these values I have tested, it works fine and all the values and fields are preserved

Only if you add this one field artifacts.pipelinerun.enable-deep-inspection then it gets override with the default, that is a known bug and it is addressed in this pr #2179

@PuneetPunamiya
Copy link
Member

Hey @gbenhaim I'm able to reproduce this issue, working on the fix

PuneetPunamiya added a commit to PuneetPunamiya/operator that referenced this issue Jul 30, 2024
After making Chains install through TektonConfig, for backward compatibility
to keep chains configured data preserved a mechanism was added to add
the data from chains-config configMap to TektonConfig

But if Chains is installed through TektonConfig and some default chains
config, then those were getting over written because chains-config
configMap was empty.

Hence with this patch removing this support for backward compatibilty

Fixes: tektoncd#2160

Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>
PuneetPunamiya added a commit to PuneetPunamiya/operator that referenced this issue Jul 30, 2024
After making Chains install through TektonConfig, for backward compatibility
to keep chains configured data preserved a mechanism was added to add
the data from chains-config configMap to TektonConfig

But if Chains is installed through TektonConfig and some default chains
config, then those were getting over written because chains-config
configMap was empty.

Hence with this patch removing this support for backward compatibilty

Fixes: tektoncd#2160

Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>
PuneetPunamiya added a commit to PuneetPunamiya/operator that referenced this issue Jul 30, 2024
After making Chains install through TektonConfig, for backward compatibility
to keep chains configured data preserved a mechanism was added to add
the data from chains-config configMap to TektonConfig

But if Chains is installed through TektonConfig and some default chains
config, then those were getting over written because chains-config
configMap was empty.

Hence with this patch removing this support for backward compatibilty

Fixes: tektoncd#2160

Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>
tekton-robot pushed a commit that referenced this issue Jul 30, 2024
After making Chains install through TektonConfig, for backward compatibility
to keep chains configured data preserved a mechanism was added to add
the data from chains-config configMap to TektonConfig

But if Chains is installed through TektonConfig and some default chains
config, then those were getting over written because chains-config
configMap was empty.

Hence with this patch removing this support for backward compatibilty

Fixes: #2160

Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants