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

Exclude by default address flag fix #1234 #71973

Merged
merged 1 commit into from
Jan 1, 2019
Merged

Exclude by default address flag fix #1234 #71973

merged 1 commit into from
Jan 1, 2019

Conversation

MalloZup
Copy link
Contributor

@MalloZup MalloZup commented Dec 11, 2018 •
edited
Loading

What type of PR is this?

/kind bug

Does this PR introduce a user-facing change?

kubeadm: remove the deprecated "--address" flag for controller-manager and scheduler.

Which issue(s) this PR fixes:

fix kubernetes/kubeadm#1234

release note

kubeadm: remove the deprecated "--address" flag for controller-manager and scheduler.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 11, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @MalloZup. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 11, 2018
@neolit123
Copy link
Member

neolit123 commented Dec 12, 2018 •
edited
Loading

@kubernetes/sig-cluster-lifecycle-pr-reviews
/kind cleanup
/priority important-longterm

@MalloZup
thank you for this PR.

some k8s PRs need a "release note" to be added in the PR description.
https://git.k8s.io/community/contributors/guide/release-notes.md

just add this:

Does this PR introduce a user-facing change?:

kubeadm: remove the deprecated "--address" flag for controller-manager and scheduler.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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 Dec 12, 2018
neolit123
neolit123 approved these changes Dec 12, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 12, 2018
@neolit123
Copy link
Member

/retest

@neolit123
Copy link
Member

@MalloZup
please add the release note in a block as can be seen here:
https://raw.githubusercontent.com/kubernetes/kubernetes/master/.github/PULL_REQUEST_TEMPLATE.md

it has to be in a code block using "```release-note"

@MalloZup
Copy link
Contributor Author

@neolit123 many thanks for your comments/helps ❤️ . Is the description now correct ? thank you

@neolit123
Copy link
Member

@neolit123 many thanks for your comments/helps heart . Is the description now correct ? thank you

not quite yet :)

as you can see the bot has not removed the do-not-merge/release-note-label-needed label yet.
to fix that you need to have your release note in a block starting with

" ```release-note "

instead of:

" ``` "

@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/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 12, 2018
@MalloZup
Copy link
Contributor Author

/retest

rosti
rosti approved these changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @MalloZup ! Finally we confine insecure listeners to history!

/lgtm

@rosti
Copy link
Contributor

rosti commented Dec 13, 2018

/retest

@cblecker
Copy link
Member

/test pull-kubernetes-godeps

@MalloZup
Copy link
Contributor Author

seems ok now. flakyness in the tests has gone

yagonobre
yagonobre approved these changes Dec 17, 2018
View reviewed changes
Copy link
Member

@yagonobre yagonobre left a comment

Choose a reason for hiding this comment

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

/lgtm

@rosti
Copy link
Contributor

rosti commented Dec 18, 2018

Not an urgent one, but it would be great to merge this simple fix this year.
/assign @fabriziopandini

fabriziopandini
fabriziopandini approved these changes Dec 20, 2018
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@MalloZup Thanks for this PR! It is always nice to see new contributors start helping on kubeadm!

I'm ok with the change with only a minor concern that I want to double check with @luxas or @timothysc.
By removing --address=127.0.0.1 we are moving to use --bind-address dafult that is 0.0.0.0. That's means that we will expose the scheduler and the controller-manager secure port to the outside world. Is that ok for everyone?

/lgtm
I let final approval pending for the above check

@MalloZup
Copy link
Contributor Author

MalloZup commented Dec 20, 2018 •
edited
Loading

@fabriziopandini thank you for feedback. if i can help here let me know.

I hope one day to become a well-known old one contributor in the club 👵 😄

@neolit123
Copy link
Member

That's means that we will expose the scheduler and the controller-manager secure port to the outside world. Is that ok for everyone?

this is my concern exactly. we should do --bind-address in the same PR.

@MalloZup
PTAL at this comment in the attached issue:

--bind-address=127.0.0.1 in both manifests (kube-controller-manager and kube-scheduler) instead of --address=127.0.0.1

@neolit123
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2018
@MalloZup MalloZup mentioned this pull request Dec 20, 2018
Closed
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 20, 2018
@MalloZup
Copy link
Contributor Author

@neolit123 @fabriziopandini i have sed accordingly your remarks.

Feel free to take a look once you have some time. tia 🌻

@fabriziopandini
Copy link
Member

@MalloZup thanks for this new version
/approve
I left the final lgtm + lift the hold to @neolit123

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, MalloZup

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 28, 2018
neolit123
neolit123 approved these changes Jan 1, 2019
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm
thanks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 1, 2019
@neolit123
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit 993c656 into kubernetes:master Jan 1, 2019
@aojea aojea mentioned this pull request Dec 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yagonobre yagonobre yagonobre approved these changes

@zouyee zouyee zouyee approved these changes

@fabriziopandini fabriziopandini fabriziopandini approved these changes

@rosti rosti rosti approved these changes

@neolit123 neolit123 neolit123 approved these changes

@chuckha chuckha Awaiting requested review from chuckha

@luxas luxas Awaiting requested review from luxas

Assignees

@timothysc timothysc

@neolit123 neolit123

@rosti rosti

@fabriziopandini fabriziopandini

@luxas luxas

@yagonobre yagonobre

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubeadm use deprecated controller-manager and scheduller flags
10 participants
@MalloZup @k8s-ci-robot @neolit123 @rosti @cblecker @fabriziopandini @zouyee @yagonobre @timothysc @luxas