CustomResource panic when changing name field in Tekton Pipeline #1970
Assignees
Labels
impact/panic
This bug represents a panic or unexpected crash
kind/bug
Some behavior is incorrect or out of spec
resolution/fixed
This issue was fixed
Comments
jsravn
changed the title
|
Thanks for filing this issue! Well take a look as soon as we can. |
|
Seems to happen when I change other fields as well, it's not clear what exactly triggers this bug. For example, I made this change to a Pipeline today: In other words, I simply added a param item. But now it always fails with a panic when trying to do a |
|
Okay, I think I figured out the cause. After applying the Pipeline, the resource in Kubernetes ends up with a taskSpec:
metadata: {}
params:
- name: builder-image
type: string
spec: null
steps:
- image: $(params.builder-image)
<snip>Unsure where this is coming from - it's an optional field and should be omitted if empty (https://github.com/tektoncd/pipeline/blob/7de70f1c095a12c10e81932ab3b0e2268dd80ec5/pkg/apis/pipeline/v1beta1/pipeline_types.go#L149). Could be a Tekton bug that is causing this to get set to null somewhere rather than being omitted. Anyways, it seems the kubernetes differ can't handle this properly. It encounters "spec: null" and ends up passing a nil object to the diff logic for both old and new value, and the assertion blows up. Seems to also only happen when modifying a A workaround is to specify an empty |
|
I'm facing same issue with Error: Config: |
|
I have a |
blampe
added a commit
that referenced
this issue
May 9, 2024
Users can run into a panic because we invoke `addPatchValueToDiff` like so: ```go pc.addPatchValueToDiff(append(path, k), v, old[k], newInput[k], oldInput[k], inArray) ``` These are `map[string]any` types and will return `nil` when the `[k]` lookup returns `nil` _or_ when the key `k` is missing. Then in `addPatchValueToDiff` we assert: ```go contract.Assertf(v != nil || old != nil || oldInput != nil || newInput != nil) ``` but we don't have enough information to distinguish the "all values were `nil`" case from the "one value was `nil` and the other was missing" case. The assertion is only valid if all keys were present, hence we panic when a key is removed when it previously had a value of `nil`. The easiest fix is to relax the assertion (b886cfb) -- allow the all-`nil` case since it can be valid. The downside is a poorer user experience, because we don't have enough information to say whether the update is an add or a delete; we could call it an update (odd) or a no-op (lossy). The upside is it's a much smaller change with less risk. Alternatively, we can have a better user experience by making `addPatchValueToDiff` aware of whether keys were missing. This PR does this by introducing a `missing` sentinel to distinguish from `nil` values. This is riskier, but fortunately our test coverage is comprehensive. A failing unit test was added, along with another test to cover a small bit of logic we weren't testing. Fixes #1970.
lumiere-bot bot
added a commit
to coolguy1771/home-ops
that referenced
this issue
May 24, 2024
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/kubernetes](https://pulumi.com) ([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies | minor | [`4.11.0` -> `4.12.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.11.0/4.12.0) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pulumi/pulumi-kubernetes (@​pulumi/kubernetes)</summary> ### [`v4.12.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#4120-May-21-2024) [Compare Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.11.0...v4.12.0) ##### Added - Added a new Helm Chart v4 resource. ([pulumi/pulumi-kubernetes#2947) - Added support for deletion propagation policies (e.g. Orphan). ([pulumi/pulumi-kubernetes#3011) - Server-side apply conflict errors now include the original field manager's name. ([pulumi/pulumi-kubernetes#2983) ##### Changed - Pulumi will now wait for DaemonSets to become ready. ([pulumi/pulumi-kubernetes#2953) - The Release resource's merge behavior for `valueYamlFiles` now more closely matches Helm's behavior. ([pulumi/pulumi-kubernetes#2963) ##### Fixed - Helm Chart V3 previews no longer fail when the cluster is unreachable. ([pulumi/pulumi-kubernetes#2992) - Fixed a panic that could occur when a missing field became `null`. ([pulumi/pulumi-kubernetes#1970) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzEuMSIsInVwZGF0ZWRJblZlciI6IjM3LjM3MS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJ0eXBlL21pbm9yIl19--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What happened?
I'm trying to modify a Tekton Pipeline via
kubernetes.apiextensions.CustomResource. If I try changing a step name, I get a panic.Steps to reproduce
pulumi upto applypulumi upto applyExpected Behavior
It should successfully change the step name.
Actual Behavior
It panics.
Versions used
Additional context
Using plain
kubectl editI can change the step name without issue.Contributing
Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).
The text was updated successfully, but these errors were encountered: