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

update admission webhook to accept client config #54156

Merged
merged 1 commit into from Oct 19, 2017
Merged

update admission webhook to accept client config #54156

merged 1 commit into from Oct 19, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Oct 18, 2017

Fixes #53827

This plumbs a complete client through the plugin initializer for admission webhooks. It achieves parity with our existing webhooks and provides flexibility if people want to do something special or different. Easy things are easy, hard things are possible. This does not change behavior for kube-apiserver.

@kubernetes/sig-auth-api-reviews @kubernetes/sig-api-machinery-bugs

@k8s-ci-robot
Copy link
Contributor

@deads2k: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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 k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Oct 18, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2017
@deads2k deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 18, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2017
liggitt
liggitt reviewed Oct 18, 2017
View reviewed changes
cmd/kube-apiserver/app/server.go Outdated
if proxyTransport != nil && proxyTransport.Dial != nil {
webhookClientConfig.Dial = proxyTransport.Dial
// if you set this, you lost e2e safety on your connnection.
webhookClientConfig.TLSClientConfig.Insecure = proxyTransport.TLSClientConfig.InsecureSkipVerify
Copy link
Member

Choose a reason for hiding this comment

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

proxyTransport.TLSClientConfig can be nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proxyTransport.TLSClientConfig can be nil

Not in this path, but I'll gate.

liggitt
liggitt reviewed Oct 18, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go Outdated
@@ -42,10 +43,10 @@ type WantsAuthorizer interface {
admission.Validator
}

// WantsClientCert defines a fuction that accepts a cert & key for admission
// WantsWebhookRESTClient defines a function that accepts a cert & key for admission
Copy link
Member

Choose a reason for hiding this comment

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

fix godoc

@liggitt liggitt self-assigned this Oct 18, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017
caesarxuchao
caesarxuchao reviewed Oct 18, 2017
View reviewed changes
plugin/pkg/admission/webhook/admission.go
},
Dial: dial,
}
cfg := rest.CopyConfig(a.restClientConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Where do you set the ServerName and CABundle?

@deads2k deads2k force-pushed the admission-06-restclient branch 3 times, most recently from 5ce5932 to ae60ff2 Compare October 18, 2017 18:08
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017
@kubernetes kubernetes deleted a comment from liggitt Oct 18, 2017
@kubernetes kubernetes deleted a comment from liggitt Oct 18, 2017
@kubernetes kubernetes deleted a comment from liggitt Oct 18, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Oct 18, 2017

/retest

@caesarxuchao
Copy link
Member

Could you leave a grace period for @cheftako or @lavalamp to comment about using the SA token? Thank you.

@liggitt
Copy link
Member

liggitt commented Oct 18, 2017

Could you leave a grace period for @cheftako or @lavalamp to comment about using the SA token? Thank you.

As noted, this PR doesn't change kube-apiserver or federation-apiserver behavior. Plumbing a full client config is consistent with other webhooks and is the standard way to propagate info used to build a client.

Discussions about the content of that client config when used from extension API servers can be a follow up.

Will wait until tomorrow morning to tag, but I'd expect any approach to continue plumbing the right config structure through

@liggitt
Copy link
Member

liggitt commented Oct 19, 2017

Tagging as agreed. Can continue discussion as part of subsequent webhook admission work.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

Associated issue: 53827

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@deads2k deads2k removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2017
@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2017
@liggitt
Copy link
Member

liggitt commented Oct 19, 2017

/retest

1 similar comment
@liggitt
Copy link
Member

liggitt commented Oct 19, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f07b359 into kubernetes:master Oct 19, 2017
@erictune erictune added area/admission-control and removed area/admission-control approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 26, 2017
@deads2k deads2k deleted the admission-06-restclient branch July 3, 2018 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@caesarxuchao caesarxuchao caesarxuchao left review comments

Assignees

@lavalamp lavalamp

@liggitt liggitt

@caesarxuchao caesarxuchao

Labels
area/admission-control cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@deads2k @k8s-ci-robot @caesarxuchao @liggitt @k8s-github-robot @lavalamp @erictune