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

Jump to bottom

Add docs for KEP 2200 (DenyServiceExternalIPs) #26297

Merged
merged 1 commit into from
Feb 9, 2021
Merged

Add docs for KEP 2200 (DenyServiceExternalIPs) #26297

merged 1 commit into from
Feb 9, 2021

Conversation

thockin
Copy link
Member

@thockin thockin commented Jan 29, 2021

  • Document DenyServiceExternalIPs admission controller
  • Re-order other admission controller blocks to be alphabetical
  • Document DefaultIngressClass (missing)

KEP: kubernetes/enhancements#2200
Code: kubernetes/kubernetes#97395

@k8s-ci-robot k8s-ci-robot added this to the 1.21 milestone Jan 29, 2021
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Jan 29, 2021 •
edited
Loading

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit c111b4a

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6021d5b63510b400084cad5d

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 29, 2021
@thockin
Copy link
Member Author

thockin commented Jan 29, 2021

Ugh, master not synced, will rebase.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/id Issues or PRs related to Indonesian language language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language labels Jan 29, 2021
@k8s-ci-robot k8s-ci-robot added language/ko Issues or PRs related to Korean language language/zh Issues or PRs related to Chinese language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jan 29, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2021
@thockin thockin changed the title Docs kep2200 Docs for KEP 2200 (DenyServiceExternalIPs0 Jan 29, 2021
@thockin
Copy link
Member Author

thockin commented Jan 29, 2021

/assign @bowei @rramkumar1 for the ingress block
/assign @tallclair for the externalIPs block

@thockin thockin mentioned this pull request Jan 29, 2021
Closed
4 tasks
@bells17
Copy link
Contributor

bells17 commented Jan 30, 2021

/remove-language es fr id it ja ko zh

@k8s-ci-robot k8s-ci-robot removed language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/id Issues or PRs related to Indonesian language language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language language/zh Issues or PRs related to Chinese language labels Jan 30, 2021
@bells17
Copy link
Contributor

bells17 commented Jan 31, 2021

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from bells17 January 31, 2021 09:15
@thockin
Copy link
Member Author

thockin commented Feb 1, 2021

nits fixed

@sftim
Copy link
Contributor

sftim commented Feb 2, 2021

/retitle Add docs for KEP 2200 (DenyServiceExternalIPs)

@k8s-ci-robot k8s-ci-robot changed the title Docs for KEP 2200 (DenyServiceExternalIPs0 Add docs for KEP 2200 (DenyServiceExternalIPs) Feb 2, 2021
@sftim
Copy link
Contributor

sftim commented Feb 2, 2021

This looks ready for tech review (which, I'm hoping, is straightforward).

@thockin
Copy link
Member Author

thockin commented Feb 3, 2021

This will be a short review, I hope :)

@bowei for Ingress paragraph
@tallclair for externalIPs paragraph

/assign bowei
/assign tallclair

@reylejano
Copy link
Member

/assign @PI-Victor

tallclair
tallclair reviewed Feb 9, 2021
View reviewed changes
content/en/docs/reference/access-authn-authz/admission-controllers.md Outdated
@@ -94,7 +94,7 @@ kube-apiserver -h | grep enable-admission-plugins
In the current version, the default ones are:

```shell
NamespaceLifecycle, LimitRanger, ServiceAccount, TaintNodesByCondition, Priority, DefaultTolerationSeconds, DefaultStorageClass, StorageObjectInUseProtection, PersistentVolumeClaimResize, RuntimeClass, CertificateApproval, CertificateSigning, CertificateSubjectRestriction, DefaultIngressClass, MutatingAdmissionWebhook, ValidatingAdmissionWebhook, ResourceQuota
AlwaysAdmit, AlwaysDeny, AlwaysPullImages, CertificateApproval, CertificateSigning, CertificateSubjectRestriction, DefaultIngressClass, DefaultStorageClass, DefaultTolerationSeconds, DenyEscalatingExec, DenyExecOnPrivileged, DenyServiceExternalIPs, EventRateLimit, ExtendedResourceToleration, ImagePolicyWebhook, LimitPodHardAntiAffinityTopology, LimitRanger, MutatingAdmissionWebhook, NamespaceAutoProvision, NamespaceExists, NamespaceLifecycle, NodeRestriction, OwnerReferencesPermissionEnforcement, PersistentVolumeClaimResize, PersistentVolumeLabel, PodNodeSelector, PodSecurityPolicy, PodTolerationRestriction, Priority, ResourceQuota, RuntimeClass, SecurityContextDeny, ServiceAccount, StorageObjectInUseProtection, TaintNodesByCondition, ValidatingAdmissionWebhook
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right to me. Isn't this just all of them? I think the default list is defined here: https://github.com/kubernetes/kubernetes/blob/1119a505aca14467accedf850daf30aa9c532ef2/pkg/kubeapiserver/options/plugins.go#L143-L161

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I cut-and-pasted the wrong list. That said, is it really worthwhile to list "in the current version" in docs - that doesn't help much. What version? Maybe we should just nix this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update the list, but still think we should remove it totally.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think just giving the command should be sufficient, or maybe even linking to the code. I could go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm remembering this correctly, the order of this list matters. Is that still the case? I just noticed that the list is now in alphabetic order.

Copy link
Member

Choose a reason for hiding this comment

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

When using --enable-admission-plugins/--disable-admission-plugins, the order is unimportant. That just toggles them on/off, but the order is predetermined.

content/en/docs/reference/access-authn-authz/admission-controllers.md Outdated

The DenyEscalatingExec admission plugin is deprecated and will be removed in v1.18.
This functionality has been merged into [DenyEscalatingExec](#denyescalatingexec).
The DenyExecOnPrivileged admission plugin is deprecated and will be removed in v1.18.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks Jordan. Let's update this to say v1.21 (same above)

@thockin
Docs for KEP 2200
c111b4a 
* Document DenyServiceExternalIPs admission controller
* Re-order other admission controller blocks to be alphabetical
* Document DefaultIngressClass (missing)
@tallclair
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: a6ff8b5ed255331976f8f2f343b2322b49647e37

@irvifa irvifa requested review from sftim and kbhawkey February 9, 2021 13:25
@irvifa
Copy link
Member

irvifa commented Feb 9, 2021

/assign

sftim
sftim reviewed Feb 9, 2021
View reviewed changes
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.

/approve

content/en/docs/reference/access-authn-authz/admission-controllers.md
"ingressclass.kubernetes.io/is-default-class"). This admission controller ignores any `Ingress`
updates; it acts only on creation.

See the [ingress](/docs/concepts/services-networking/ingress/) documentation for more about ingress
Copy link
Contributor

Choose a reason for hiding this comment

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

mini nit:

Suggested change
See the [ingress](/docs/concepts/services-networking/ingress/) documentation for more about ingress
See the [Ingress](/docs/concepts/services-networking/ingress/) documentation for more about ingress

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit d7d113a into kubernetes:dev-1.21 Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@sftim sftim sftim left review comments

@tengqm tengqm tengqm left review comments

@liggitt liggitt liggitt left review comments

@bart0sh bart0sh Awaiting requested review from bart0sh

@kbhawkey kbhawkey Awaiting requested review from kbhawkey

Assignees

@bowei bowei

@PI-Victor PI-Victor

@irvifa irvifa

@tallclair tallclair

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/docs Categorizes an issue or PR as relevant to SIG Docs. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.21
Development

Successfully merging this pull request may close these issues.