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

Expose system component ports to the cluster #358

Closed
wants to merge 1 commit into from

Conversation

kawych
Copy link
Contributor

@kawych kawych commented Jun 19, 2018

What this PR does / why we need it:
Expose system component ports to the cluster. The goal is to expose metrics from master to a monitoring agent that runs on a cluster.

Special notes for your reviewer
I'm adding these endpoints to make sure the components running on master can be monitored. Is it OK to expose these endpoints from security point of view?

Also, the way how MasterConfiguration is specified is inconsistent between GCP (part of clusterctl input) and vSphere (part of machine controller image). Is there some fundamental reason to do it differently, or are there plans to converge?

Release note:

Controller Manager, Scheduler, Kube Proxy and API Server ETCD ports are now exposed to the cluster.

@kubernetes/kube-deploy-reviewers
@krousey @karan @jessicaochen

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kawych
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: krousey

Assign the PR to them by writing /assign @krousey in a comment when ready.

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 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 Jun 19, 2018
@kawych
Copy link
Contributor Author

kawych commented Jun 19, 2018

/cc @serathius

@k8s-ci-robot
Copy link
Contributor

@kawych: GitHub didn't allow me to request PR reviews from the following users: serathius.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @serathius

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.

@serathius
Copy link

/lgtm
We are still need to do the same for kube-proxy

@k8s-ci-robot
Copy link
Contributor

@serathius: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/lgtm
We are still need to do the same for kube-proxy

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.

@kawych
Copy link
Contributor Author

kawych commented Jun 21, 2018

@k8s-ci-robot k8s-ci-robot requested review from jessicaochen, karan, kcoronado and rsdcastro and removed request for timothysc June 21, 2018 09:51
@spew
Copy link
Contributor

spew commented Jun 24, 2018

Please change the release notes to be written from the action based perspective of a user, for example, something like

System component ports are now exposed to the cluster.

Ideally the notes would specify an example of what a "system component" is.

@kawych
Copy link
Contributor Author

kawych commented Jun 25, 2018

Done

@kawych
Copy link
Contributor Author

kawych commented Jun 25, 2018

Also added Kube Proxy to the exposed components. PTAL

@@ -252,6 +252,12 @@ data:
api:
advertiseAddress: ${PUBLICIP}
bindPort: ${PORT}
etcd:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update clusterctl/examples/vsphere/provider-components.yaml.template as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no analogous MasterConfiguration there. For vSphere, this is configured in cloud/vshpere/templates.go (hardcoded in the machine controller image). In this PR I edit this file as well. Long term I think it would be useful to have a consistency, although I don't have a full context - it was probably implemented differently for a reason.

@roberthbailey roberthbailey self-assigned this Jun 26, 2018
@roberthbailey
Copy link
Contributor

@kawych - I don't think it's a good idea to expose etcd to the cluster. etcd should only be accessible to the apiserver, otherwise you can bypass the authn/z and have root level access to the cluster. It's going to be difficult to lock it down later if it's exposed now as we will inevitably gain new dependencies that assume they can reach it.

Can you explain more about why this is necessary? I've enqueued this PR into the community meeting tomorrow if you'd rather do it verbally than through comments.

@serathius
Copy link

We would like to have access to metrics from cluster.
Best solution would be to bind metrics on separate port. Based on zalando-stups/stups-etcd-cluster#48 it's available from version 3.3

Could we upgrade etcd deployed to 3.3+?

@kawych
Copy link
Contributor Author

kawych commented Jul 2, 2018

@roberthbailey
Is there an option to upgrade etcd to 3.3, so that we can expose just a metrics port?

@krousey krousey removed their request for review July 16, 2018 16:20
@roberthbailey
Copy link
Contributor

All of the files modified by this PR have been migrated out of these repositories so I'm going to close this PR. Please re-open against the specific provider repositories that are affected.

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants