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

apiserver: get rid of ReadWritePort in config #65830

Merged
merged 3 commits into from
Jul 9, 2018
Merged

apiserver: get rid of ReadWritePort in config #65830

merged 3 commits into from
Jul 9, 2018

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jul 4, 2018 •
edited
Loading

Executing removal TODO by making the read write port logic explicit, and not hidden deep in the secure serving code.

Preparation for #65832

@k8s-ci-robot
Copy link
Contributor

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 4, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2018
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed 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 Jul 4, 2018
@sttts sttts force-pushed the sttts-apiserver-readwrite-port branch 2 times, most recently from 0ba9dad to 759f6e5 Compare July 4, 2018 17:56
@k8s-ci-robot
Copy link
Contributor

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 4, 2018
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed 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 Jul 4, 2018
deads2k
deads2k reviewed Jul 5, 2018
View reviewed changes
pkg/master/controller.go Outdated
@@ -79,6 +79,15 @@ type Controller struct {

// NewBootstrapController returns a controller for watching the core capabilities of the master
func (c *completedConfig) NewBootstrapController(legacyRESTStorage corerest.LegacyRESTStorage, serviceClient coreclient.ServicesGetter, nsClient coreclient.NamespacesGetter, eventClient coreclient.EventsGetter) *Controller {
publicServicePort := 443 // this is the legacy behaviour. Without a secure port, we cannot serve the kubernetes service correctly though.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird. If we require it, why not simply fail early if we have no secure port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just replicating the old behaviour here. Am fine with failing as well. In other words: do not allow zero with --secure-port.

Copy link
Member

Choose a reason for hiding this comment

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

What's the behavior previously when --secure-port is set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then https://github.com/kubernetes/kubernetes/pull/65830/files#diff-01f8ed603e3f5cc358d0394c0ab0cb00L51 was skipped and the default value of 443 was staying.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I tested this. And it is really not grace. In this case, kube-apiserver creates kubernetes service and with its own endpoints, but can not access.

In other words: do not allow zero with --secure-port.

+1

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

@sttts sttts force-pushed the sttts-apiserver-readwrite-port branch 2 times, most recently from 4835dad to 3595746 Compare July 6, 2018 16:29
deads2k
deads2k reviewed Jul 6, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/config.go Outdated
host = net.JoinHostPort(host, strconv.Itoa(port))
} else {
// last way out: 443. This is the legacy behaviour for this case.
host = net.JoinHostPort(host, strconv.Itoa(443))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we error here in instead?

deads2k
deads2k reviewed Jul 6, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/config.go Outdated
if s == nil || s.Listener == nil {
return "", 0, fmt.Errorf("no listener found")
}
host, portStr, err := net.SplitHostPort(s.Listener.Addr().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

lost this error.

@deads2k
Copy link
Contributor

deads2k commented Jul 6, 2018

minor comments, lgtm otherwise, but the lost error needs handling.

@sttts sttts force-pushed the sttts-apiserver-readwrite-port branch from 6dcc9a4 to 15e7b0f Compare July 9, 2018 11:51
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2018
@sttts sttts force-pushed the sttts-apiserver-readwrite-port branch 2 times, most recently from f980b50 to a06de13 Compare July 9, 2018 12:50
@sttts sttts force-pushed the sttts-apiserver-readwrite-port branch from a06de13 to 6bb3aba Compare July 9, 2018 13:01
@deads2k
Copy link
Contributor

deads2k commented Jul 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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]

@ingvagabund
Copy link
Contributor

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 6bb3aba link /test pull-kubernetes-e2e-kops-aws

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
Copy link

Automatic merge from submit-queue (batch tested with PRs 65830, 65780, 65961). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f6bbf1f into kubernetes:master Jul 9, 2018
@fedebongio
Copy link
Contributor

/cc @caesarxuchao

@mikedanese mikedanese mentioned this pull request Dec 11, 2019
Merged
@aryan9600 aryan9600 mentioned this pull request Sep 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@deads2k deads2k deads2k left review comments

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

Assignees

@deads2k deads2k

@hzxuzhonghu hzxuzhonghu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sttts @k8s-ci-robot @deads2k @k8s-github-robot @ingvagabund @fedebongio @hzxuzhonghu