-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Honor --use-service-account-credentials in cloud-controller-manager #50289
Honor --use-service-account-credentials in cloud-controller-manager #50289
Conversation
cc @kubernetes/sig-auth-pr-reviews |
@@ -75,7 +75,6 @@ func (s *CloudControllerManagerServer) AddFlags(fs *pflag.FlagSet) { | |||
fs.DurationVar(&s.NodeMonitorPeriod.Duration, "node-monitor-period", s.NodeMonitorPeriod.Duration, | |||
"The period for syncing NodeStatus in NodeController.") | |||
fs.DurationVar(&s.NodeStatusUpdateFrequency.Duration, "node-status-update-frequency", s.NodeStatusUpdateFrequency.Duration, "Specifies how often the controller updates nodes' status.") | |||
fs.StringVar(&s.ServiceAccountKeyFile, "service-account-private-key-file", s.ServiceAccountKeyFile, "Filename containing a PEM-encoded private RSA or ECDSA key used to sign service account tokens.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag was definitely around in 1.7 https://github.com/kubernetes/kubernetes/blob/v1.7.0/cmd/cloud-controller-manager/app/options/options.go#L78
Do we need to mark it depricated first or is it okay just to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to mark it depricated first or is it okay just to remove it?
not sure, so marked deprecated for now instead of removing
@liggitt relnote? |
c9bbf89
to
55c7ce1
Compare
/retest |
1 similar comment
/retest |
/lgtm /test pull-kubernetes-e2e-gce-etcd3 |
This looks good! Thanks for the PR @liggitt |
/test pull-kubernetes-kubemark-e2e-gce |
/retest |
Whoa, how didn't this get merged? /lgtm |
/approve no-issue |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, luxas, wlan0 Associated issue requirement bypassed by: luxas 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 |
/retest |
@@ -77,7 +77,9 @@ func (s *CloudControllerManagerServer) AddFlags(fs *pflag.FlagSet) { | |||
fs.DurationVar(&s.NodeMonitorPeriod.Duration, "node-monitor-period", s.NodeMonitorPeriod.Duration, | |||
"The period for syncing NodeStatus in NodeController.") | |||
fs.DurationVar(&s.NodeStatusUpdateFrequency.Duration, "node-status-update-frequency", s.NodeStatusUpdateFrequency.Duration, "Specifies how often the controller updates nodes' status.") | |||
// TODO: remove --service-account-private-key-file 6 months after 1.8 is released (~1.10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is in alpha, we're probably gonna delete this (not include) when going to beta (hopefully in v1.9)
/test all Tests are more than 96 hours old. Re-running tests. |
/retest Review the full test history for this PR. |
/test pull-kubernetes-federation-e2e-gce |
1 similar comment
/test pull-kubernetes-federation-e2e-gce |
/retest Review the full test history for this PR. |
1 similar comment
/retest Review the full test history for this PR. |
/test all [submit-queue is verifying that this PR is safe to merge] |
@liggitt: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 50289, 52106) |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. remove --service-account-private-key-file in v1.11 `kube-cloud-controller-manager` flag `--service-account-private-key-file` is removed in v1.11 ref: #50289 **Release note**: ```release-note `kube-cloud-controller-manager` flag `--service-account-private-key-file` is removed in v1.11 ```
If --use-service-account-credentials is specified, the cloud controller manager should honor it
The distinction between the rootclientbuilder and the clientbuilder came from kube-controller-manager, which is responsible for running the very controllers that enable service accounts. That two-layer approach is not needed in the cloud-controller-manager.