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

use controller to publish cluster authentication info #82705

Merged
merged 4 commits into from Nov 7, 2019
Merged

use controller to publish cluster authentication info #82705

merged 4 commits into from Nov 7, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 13, 2019 •
edited

fixes #82429

/kind bug
/priority important-soon

configmaps/extension-apiserver-authentication in kube-system is continuously updated by kube-apiservers, instead of just at apiserver start

@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. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 13, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 1, 2019
@deads2k deads2k changed the title [WIP] use controller to publish cluster authentication info use controller to publish cluster authentication info Nov 1, 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. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Nov 1, 2019
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 1, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Nov 1, 2019

/retest

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
cmd/kube-apiserver/app/server.go Outdated
}
if requestHeaderConfig, err := s.Authentication.RequestHeader.ToAuthenticationRequestHeaderConfig(); err != nil {
return nil, nil, nil, nil, err
} else if requestHeaderConfig != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

drop else

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
RequestHeaderCA dynamiccertificates.CAContentProvider
}

// NewClusterAuthenticationTrustController returns a controller that will maintain the `kubectl get -n kube-system configmap/extension-apiserver-authentication`
Copy link
Contributor

Choose a reason for hiding this comment

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

this kubectl wording makes me stumble upon on every read.

Copy link
Contributor

Choose a reason for hiding this comment

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

... that will keep the configmap kube-system/.... up to date ...

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
klog.V(4).Info("no changes to configmap")
return nil
}
klog.V(2).Infof("writing updated authentication info to `kubectl get -n %s configmaps/%s", configMapNamespace, configMapName)
Copy link
Contributor

Choose a reason for hiding this comment

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

another occurence of this strange kubectl wording.

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
}

if equality.Semantic.DeepEqual(authConfigMap, originalAuthConfigMap) {
klog.V(4).Info("no changes to configmap")
Copy link
Contributor

Choose a reason for hiding this comment

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

Increase verbosity for doing nothing.

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
return nil
}
klog.V(2).Infof("writing updated authentication info to `kubectl get -n %s configmaps/%s", configMapNamespace, configMapName)
klog.V(4).Infof("%v", spew.Sdump(authConfigMap.Data))
Copy link
Contributor

Choose a reason for hiding this comment

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

That a lot of output for V(4)

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
}
}

if authenticationInfo.RequestHeaderCA != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

return early

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go Outdated
return headerrequest.StaticStringSlice(out), nil
}

func combineStringSlices(lhs, rhs headerrequest.StringSliceProvider) headerrequest.StringSliceProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually combineUniqueStringSlices

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/cluster_authentication_trust_controller.go
certificates = filterExpiredCerts(certificates...)

finalCertificates := []*x509.Certificate{}
// now check for duplicates. n^2, but super simple
Copy link
Contributor

@sttts sttts Nov 5, 2019 •
edited

Choose a reason for hiding this comment

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

one of lhs or rhs is small => n^2 is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't there some unique property in the CA you can use instead?

How big can n get? Another cert per rotation? Rotation is each 30 days => 12 per year. So probably not so big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't there some unique property in the CA you can use instead?

How big can n get? Another cert per rotation? Rotation is each 30 days => 12 per year. So probably not so big.

You're looking at order of 10 infrequently checked. I think this is fine until you see it in a profile.

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/master.go Outdated

"k8s.io/client-go/kubernetes"

"k8s.io/kubernetes/pkg/master/controller/clusterauthenticationtrust"
Copy link
Contributor

Choose a reason for hiding this comment

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

order

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
pkg/master/controller/clusterauthenticationtrust/testdata/generate.sh Outdated
cfssl gencert -initca root-1.csr.json | cfssljson -bare root
cfssl gencert -initca root-2.csr.json | cfssljson -bare root
cfssl gencert -initca root-3.csr.json | cfssljson -bare root
cfssl gencert -initca root-4.csr.json | cfssljson -bare root
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is relevant: do they pass the fips tests?

sttts
sttts reviewed Nov 5, 2019
View reviewed changes
test/integration/apiserver/certreload/certreload_test.go

"k8s.io/apimachinery/pkg/util/wait"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

order

@deads2k deads2k force-pushed the agg-authn-publish branch 2 times, most recently from fc4ef2d to 46f10e5 Compare November 5, 2019 19:56
@sttts
Copy link
Contributor

sttts commented Nov 6, 2019

/retest
/lgtm

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

deads2k commented Nov 6, 2019

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit a5fe905 into kubernetes:master Nov 7, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 7, 2019
@ingvagabund ingvagabund mentioned this pull request Nov 16, 2019
Closed
@ingvagabund ingvagabund mentioned this pull request Dec 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@david-mcmahon david-mcmahon Awaiting requested review from david-mcmahon

Assignees

@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.

cm/extension-apiserver-authentication is "last one wins"
4 participants
@deads2k @k8s-ci-robot @sttts @fejta-bot