Make sure we pass the live config to the conversion webhook #6450

vdemeester · 2023-03-29T04:09:01Z

Changes

This makes sure we pass the "live" configuration to the conversion webhook. Without this, that controller doesn't see any changes on the feature-flags configmap, and thus only uses the default value.

This is a relatively big problem if some part of the conversion depends on it (which was the case for embedded-status for example) as it would cause loss of data.

Fixes #6443
Fixes possibly other "weird" behavior we've seen in the past or present (like #5964).

As this has been there for as long as we had conversion webhook, we need backport this to all supported LTS at least (0.44.0 and 0.41.0).

/kind bug
/cc @JeromeJu @lbernick @tektoncd/core-maintainers

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Make sure the conversion webhook sees the live configmaps instead of the default ones

This makes sure we pass the "live" configuration to the conversion webhook. Without this, that controller doesn't see any changes on the `feature-flags` configmap, and thus only uses the default value. This is a relatively big problem if some part of the conversion depends on it (which was the case for `embedded-status` for example) as it would cause loss of data. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester · 2023-03-29T04:09:18Z

/cherry-pick release-v0.44.x

tekton-robot · 2023-03-29T04:09:19Z

@vdemeester: once the present PR merges, I will cherry-pick it on top of release-v0.44.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.44.x
/cherry-pick release-v0.41.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2023-03-29T04:09:53Z

/cherry-pick release-v0.41.x

tekton-robot · 2023-03-29T04:09:55Z

@vdemeester: once the present PR merges, I will cherry-pick it on top of release-v0.41.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.41.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2023-03-29T04:10:29Z

We may as well backport this to 0.45 and 0.46 to be honest 👼🏼

vdemeester · 2023-03-29T04:10:42Z

/cherry-pick release-v0.45.x

tekton-robot · 2023-03-29T04:10:43Z

@vdemeester: once the present PR merges, I will cherry-pick it on top of release-v0.45.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.45.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2023-03-29T04:10:56Z

/cherry-pick release-v0.46.x

tekton-robot · 2023-03-29T04:10:57Z

@vdemeester: once the present PR merges, I will cherry-pick it on top of release-v0.46.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.46.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2023-03-29T04:31:32Z

/test check-pr-has-kind-label

tekton-robot · 2023-03-29T04:31:33Z

@vdemeester: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-beta-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2023-03-29T04:50:45Z

/retest

JeromeJu

Thanks for the speedy catch @vdemeester

lbernick · 2023-03-29T14:06:26Z

cmd/webhook/main.go

@@ -158,6 +158,9 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro
 		resolutionv1alpha1GroupVersion = resolutionv1alpha1.SchemeGroupVersion.Version
 		resolutionv1beta1GroupVersion  = resolutionv1beta1.SchemeGroupVersion.Version
 	)
+	// Decorate contexts with the current state of the config.
+	store := defaultconfig.NewStore(logging.FromContext(ctx).Named("config-store"))


do you know why in the other controllers, store.WatchConfigs is called within the inner function?

do you know why in the other controllers, store.WatchConfigs is called within the inner function?

Ah, that's a good point, I don't know, but it should be the same for all at least 😛 So I can move it there. It works in both cases, not sure what is the reason.

It's because of the difference in the outer function definitions.

Note newValidationAdmissionController takes in a parameter of type string and returns func which has two parameters (Context, Watcher) and returns controller.Impl whereas newConversionController takes two parameters (Context, Watcher) and returns controller.Impl.

func newValidationAdmissionController(name string) func(context.Context, configmap.Watcher) *controller.Impl { ... }

func newConversionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { ... }

Does it make sense?

@pritidesai I think it does 😝

tekton-robot · 2023-03-29T14:06:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, lbernick

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~~ [lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pritidesai · 2023-03-29T16:16:55Z

/lgtm

tekton-robot · 2023-03-29T16:59:57Z

@vdemeester: new pull request created: #6460

In response to this:

/cherry-pick release-v0.44.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2023-03-29T17:00:39Z

@vdemeester: new pull request created: #6461

In response to this:

/cherry-pick release-v0.41.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2023-03-29T17:01:46Z

@vdemeester: new pull request created: #6462

In response to this:

/cherry-pick release-v0.45.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2023-03-29T17:03:29Z

@vdemeester: new pull request created: #6464

In response to this:

/cherry-pick release-v0.46.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot requested a review from JeromeJu March 29, 2023 04:09

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 29, 2023

tekton-robot requested review from lbernick and a team March 29, 2023 04:09

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 29, 2023

JeromeJu approved these changes Mar 29, 2023

View reviewed changes

lbernick approved these changes Mar 29, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2023

tekton-robot assigned pritidesai Mar 29, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2023

tekton-robot merged commit eae5c3a into tektoncd:main Mar 29, 2023

tekton-robot mentioned this pull request Mar 29, 2023

[release-v0.44.x] Make sure we pass the live config to the conversion webhook #6460

Merged

tekton-robot mentioned this pull request Mar 29, 2023

[release-v0.41.x] Make sure we pass the live config to the conversion webhook #6461

Merged

tekton-robot mentioned this pull request Mar 29, 2023

[release-v0.45.x] Make sure we pass the live config to the conversion webhook #6462

Merged

tekton-robot mentioned this pull request Mar 29, 2023

[release-v0.46.x] Make sure we pass the live config to the conversion webhook #6464

Merged

vdemeester deleted the 6437-embedded-status branch March 30, 2023 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure we pass the live config to the conversion webhook #6450

Make sure we pass the live config to the conversion webhook #6450

vdemeester commented Mar 29, 2023

vdemeester commented Mar 29, 2023 •

edited

Loading

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

JeromeJu left a comment •

edited

Loading

lbernick Mar 29, 2023

vdemeester Mar 29, 2023

pritidesai Mar 29, 2023

vdemeester Mar 30, 2023

tekton-robot commented Mar 29, 2023

pritidesai commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

Make sure we pass the live config to the conversion webhook #6450

Make sure we pass the live config to the conversion webhook #6450

Conversation

vdemeester commented Mar 29, 2023

Changes

Submitter Checklist

Release Notes

vdemeester commented Mar 29, 2023 • edited Loading

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023

JeromeJu left a comment • edited Loading

Choose a reason for hiding this comment

lbernick Mar 29, 2023

Choose a reason for hiding this comment

vdemeester Mar 29, 2023

Choose a reason for hiding this comment

pritidesai Mar 29, 2023

Choose a reason for hiding this comment

vdemeester Mar 30, 2023

Choose a reason for hiding this comment

tekton-robot commented Mar 29, 2023

pritidesai commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

vdemeester commented Mar 29, 2023 •

edited

Loading

JeromeJu left a comment •

edited

Loading