Skip to content

Conversation

@tkashem
Copy link
Contributor

@tkashem tkashem commented Oct 21, 2024

KEP: https://kep.k8s.io/3926
PR: kubernetes/kubernetes#127513

/sig api-machinery
/sig auth

@k8s-ci-robot k8s-ci-robot added this to the 1.32 milestone Oct 21, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 21, 2024
@netlify
Copy link

netlify bot commented Oct 21, 2024

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit f7cce0d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6745e8b24a6dbc00097c23c5

@netlify
Copy link

netlify bot commented Oct 21, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit f7cce0d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6745e8b2f736e200083be4b4
😎 Deploy Preview https://deploy-preview-48463--kubernetes-io-main-staging.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 site configuration.

@spurin
Copy link
Contributor

spurin commented Nov 1, 2024

Hello @tkashem 👋 please take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before Tuesday November 19th 2024 18:00 PST. Thank you!

@sftim
Copy link
Contributor

sftim commented Nov 11, 2024

@spurin
Copy link
Contributor

spurin commented Nov 11, 2024

Hello @tkashem 👋, 1.32 Docs Shadow here,

A quick reminder to get your PR ready for review before the deadline, Tuesday November 19th at 18:00 PDT. For additional information, refer to: Documenting for a release - PR Ready for Review.

Thank you!

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Nov 18, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 18, 2024
@tkashem tkashem marked this pull request as ready for review November 18, 2024 15:48
@tkashem
Copy link
Contributor Author

tkashem commented Nov 18, 2024

I have a couple of questions:

a) do i need to generate the updated api-reference?

b) from https://kubernetes.io/docs/contribute/new-content/new-features/#ready-for-review-feature-gates

With net new feature gates, a separate description of the feature gate is also required; create a new Markdown file inside content/en/docs/reference/command-line-tools-reference/feature-gates/ (use other files as a template).

I guess it is referring to the same markdown file at the beginning of the section, for a net new feature, i just need to create one markdown file, correct?

@tkashem tkashem changed the title WIP: Placeholder for 1.32 KEP-3926 docs. add doc for new delete option ignoreStoreReadErrorWithClusterBreakingPotential Nov 18, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024
@tkashem
Copy link
Contributor Author

tkashem commented Nov 18, 2024

/cc @stlaz @enj @dgrisonnet @deads2k

@stlaz
Copy link
Member

stlaz commented Nov 18, 2024

/assign
per sig-auth triage

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2024
@tkashem tkashem force-pushed the kep-3926 branch 3 times, most recently from 393eabf to 5deea87 Compare November 25, 2024 19:15

Once the last finalizer is removed, the resource is actually removed from etcd.

### Force deletion
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit)

Suggested change
### Force deletion
### Forcible resource deletion {#forced-deletion}

Copy link
Contributor

Choose a reason for hiding this comment

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

should be {#force-deletion}, no d, to preserve existing links.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tengqm this is new content (no existing links). Here I'm just recommending a way to make the eventual URL shorter.

Comment on lines 866 to 867
The user performing the force **delete** operation must have the
`unsafe-delete-ignore-read-errors` permission on the resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

(style nit)

Suggested change
The user performing the force **delete** operation must have the
`unsafe-delete-ignore-read-errors` permission on the resource.
The user performing the force **delete** operation must be permitted to perform
both the **delete** and **unsafe-delete-ignore-read-errors** verbs on that resource.

We write API verbs in non-monospaced bold. I assume that unsafe-delete-ignore-read-errors is a verb and that a SubjectAccessReview happens. Have I got that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unsafe-delete-ignore-read-errors is a right/verb. I was going through the existing doc (for example https://kubernetes.io/docs/concepts/security/rbac-good-practices/) and how they format RBAC rights. I used the same format ( verb ) in this doc. It seems the format delete is used to represent an API operation, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're defining access through RBAC, you use monospace. But: outside of the RBAC docs, we avoid assuming that clusters use RBAC for authz. It's a common choice but not the only option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm rewording here to avoid an RBAC-centric framing.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

This is already alpha quality.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 048f93145320f6d10ab034abf0ef3d2af6882320

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2024
@k8s-ci-robot k8s-ci-robot requested a review from sftim November 26, 2024 14:26
@tkashem
Copy link
Contributor Author

tkashem commented Nov 26, 2024

@sftim i dropped the and - #48463 (comment), ptal

Comment on lines 866 to 867
In addition to `delete` rights, the user performing the force **delete**
operation must have `unsafe-delete-ignore-read-errors` rights on the given resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

(style nit)

Suggested change
In addition to `delete` rights, the user performing the force **delete**
operation must have `unsafe-delete-ignore-read-errors` rights on the given resource.
The user performing the force **delete** operation must be permitted to perform
both the **delete** and **unsafe-delete-ignore-read-errors** verbs on that resource.

We write API verbs in non-monospaced bold. I assume that unsafe-delete-ignore-read-errors is a verb and that a SubjectAccessReview happens. Have I got that right?

Also, avoid implying that clusters always use Kubernetes RBAC. Many do but it's not mandatory to.

@sftim
Copy link
Contributor

sftim commented Nov 26, 2024

I'm happy with the changes since #48463 (review)

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: bdd3173a87feaa1b76dcaa75278a71fd685898d5

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2024
Copy link
Member

@dgrisonnet dgrisonnet left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, sftim

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

@sftim
Copy link
Contributor

sftim commented Nov 26, 2024

Also, please fix the changelog for the k/k PR; see kubernetes/kubernetes#127513 (comment)

@k8s-ci-robot k8s-ci-robot merged commit f8f5eda into kubernetes:dev-1.32 Nov 26, 2024
6 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants