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-proxy: Fix flag validation for healthz-bind-address and metrics-bind-address #54191

Merged
merged 1 commit into from
Feb 22, 2018
Merged

kube-proxy: Fix flag validation for healthz-bind-address and metrics-bind-address #54191

merged 1 commit into from
Feb 22, 2018
+179 −10

Conversation

MrHohn
Copy link
Member

@MrHohn MrHohn commented Oct 19, 2017 •
edited by ncdc
Loading

What this PR does / why we need it: --healthz-bind-address and --metrics-bind-address are broken for kube-proxy as they do not allow ip:port format, though they claim to support it.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): Fixes #53754

Special notes for your reviewer:
cc @ncdc

Release note:

Fix kube-proxy flags validation for --healthz-bind-address and --metrics-bind-address to allow specifying ip:port.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 19, 2017
@MrHohn MrHohn mentioned this pull request Oct 19, 2017
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2017
@ncdc
Copy link
Member

ncdc commented Nov 14, 2017

@MrHohn do you plan to rebase this (to match the move from componentconfig to pkg/proxy/apis/kubeproxyconfig) for 1.9?

@MrHohn
Copy link
Member Author

MrHohn commented Nov 14, 2017

@ncdc Thanks for the reminder, I missed the rebase-needed signal. Will rebase and let you know.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Nov 14, 2017

@ncdc Rebased. Though IPPortVar is kept in componentconfig/helpers.go as I haven't figured out a better place.

@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 release-note-none Denotes a PR that doesn't merit a release note. labels Nov 14, 2017
@ncdc
Copy link
Member

ncdc commented Nov 15, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017
@ncdc
Copy link
Member

ncdc commented Nov 15, 2017

/sig network

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Nov 15, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Nov 15, 2017

Thanks for the quick review.

/assign @mikedanese
for approval.

@MrHohn
Copy link
Member Author

MrHohn commented Nov 27, 2017

/retest

@MrHohn
Copy link
Member Author

MrHohn commented Nov 27, 2017

/test pull-kubernetes-unit

@BenTheElder
Copy link
Member

/test pull-kubernetes-cross
[borrowing legitimate PR for test-infra purposes :-)]

@MrHohn
Copy link
Member Author

MrHohn commented Nov 29, 2017

/test pull-kubernetes-e2e-gce

1 similar comment
@MrHohn
Copy link
Member Author

MrHohn commented Dec 4, 2017

/test pull-kubernetes-e2e-gce

@MrHohn
Copy link
Member Author

MrHohn commented Dec 5, 2017 •
edited
Loading

/assign @bowei @thockin
It might be worth to get this in 1.9 (and cherrypick to 1.8, 1.7 as well). These two flags (especially --metrics-bind-address) are almost unusable without the fix.

@MrHohn
Copy link
Member Author

MrHohn commented Dec 5, 2017

/kind bug
/sig network
/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Dec 5, 2017
@kubernetes kubernetes deleted a comment from k8s-github-robot Dec 27, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 27, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Dec 27, 2017

Updated PR for ipv6.

@MrHohn
Copy link
Member Author

MrHohn commented Jan 8, 2018

/retest

bowei
bowei reviewed Jan 31, 2018
View reviewed changes
pkg/apis/componentconfig/helpers.go Outdated
return nil
}
// Both IP and IP:port are valid.
if net.ParseIP(s) == nil {
Copy link
Member

Choose a reason for hiding this comment

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

reads better if you just bail out early

if net.ParseIP(s) != nil {
  *v.Val = s
  return nil
}

...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, done.

@bowei
Copy link
Member

bowei commented Jan 31, 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 Jan 31, 2018
@bowei
Copy link
Member

bowei commented Jan 31, 2018

/approve

@krzyzacy
Copy link
Member

/retest

@MrHohn MrHohn force-pushed the kube-proxy-metrics-flag-fix branch from 7d7318d to 316c369 Compare January 31, 2018 22:02
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2018
@MrHohn
Copy link
Member Author

MrHohn commented Jan 31, 2018

@bowei @thockin PR is updated. Thanks!

@krzyzacy
Copy link
Member

/test pull-kubernetes-bazel-build

@bowei
Copy link
Member

bowei commented Feb 21, 2018

/status approved-for-milestone

@mikedanese
Copy link
Member

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2018
@mikedanese
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, mikedanese, MrHohn, ncdc

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

Tests are more than 96 hours old. Re-running tests.

@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. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 097d3f1 into kubernetes:master Feb 22, 2018
@xiangpengzhao xiangpengzhao mentioned this pull request Apr 13, 2018
Closed
@MrHohn MrHohn mentioned this pull request Apr 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei bowei left review comments

@thockin thockin thockin left review comments

Assignees

@ncdc ncdc

@bowei bowei

@mikedanese mikedanese

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kube-proxy cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. 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.

Kube-proxy metrics bind
9 participants
@MrHohn @ncdc @BenTheElder @bowei @krzyzacy @mikedanese @k8s-ci-robot @k8s-github-robot @thockin