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

dynamic reload cluster authentication info for aggregated API servers #85004

Merged
merged 4 commits into from
Nov 13, 2019
Merged

dynamic reload cluster authentication info for aggregated API servers #85004

merged 4 commits into from
Nov 13, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 8, 2019 •
edited
Loading

Fixes #82141

Final step to automatically reload cluster authentication.

/kind bug
@kubernetes/sig-api-machinery-bugs
/priority important-soon

apiservers based on k8s.io/apiserver with delegated authn based on cluster authentication will automatically update to new authentication information when the authoritative configmap is updated.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 12, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 12, 2019
@deads2k deads2k changed the title [wip] dynamic reload cluster authentication info for aggregated API servers dynamic reload cluster authentication info for aggregated API servers Nov 12, 2019
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 12, 2019
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2019
@deads2k deads2k added this to the v1.17 milestone Nov 12, 2019
@sttts
Copy link
Contributor

sttts commented Nov 13, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Nov 13, 2019
tedyu
tedyu reviewed Nov 13, 2019
View reviewed changes
cmd/kube-apiserver/app/testing/testserver.go
return result, err
}
proxyCACertFile := path.Join(s.SecureServing.ServerCert.CertDirectory, "proxy-ca.crt")
if err := ioutil.WriteFile(proxyCACertFile, testutil.EncodeCertPEM(proxySigningCert), 0644); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the proxyCACertFile be deleted if there is error in subsequent handling in this func ?

tedyu
tedyu reviewed Nov 13, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go

// check to see if we have a change. If the values are the same, do nothing.
existing, ok := uncastExisting.(*caBundleAndVerifier)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should c.caBundle be unloaded (cleared) in this case ?

tedyu
tedyu reviewed Nov 13, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go
return nil // this can happen if we've been unable load data from the apiserver for some reason
}

return c.caBundle.Load().(*caBundleAndVerifier).caBundle
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should add verification that cast to *caBundleAndVerifier is successful.

tedyu
tedyu reviewed Nov 13, 2019
View reviewed changes
staging/src/k8s.io/client-go/util/cert/server_inspection.go
if err != nil {
return nil, err
}
defer conn.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we can directly close (without deferring)

tedyu
tedyu reviewed Nov 13, 2019
View reviewed changes
staging/src/k8s.io/client-go/util/cert/server_inspection.go
if err != nil {
return nil, nil, err
}
conn.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

conn.ConnectionState() depends on completed handshake.

I think we should check the return code from Close() where:

	if c.handshakeComplete() {
		alertErr = c.closeNotify()

If error is returned from Close(), most likely the assignment on line 77 wouldn't be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conn.ConnectionState() depends on completed handshake.

I think we should check the return code from Close() where:

	if c.handshakeComplete() {
		alertErr = c.closeNotify()

If error is returned from Close(), most likely the assignment on line 77 wouldn't be useful.

Yes, this is a reasonable change. Feel free to get me on slack to review/approve the change. Please update the rest of the file to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #85284

@k8s-ci-robot k8s-ci-robot merged commit 02af1dd into kubernetes:master Nov 13, 2019
@tedyu tedyu mentioned this pull request Nov 14, 2019
Merged
@liggitt liggitt mentioned this pull request Apr 27, 2020
Open
tamalsaha added a commit to stashed/installer that referenced this pull request May 31, 2020
@tamalsaha
Add configmap list/watch to delegated authentication reader role
3200058 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to stashed/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch -ing for delegated authentication checking (
be006f6 
#55)

xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to kubevault/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
215a909 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to kubevault/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
fa36825 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to kubevault/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication (#31)
3ba9834 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to kubedb/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
ea83fde 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to kubedb/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication (#81)
f391304 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to open-viz/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
972cdea 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to open-viz/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication (#9)
b1acae9 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to kubeshield/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
5e5d960 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to kubeshield/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication (#7)
3b270c4 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to kubepack/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication
1c16388 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha tamalsaha mentioned this pull request May 31, 2020
Merged
tamalsaha added a commit to kubepack/installer that referenced this pull request May 31, 2020
@tamalsaha
Permit configmap list/watch for delegated authentication (#7)
1a7c761 
xref: kubernetes/kubernetes#85004

Signed-off-by: Tamal Saha <tamal@appscode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tedyu tedyu tedyu left review comments

@sttts sttts sttts left review comments

@enj enj Awaiting requested review from enj

@jennybuckley jennybuckley Awaiting requested review from jennybuckley

Assignees

@mfojtik mfojtik

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

delegated authn does not automatically reload when ca bundles change
5 participants
@deads2k @k8s-ci-robot @mfojtik @sttts @tedyu