[PodSecurity]Add context to failure message #105314

calvin0327 · 2021-09-28T08:21:40Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Provide more context for the failure message.

Which issue(s) this PR fixes:

For #103561

Special notes for your reviewer:

I worked on it for a few days, I don't know that is reasonable, correct me if not.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-09-28T08:21:47Z

@calvin0327: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2021-09-28T08:21:48Z

Hi @calvin0327. Thanks for your PR.

I'm waiting for a kubernetes 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.

pacoxu · 2021-09-28T09:47:47Z

/ok-to-test

calvin0327 · 2021-09-28T12:48:32Z

/retest

tallclair

Thanks for picking this up!

tallclair · 2021-09-28T17:35:03Z

staging/src/k8s.io/pod-security-admission/admission/admission.go

+				"Pod violates %q version of %q PodSecurity profile: %s",
+				nsPolicy.Warn.Version.String(),
+				nsPolicy.Warn.Level,


There's a LevelVersion string method that renders this as <level>:<version>, for a more condensed version:

Suggested change

"Pod violates %q version of %q PodSecurity profile: %s",

nsPolicy.Warn.Version.String(),

nsPolicy.Warn.Level,

"Pod violates PodSecurity %s: %s",

nsPolicy.Enforce.String(),

I would apply the same change to the audit & warn messages too.

Should be Enforce, not Warn

tallclair · 2021-09-28T17:37:57Z

staging/src/k8s.io/pod-security-admission/admission/admission.go

 		}
 	}

 	// TODO: reuse previous evaluation if audit level+version is the same as enforce level+version
 	if result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Audit, podMetadata, podSpec)); !result.Allowed {
-		auditAnnotations["audit"] = result.ForbiddenDetail()
+		auditAnnotations["audit"] = fmt.Sprintf(
+			"Pod violates %q version of %q PodSecurity profile: %s",


The resource might not be a Pod here. I would pass the Attributes in as an argument to EvaluatePod, and then grab the Kind from that (several other open PRs are already passing in attributes: #104365)

tallclair · 2021-09-28T17:38:08Z

staging/src/k8s.io/pod-security-admission/admission/admission.go

+		auditAnnotations["audit"] = fmt.Sprintf(
+			"Pod violates %q version of %q PodSecurity profile: %s",
+			nsPolicy.Warn.Version.String(),
+			nsPolicy.Warn.Level,


Should be audit here.

Thanks for review.

calvin0327 · 2021-10-09T01:23:25Z

/retest

tallclair

last round of comments 🙂

tallclair · 2021-10-14T16:30:54Z

staging/src/k8s.io/pod-security-admission/admission/admission.go

 		}
 	}

 	// TODO: reuse previous evaluation if audit level+version is the same as enforce level+version
 	if result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Audit, podMetadata, podSpec)); !result.Allowed {
-		auditAnnotations["audit"] = result.ForbiddenDetail()
+		auditAnnotations["audit"] = fmt.Sprintf(
+			"%s violates PodSecurity %q: %s",


nit: match the warning mesasge with would violate

Suggested change

"%s violates PodSecurity %q: %s",

"%s would violate PodSecurity %q: %s",

staging/src/k8s.io/pod-security-admission/admission/admission.go

calvin0327 · 2021-10-18T03:03:34Z

/retest

tallclair · 2021-10-19T18:46:11Z

staging/src/k8s.io/pod-security-admission/admission/admission_test.go

@@ -563,16 +563,16 @@ func TestValidatePodController(t *testing.T) {
 			desc:                   "bad deploy creates produce correct user-visible warnings and correct auditAnnotations",
 			newObject:              &badDeploy,
 			gvr:                    schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"},
-			expectAuditAnnotations: map[string]string{"audit": "forbidden sysctls (unknown)"},
-			expectWarnings:         []string{"would violate \"latest\" version of \"baseline\" PodSecurity profile: forbidden sysctls (unknown)"},
+			expectAuditAnnotations: map[string]string{"audit": "deployments would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"},


Hmm, this doesn't read quite right. It should be "Deployment would violate ...". I think you need to change attrs.GetResource().Resource to attrs.GetObject().GetObjectKind().GroupVersionKind().Kind

@liggitt do you know if we can depend on Kind always being populated for the admission object? Otherwise we'll need to expose a new attributes method to pull it off the admission request...

I... don't think kind is populated when converting an internal object to a versioned object

The other option is we just drop the kind from the message... just "Would violate ...

@tallclair Ok, thanks for review. And now, we would't need the kind from the Attributes argument.

I will dorp it from the message.

calvin0327 · 2021-10-20T07:29:34Z

/retest

calvin0327 · 2021-10-20T08:25:58Z

/retest

calvin0327 · 2021-10-21T10:18:01Z

/retest

tallclair

/lgtm
/approve

k8s-ci-robot · 2021-10-25T17:23:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calvin0327, tallclair

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:

~~staging/src/k8s.io/pod-security-admission/OWNERS~~ [tallclair]

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

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 28, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 28, 2021

k8s-ci-robot requested review from deads2k and liggitt September 28, 2021 08:22

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 28, 2021

calvin0327 changed the title ~~add context to failure message.~~ add context to failure message and group error message by container Sep 28, 2021

k8s-ci-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 Sep 28, 2021

calvin0327 changed the title ~~add context to failure message and group error message by container~~ [PodSecurity]Add context to failure message and group error message by container Sep 28, 2021

calvin0327 changed the title ~~[PodSecurity]Add context to failure message and group error message by container~~ [PodSecurity]Add context to failure message Sep 28, 2021

calvin0327 force-pushed the issue-podsecurity-errormessage branch from aa06ccb to 9c0e0c1 Compare September 28, 2021 10:18

tallclair self-assigned this Sep 28, 2021

tallclair reviewed Sep 28, 2021

View reviewed changes

calvin0327 mentioned this pull request Sep 29, 2021

REQUEST: New membership for calvin0327 kubernetes/org#3019

Closed

7 tasks

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 7, 2021

calvin0327 force-pushed the issue-podsecurity-errormessage branch from e8b9f9b to 784f6ef Compare October 8, 2021 02:46

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 8, 2021

calvin0327 force-pushed the issue-podsecurity-errormessage branch from 784f6ef to bbb372a Compare October 8, 2021 09:44

tallclair reviewed Oct 14, 2021

View reviewed changes

calvin0327 force-pushed the issue-podsecurity-errormessage branch 3 times, most recently from 68ecd7d to c62521c Compare October 18, 2021 02:30

tallclair reviewed Oct 19, 2021

View reviewed changes

calvin0327 force-pushed the issue-podsecurity-errormessage branch from c62521c to fa56d15 Compare October 20, 2021 02:56

calvin0327 force-pushed the issue-podsecurity-errormessage branch from fa56d15 to a8b6ae1 Compare October 21, 2021 02:06

add context to failure message.

12bfe73

calvin0327 force-pushed the issue-podsecurity-errormessage branch from 7a55d28 to 12bfe73 Compare October 21, 2021 06:41

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 21, 2021

tallclair reviewed Oct 25, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2021

k8s-ci-robot merged commit a6ffd29 into kubernetes:master Oct 25, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 25, 2021

This was referenced Nov 2, 2021

[PodSecurity] Improve error UX #103561

Closed

PodSecurity admission (PodSecurityPolicy replacement) kubernetes/enhancements#2579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PodSecurity]Add context to failure message #105314

[PodSecurity]Add context to failure message #105314

calvin0327 commented Sep 28, 2021 •

edited by tallclair

Loading

k8s-ci-robot commented Sep 28, 2021

k8s-ci-robot commented Sep 28, 2021

pacoxu commented Sep 28, 2021

calvin0327 commented Sep 28, 2021

tallclair left a comment

tallclair Sep 28, 2021

tallclair Sep 28, 2021

tallclair Sep 28, 2021

tallclair Sep 28, 2021

tallclair Sep 28, 2021

calvin0327 Sep 29, 2021 •

edited

Loading

calvin0327 commented Oct 9, 2021

tallclair left a comment

tallclair Oct 14, 2021

calvin0327 commented Oct 18, 2021

tallclair Oct 19, 2021

liggitt Oct 19, 2021

tallclair Oct 19, 2021

calvin0327 Oct 20, 2021

calvin0327 Oct 20, 2021

calvin0327 commented Oct 20, 2021

calvin0327 commented Oct 20, 2021

calvin0327 commented Oct 21, 2021

tallclair left a comment

k8s-ci-robot commented Oct 25, 2021

	"%s violates PodSecurity %q: %s",
	"%s would violate PodSecurity %q: %s",

[PodSecurity]Add context to failure message #105314

[PodSecurity]Add context to failure message #105314

Conversation

calvin0327 commented Sep 28, 2021 • edited by tallclair Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 28, 2021

k8s-ci-robot commented Sep 28, 2021

pacoxu commented Sep 28, 2021

calvin0327 commented Sep 28, 2021

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvin0327 Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

calvin0327 commented Oct 9, 2021

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvin0327 commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvin0327 commented Oct 20, 2021

calvin0327 commented Oct 20, 2021

calvin0327 commented Oct 21, 2021

tallclair left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 25, 2021

calvin0327 commented Sep 28, 2021 •

edited by tallclair

Loading

calvin0327 Sep 29, 2021 •

edited

Loading