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

Injected sidecars still can show up in TaskRun status when the TaskRun has explicit sidecars #7640

Closed
abayer opened this issue Feb 6, 2024 · 2 comments · Fixed by #7642
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@abayer
Copy link
Contributor

abayer commented Feb 6, 2024

Expected Behavior

If a pod has a container injected into it by some non-Tekton method, such as a mutating webhook, as well as explicitly defined sidecars, that container should not show up in taskRun.Status.Sidecars.

Actual Behavior

Any container in the pod which doesn't start with step- is treated as a sidecar by the TaskRun reconciler's updateStoppedSidecarStatus function. If there are also explicitly specified sidecars in the TaskRun, the podconvert.IsSidecarStatusRunning call after StopSidecars will return true if the explicit sidecar(s) weren't already terminated, resulting in updateStoppedSidecarStatus getting called.

MakeTaskRunStatus correctly filters to only include step- and sidecar- containers, but updateStoppedSidecarStatus doesn't have that filtering.

Steps to Reproduce the Problem

  1. Inject a container into a TaskRun pod without specifying it as a step or sidecar, with the TaskRun containing at least one explicit sidecar.
  2. Wait for the pod to complete, and see that the injected sidecar container shows up in the TaskRun status, but was not present in the status until the sidecars were stopped.

Additional Info

  • Tekton Pipeline version:

Every version I've checked through the latest v0.56.0.

@abayer abayer added the kind/bug Categorizes issue or PR as related to a bug. label Feb 6, 2024
@abayer abayer self-assigned this Feb 6, 2024
@abayer
Copy link
Contributor Author

abayer commented Feb 6, 2024

Looking deeper into the code, I see that we are very deliberately stopping injected "sidecars", but I still think they shouldn't show up in the TaskRun.Status.Sidecars - #5565 (comment) seems to agree with me. =) Looks like we just missed the updateStoppedSidecarStatus angle in the original PR.

@abayer abayer changed the title Containers in TaskRun pod not specified by Tekton are treated as sidecars Injected sidecars still can show up in TaskRun status Feb 6, 2024
@abayer abayer changed the title Injected sidecars still can show up in TaskRun status Injected sidecars still can show up in TaskRun status when the TaskRun has explicit sidecars Feb 6, 2024
@abayer
Copy link
Contributor Author

abayer commented Feb 6, 2024

Also, this only happens when the TaskRun has explicit sidecars as well as injected sidecars - otherwise updateStoppedSidecarStatus never gets called. Updated the issue description accordingly.

abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 6, 2024
fixes tektoncd#7640

In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
tekton-robot pushed a commit that referenced this issue Feb 8, 2024
fixes #7640

In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
tekton-robot pushed a commit to tekton-robot/pipeline that referenced this issue Feb 12, 2024
fixes tektoncd#7640

In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
tekton-robot pushed a commit that referenced this issue Feb 12, 2024
fixes #7640

In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
l-qing pushed a commit to katanomi/pipeline that referenced this issue Feb 15, 2024
fixes tektoncd#7640

In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.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.

1 participant