Skip to content

Conversation

@nader-ziada
Copy link
Member

Proposed Changes

/cc @dprotaso

@knative-prow knative-prow bot requested a review from dprotaso October 21, 2025 16:35
@netlify
Copy link

netlify bot commented Oct 21, 2025

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 44a389a
🔍 Latest deploy log https://app.netlify.com/projects/knative/deploys/68f8e319a276e6000808535b
😎 Deploy Preview https://deploy-preview-6461--knative.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 21, 2025
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/assign

Thanks for writing this up! A couple comments, but it's good to have this documented more clearly.

- Installing Security-Guard: serving/app-security/security-guard-install.md
- Security-guard quickstart: serving/app-security/security-guard-quickstart.md
- Security-Guard example alerts: serving/app-security/security-guard-example-alerts.md
- SecurePodDefaults: serving/app-security/secure-pod-defaults.md
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to "Administration / Configuring Knative / Serving Configuration, around line 280.

Sorry for leaving this section here to confuse -- I think it may be removed altogether soon.

Copy link
Member

@dprotaso dprotaso Oct 21, 2025

Choose a reason for hiding this comment

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

I suggest to Nader this section because I think it's relevant and we can have documentation in two places.

I'm going to make a separate PR that collapses security guard into a single page. Thus the section becomes about 'Application Security' instead of just exclusive to security guard.

(edit) done: #6464

Copy link
Member

Choose a reason for hiding this comment

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

As can we move Secure Pod Defaults to the top of the section

Copy link
Member

Choose a reason for hiding this comment

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

@evankanderson I'll defer to you what content you feel belongs in one spot vs the other


## SecurePodDefaults

Knative Serving provides a `secure-pod-defaults` configuration option that gives operators granular control over pod security defaults, enabling progressive adoption of more secure pod configurations while maintaining backward compatibility with existing workloads. This feature offers three security levels—`disabled`, `root-allowed`, and `enabled` allowing organizations to gradually adopt security best practices without breaking container images that require specific permissions. The default is `disabled` to ensure existing deployments continue to work without modification.
Copy link
Member

Choose a reason for hiding this comment

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

I'd be careful about saying "granular control". I'd rather link to Pod Security Standards, and say that it allows the default Service configuration to run in the "restricted" profile without requiring application developers to explicitly set security properties. Another possible phrasing: enabled will default to the values permitted by the "restricted" profile, and root-allowed will default most values to the "restricted" profile while allowing several common application containers to run.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm wondering if a little bit of philosophy would be useful here. (Defaults should be safe, but k8s defaults aren't always / values should be safe if but specified, but don't block explicit reduction of security stance.)

We may also want to point to k8s Pod Security Admission and/or policy frameworks like OPA gatekeeper and Kyverno for "enforce secure settings" (as opposed to "default to safe"). An additional benefit of these other frameworks is that they cover all types of pods, while secure-pod-defaults only covers Knative pods that haven't thought about security settings.

Copy link
Member

@dprotaso dprotaso left a comment

Choose a reason for hiding this comment

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

Looks good - some minor stuff

---


## SecurePodDefaults
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a !!! notice/important/warning banner on this page that the current default will be changed in 1.21 and that users should explicitly disable this setting to retain the existing behaviour

Then we can link this section in the 1.20 release notes

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, good point!

Yes, I think a warning banner would be a good call given that we know we'll be changing this.

@dprotaso
Copy link
Member

/assign @evankanderson for further review

@dprotaso
Copy link
Member

Also please see this issue comment: knative/serving#16137 (comment)

I realize we might have an opportunity to simplify this a bit in 1.21

@nader-ziada
Copy link
Member Author

@evankanderson @dprotaso I think I covered all of the comments in the discussion, please take another look

note: I moved the page link in the nav.yaml, but didn't move the actual file location because I wasn't sure if github would be able to show the diff and comments, I can move it once you both think its ready.

@evankanderson
Copy link
Member

I think we'll need a big reorganization one day; in the meantime, the nav.yml magic picks up the first place the page appears in the nav bar

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2025
@knative-prow
Copy link

knative-prow bot commented Oct 22, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, nader-ziada

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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2025
@knative-prow knative-prow bot merged commit f659ed1 into knative:main Oct 22, 2025
19 checks passed
@dprotaso
Copy link
Member

thanks @nader-ziada

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants