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

Exclude by default address flag fix #1234 #71973

Merged
merged 1 commit into from Jan 1, 2019

Conversation

@MalloZup
Copy link
Contributor

MalloZup commented Dec 11, 2018

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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 11, 2018

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.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 12, 2018

@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.

Copy link
Member

neolit123 left a comment

/ok-to-test

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 12, 2018

/retest

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 12, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

MalloZup commented Dec 12, 2018

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

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 12, 2018

@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:

" ``` "

@MalloZup

This comment has been minimized.

Copy link
Contributor Author

MalloZup commented Dec 12, 2018

/retest

@rosti
rosti approved these changes Dec 12, 2018
Copy link
Member

rosti left a comment

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

/lgtm

@rosti

This comment has been minimized.

Copy link
Member

rosti commented Dec 13, 2018

/retest

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Dec 13, 2018

/test pull-kubernetes-godeps

@MalloZup

This comment has been minimized.

Copy link
Contributor Author

MalloZup commented Dec 17, 2018

seems ok now. flakyness in the tests has gone

@zouyee
zouyee approved these changes Dec 17, 2018
Copy link
Member

yagonobre left a comment

/lgtm

@rosti

This comment has been minimized.

Copy link
Member

rosti commented Dec 18, 2018

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

Copy link
Member

fabriziopandini left a comment

@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

This comment has been minimized.

Copy link
Contributor Author

MalloZup commented Dec 20, 2018

@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

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Member

neolit123 commented Dec 20, 2018

/hold

@MalloZup MalloZup force-pushed the MalloZup:fix-1234 branch from a038492 to 6148992 Dec 20, 2018
@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/S labels Dec 20, 2018
@MalloZup

This comment has been minimized.

Copy link
Contributor Author

MalloZup commented Dec 21, 2018

@neolit123 @fabriziopandini i have sed accordingly your remarks.

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

@fabriziopandini

This comment has been minimized.

Copy link
Member

fabriziopandini commented Dec 28, 2018

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 28, 2018

[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

Copy link
Member

neolit123 left a comment

/lgtm
thanks

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jan 1, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 993c656 into kubernetes:master Jan 1, 2019
19 checks passed
19 checks passed
cla/linuxfoundation MalloZup authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.