fix: add PodAdmissionFailed reason to avoid confusing failed status #6295

kinsolee · 2023-03-05T07:53:45Z

Closes #5693
Signed-off-by: Jingzhao Li jzli@alauda.io

Changes

Currently CouldntGetTask is used as default case of describing reason of failed status which confuses users when unknown error occurs but the reason has nothing to do with getting task.
This PR fix this bug by replacing CouldntGetTask with PodCreationFailed as default error reason.
Reason PodAdmissionFailed is added to describing errors cause by validating pod addmission, including K8S PodSecurity and Openshift SCC.

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

NONE

linux-foundation-easycla · 2023-03-05T07:53:48Z

The committers listed above are authorized under a signed CLA.

✅ login: kinsolee / name: Kinso (0d90cf5)

tekton-robot · 2023-03-05T07:53:54Z

Hi @kinsolee. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kinsolee · 2023-03-05T08:16:02Z

/kind bug

JeromeJu · 2023-03-06T16:03:44Z

/ok-to-test

JeromeJu · 2023-03-06T16:07:09Z

Thanks for the PR, wondering if the messages would necessarily need to be the release notes since it is not indicating to be user-facing.

tekton-robot · 2023-03-06T16:09:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-03-08T13:55:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

tekton-robot · 2023-03-08T14:08:48Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.9%	85.0%	0.1

tekton-robot · 2023-03-08T14:09:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.9%	85.0%	0.1

kinsolee · 2023-03-08T14:18:13Z

/release-note-none

tekton-robot · 2023-03-08T14:18:21Z

@kinsolee: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none".

In response to this:

/release-note-none

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.

1. add PodAdmissionFailed to indicates status failed from failing validating admission 2. use PodCreationFailed to indicates unknown reason of failed status Closes tektoncd#5693 Signed-off-by: Jingzhao Li <jzli@alauda.io>

tekton-robot · 2023-03-17T06:08:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.9%	85.0%	0.1

tekton-robot · 2023-03-17T06:10:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.9%	85.0%	0.1

kinsolee · 2023-03-17T07:52:31Z

@bobcatfish @dibyom Can anyone give a lgtm so that I can merge this PR?

chitrangpatel · 2023-03-20T12:58:25Z

pkg/reconciler/taskrun/taskrun.go

@@ -813,6 +815,11 @@ func isTaskRunValidationFailed(err error) bool {
 	return err != nil && strings.Contains(err.Error(), "TaskRun validation failed")
 }

+func isPodAdmissionFailed(err error) bool {
+	return err != nil && k8serrors.IsForbidden(err) && (strings.Contains(err.Error(), "violates PodSecurity") ||
+		strings.Contains(err.Error(), "security context constraint"))


Using strings.Contains feels a little brittle since if the string content changes then it could break this logic. Is there a way to compare it better using errors.Is?

Actually, inside k8serrors.IsForbidden(err), errors.As has been used to convert err to StatusError.

In this case, StatusError.StatusReason is Forbidden, StatusError.Code is 403 and Status.Message shows the detail of error. The StatusError is from webhook as REST API server response which is the source of error.

Using strings.Contains seems to be a common way to identify error. As you can see, other funcs in this file such as isExceededResourceQuotaError, isTaskRunValidationFailed are also using strings.Contains.

LBNL, we can't find another field from StatusError to identify such similar error cases.

chitrangpatel · 2023-03-21T14:21:32Z

/lgtm

This commit removes the unused pod.ReasonCouldntGetTask. ReasonCouldntGetTask has been moved from reconciler to pod/status pkg via tektoncd#1627. The reference to it has been removed since tektoncd#6295. /kind cleanup

This commit removes the unused pod.ReasonCouldntGetTask. ReasonCouldntGetTask has been moved from reconciler to pod/status pkg via #1627. The reference to it has been removed since #6295. /kind cleanup

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Mar 5, 2023

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 5, 2023

tekton-robot requested a review from bobcatfish March 5, 2023 07:53

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 5, 2023

tekton-robot requested a review from dibyom March 5, 2023 07:53

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. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Mar 5, 2023

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2023

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

vdemeester approved these changes Mar 8, 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 8, 2023

kinsolee force-pushed the fix-confused-status-reason branch from 0d90cf5 to 1eec7c3 Compare March 8, 2023 14:02

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 8, 2023

kinsolee force-pushed the fix-confused-status-reason branch from 1eec7c3 to 8175948 Compare March 17, 2023 06:02

chitrangpatel reviewed Mar 20, 2023

View reviewed changes

tekton-robot assigned chitrangpatel Mar 21, 2023

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

tekton-robot merged commit 18d149a into tektoncd:main Mar 21, 2023

kinsolee deleted the fix-confused-status-reason branch March 21, 2023 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add PodAdmissionFailed reason to avoid confusing failed status #6295

fix: add PodAdmissionFailed reason to avoid confusing failed status #6295

kinsolee commented Mar 5, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 5, 2023 •

edited

Loading

tekton-robot commented Mar 5, 2023

kinsolee commented Mar 5, 2023

JeromeJu commented Mar 6, 2023

JeromeJu commented Mar 6, 2023

tekton-robot commented Mar 6, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

kinsolee commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 17, 2023

tekton-robot commented Mar 17, 2023

kinsolee commented Mar 17, 2023

chitrangpatel Mar 20, 2023

kinsolee Mar 20, 2023

chitrangpatel Mar 21, 2023

chitrangpatel commented Mar 21, 2023

fix: add PodAdmissionFailed reason to avoid confusing failed status #6295

fix: add PodAdmissionFailed reason to avoid confusing failed status #6295

Conversation

kinsolee commented Mar 5, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

linux-foundation-easycla bot commented Mar 5, 2023 • edited Loading

tekton-robot commented Mar 5, 2023

kinsolee commented Mar 5, 2023

JeromeJu commented Mar 6, 2023

JeromeJu commented Mar 6, 2023

tekton-robot commented Mar 6, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

kinsolee commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 17, 2023

tekton-robot commented Mar 17, 2023

kinsolee commented Mar 17, 2023

chitrangpatel Mar 20, 2023

Choose a reason for hiding this comment

kinsolee Mar 20, 2023

Choose a reason for hiding this comment

chitrangpatel Mar 21, 2023

Choose a reason for hiding this comment

chitrangpatel commented Mar 21, 2023

kinsolee commented Mar 5, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 5, 2023 •

edited

Loading