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.

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

use controller to publish cluster authentication info #82705

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 13, 2019

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

}
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

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

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.

}

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.

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)

}
}

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

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

certificates = filterExpiredCerts(certificates...)

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

@sttts sttts Nov 5, 2019

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.


"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

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?


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

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

Successfully merging this pull request may close these issues.

cm/extension-apiserver-authentication is "last one wins"
4 participants