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

kube-ctrl-mgr: enable secure port 10257 #64149

Merged
merged 5 commits into from
Aug 31, 2018
Merged

kube-ctrl-mgr: enable secure port 10257 #64149

merged 5 commits into from
Aug 31, 2018

Conversation

sttts
Copy link
Contributor

@sttts sttts commented May 22, 2018 •
edited
Loading

This PR enables authn+authz (delegated to the kube-apiserver) and the secure port 10257 for the kube-controller-manager. In addition, the insecure port is disabled.

Moreover, it adds integration test coverage for the --port and --secure-port flags, plus the testserver infrastructure to tests flags in general inside integration tests.

Enable secure serving on port 10257 to kube-controller-manager (configurable via `--secure-port`). Delegated authentication and authorization are to be configured using the same flags as for aggregated API servers. Without configuration, the secure port will only allow access to `/healthz`.

@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/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 22, 2018
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2018
@sttts sttts force-pushed the sttts-ctrl-mgr-secure-ports branch from e717dfa to e0ed50a Compare May 30, 2018 14:25
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 30, 2018
@sttts sttts changed the title WIP: ctrl-managers: enable secure ports 10257, 10258 ctrl-managers: enable secure ports 10257, 10258 May 30, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2018
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2018
@stewart-yu stewart-yu mentioned this pull request Jul 6, 2018
Closed
@sttts sttts mentioned this pull request Jul 25, 2018
Closed
andrewsykim
andrewsykim reviewed Jul 25, 2018
View reviewed changes
pkg/master/ports/ports.go Outdated
@@ -41,4 +41,10 @@ const (
// ProxyHealthzPort is the default port for the proxy healthz server.
// May be overridden by a flag at startup.
ProxyHealthzPort = 10256
// InsecureKubeControllerManagerPort is the default port for the controller manager status server.
Copy link
Member

Choose a reason for hiding this comment

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

s/InsecureKubeControllerManagerPort/KubeControllerManagerPort/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

andrewsykim
andrewsykim reviewed Jul 25, 2018
View reviewed changes
pkg/master/ports/ports.go Outdated
// InsecureKubeControllerManagerPort is the default port for the controller manager status server.
// May be overridden by a flag at startup.
KubeControllerManagerPort = 10257
// InsecureCloudControllerManagerPort is the default port for the cloud controller manager server.
Copy link
Member

Choose a reason for hiding this comment

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

s/InsecureCloudControllerManagerPort/CloudControllerManagerPort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@tanshanshan tanshanshan mentioned this pull request Jul 26, 2018
Closed
@sttts sttts force-pushed the sttts-ctrl-mgr-secure-ports branch from e0ed50a to c19c9a2 Compare August 7, 2018 08:53
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2018
@sttts sttts mentioned this pull request Aug 7, 2018
Merged
@sttts sttts force-pushed the sttts-ctrl-mgr-secure-ports branch from c19c9a2 to be106d2 Compare August 7, 2018 08:59
@sttts sttts changed the title ctrl-managers: enable secure ports 10257, 10258 kube-ctrl-mgr: enable secure port 10257 Aug 7, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 7, 2018

Split out the cloud-ctrl-mgr changes into #67069

@sttts sttts force-pushed the sttts-ctrl-mgr-secure-ports branch from be106d2 to 82d4512 Compare August 7, 2018 09:44
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 7, 2018
@sttts
kube-controller-manager: add test server and test serving
e209b64
@sttts
kube-controller-manager: disable authn/z on insecure port
8aa0eef 
This is the old behaviour and we did not intent to change it due to enabled authn/z in general.
As the kube-apiserver this sets the "system:unsecured" user info.
@sttts sttts force-pushed the sttts-ctrl-mgr-secure-ports branch from 0075df3 to 8aa0eef Compare August 30, 2018 18:17
@sttts
Copy link
Contributor Author

sttts commented Aug 30, 2018

Rebased, advanced auditing GA merged.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 30, 2018

/retest

@sttts
Copy link
Contributor Author

sttts commented Aug 31, 2018

@liggitt lgty and ready to unhold?

@liggitt
Copy link
Member

liggitt commented Aug 31, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 31, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, 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-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 67756, 64149, 68076, 68131, 68120). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 5d4b0f8 into kubernetes:master Aug 31, 2018
@k8s-ci-robot
Copy link
Contributor

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce e0ed50af2ba0a374df6990e7293a36c701ab8911 link /test pull-kubernetes-kubemark-e2e-gce
pull-kubernetes-e2e-gce-100-performance 8aa0eef link /test pull-kubernetes-e2e-gce-100-performance

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.

k8s-github-robot pushed a commit that referenced this pull request Sep 1, 2018
Merge pull request #67069 from sttts/sttts-cloud-ctrl-mgr-secure-ports
62315e8 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

cloud-ctrl-mgr: enable secure port 10258

This PR enables authn+authz (delegated to the kube-apiserver) and the secure port 10258 for the cloud-controller-manager. In addition, the insecure port is disabled.

This is the counterpart PR to #64149.

Moreover, it adds integration test coverage for the `--port` and `--secure-port` flags, plus the testserver infrastructure to tests flags in general inside integration tests.

```release-note
Enable secure serving on port 10258 to cloud-controller-manager (configurable via `--secure-port`). Delegated authentication and authorization have to be configured like for aggregated API servers.
```
@liggitt liggitt mentioned this pull request Sep 4, 2018
Closed
sttts pushed a commit to sttts/apiserver that referenced this pull request Sep 5, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
5c59329 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
sttts pushed a commit to sttts/apiserver that referenced this pull request Sep 5, 2018
@k8s-publishing-bot
Merge pull request #67545 from sttts/sttts-auth-optional-kubeconfig
067a462 
Automatic merge from submit-queue (batch tested with PRs 66960, 67545). 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>.

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig

This adds `RemoteKubeConfigFileOptional` field to the delegated authn/z option structs. If set to true, the authn/z kubeconfig file flags are optional. If no kubeconfig is given, all token requests are considered to be anonymous and no client CA is looked up in the cluster.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

Kubernetes-commit: 1b3a2dd0830ca0e02d5b95d2ecc0161d0c93a0c7
sttts pushed a commit to sttts/kube-aggregator that referenced this pull request Sep 5, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
500fa58 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
sttts pushed a commit to sttts/sample-apiserver that referenced this pull request Sep 5, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
f292ba0 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
sttts pushed a commit to sttts/apiextensions-apiserver that referenced this pull request Sep 5, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
ccbacf7 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Sep 6, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
3096781 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Sep 6, 2018
@k8s-publishing-bot
Merge pull request #67545 from sttts/sttts-auth-optional-kubeconfig
8f4cb16 
Automatic merge from submit-queue (batch tested with PRs 66960, 67545). 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>.

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig

This adds `RemoteKubeConfigFileOptional` field to the delegated authn/z option structs. If set to true, the authn/z kubeconfig file flags are optional. If no kubeconfig is given, all token requests are considered to be anonymous and no client CA is looked up in the cluster.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

Kubernetes-commit: 1b3a2dd0830ca0e02d5b95d2ecc0161d0c93a0c7
k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this pull request Sep 6, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
37b7c36 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
k8s-publishing-bot added a commit to kubernetes/sample-apiserver that referenced this pull request Sep 6, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
dd13dc7 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
k8s-publishing-bot added a commit to kubernetes/apiextensions-apiserver that referenced this pull request Sep 6, 2018
@k8s-publishing-bot
Merge pull request #67543 from sttts/sttts-auth-skip-paths
7945a57 
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>.

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz)

Add `AlwaysAllowPaths` field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for kubernetes/kubernetes#64149 and kubernetes/kubernetes#67069.

```release-note
Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.
```

Kubernetes-commit: 5ed26a348b017c3ece8ac468d15770ddf8b922ae
This was referenced Sep 10, 2018
Closed
Merged
@vsxen vsxen mentioned this pull request Dec 14, 2018
Closed
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

@deads2k deads2k deads2k left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@nikhiljindal nikhiljindal Awaiting requested review from nikhiljindal

Assignees

@liggitt liggitt

@deads2k deads2k

@yue9944882 yue9944882

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/security cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@sttts @nikhita @yue9944882 @deads2k @liggitt @k8s-ci-robot @k8s-github-robot @andrewsykim