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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Mar 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/pod/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ const (
// is that the creation of the pod backing the TaskRun failed
ReasonPodCreationFailed = "PodCreationFailed"

// ReasonPodAdmissionFailed indicates that the TaskRun's pod failed to pass admission validation
ReasonPodAdmissionFailed = "PodAdmissionFailed"

// ReasonPending indicates that the pod is in corev1.Pending, and the reason is not
// ReasonExceededNodeResources or isPodHitConfigError
ReasonPending = "Pending"
Expand Down
9 changes: 8 additions & 1 deletion pkg/reconciler/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,8 @@ func (c *Reconciler) handlePodCreationError(tr *v1beta1.TaskRun, err error) erro
tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
case k8serrors.IsAlreadyExists(err):
tr.Status.MarkResourceOngoing(podconvert.ReasonPending, "tried to create pod, but it already exists")
case isPodAdmissionFailed(err):
tr.Status.MarkResourceFailed(podconvert.ReasonPodAdmissionFailed, err)
default:
// The pod creation failed with unknown reason. The most likely
// reason is that something is wrong with the spec of the Task, that we could
Expand All @@ -637,7 +639,7 @@ func (c *Reconciler) handlePodCreationError(tr *v1beta1.TaskRun, err error) erro
msg += "invalid TaskSpec"
}
err = controller.NewPermanentError(errors.New(msg))
tr.Status.MarkResourceFailed(podconvert.ReasonCouldntGetTask, err)
tr.Status.MarkResourceFailed(podconvert.ReasonPodCreationFailed, err)
}
return err
}
Expand Down Expand Up @@ -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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

}

// updateStoppedSidecarStatus updates SidecarStatus for sidecars that were
// terminated by nop image
func updateStoppedSidecarStatus(pod *corev1.Pod, tr *v1beta1.TaskRun) error {
Expand Down
21 changes: 19 additions & 2 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2824,8 +2824,25 @@ status:
err: errors.New("this is a fatal error"),
expectedType: apis.ConditionSucceeded,
expectedStatus: corev1.ConditionFalse,
expectedReason: podconvert.ReasonCouldntGetTask,
}}
expectedReason: podconvert.ReasonPodCreationFailed,
}, {
description: "errors violating PodSecurity fail the taskrun",
err: k8sapierrors.NewForbidden(k8sruntimeschema.GroupResource{Group: "foo", Resource: "bar"}, "baz",
errors.New("violates PodSecurity \"restricted:latest\": allowPrivilegeEscalation != false ("+
"containers \"prepare\", \"place-scripts\", \"test-task\", \"test-task\" must set securityContext."+
"allowPrivilegeEscalation=false)")),
expectedType: apis.ConditionSucceeded,
expectedStatus: corev1.ConditionFalse,
expectedReason: podconvert.ReasonPodAdmissionFailed,
}, {
description: "errors validating security context constraint (Openshift) fail the taskrun",
err: k8sapierrors.NewForbidden(k8sruntimeschema.GroupResource{Group: "foo", Resource: "bar"}, "baz",
errors.New("unable to validate against any security context constraint: [provider restricted: .spec.securityContext.hostNetwork: Invalid value: true: Host network is not allowed to be used provider restricted: .spec.securityContext.hostPID: Invalid value: true: Host PID is not allowed to be used")),
expectedType: apis.ConditionSucceeded,
expectedStatus: corev1.ConditionFalse,
expectedReason: podconvert.ReasonPodAdmissionFailed,
},
}
for _, tc := range testcases {
t.Run(tc.description, func(t *testing.T) {
c.handlePodCreationError(taskRun, tc.err)
Expand Down