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

Check loopback and link-local multicast endpoints #10713

Merged
merged 2 commits into from
Aug 19, 2015
Merged

Check loopback and link-local multicast endpoints #10713

merged 2 commits into from
Aug 19, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Jul 4, 2015

Previously we just disallowed link-local (unicast). This disallows loopback
and link-local multicast.

Fixes #10349

@k8s-bot
Copy link

k8s-bot commented Jul 4, 2015

GCE e2e build/test failed for commit e299ebfcceff3d09869c453fec07ea23d7d9476c.

@bprashanth
Copy link
Contributor

Do we need to document why we disallow link-local (it sounds like we're disallowing the other 2 because we really want to disallow link-local)? It might suffice to say they're special, though I think link-local is used by cloud providers to serve metadata, so it might be the one many people try first.

@thockin
Copy link
Member Author

thockin commented Jul 4, 2015

Good point, will push with a new comment. will add to docs this week.

On Sat, Jul 4, 2015 at 11:02 AM, Prashanth B notifications@github.com
wrote:

Do we need to document why we disallow link-local (it sounds like we're
disallowing the other 2 because we really want to disallow link-local)? It
might suffice to say they're special, though I think link-local is used by
cloud providers to serve metadata, so it might be the one many people try
first.

—
Reply to this email directly or view it on GitHub
#10713 (comment)
.

@thockin thockin force-pushed the no-localhost-endpoints branch from e299ebf to 7f11d03 Compare July 4, 2015 20:26
@k8s-bot
Copy link

k8s-bot commented Jul 4, 2015

GCE e2e build/test failed for commit 7f11d03aad3bdb5f5c03cb2943975ba7baf93342.

@thockin thockin force-pushed the no-localhost-endpoints branch from 7f11d03 to 0573147 Compare July 4, 2015 21:42
@k8s-bot
Copy link

k8s-bot commented Jul 4, 2015

GCE e2e build/test failed for commit 0573147b7628a72c57f068616c8524a67364e057.

@thockin
Copy link
Member Author

thockin commented Jul 4, 2015

@nikhiljindal there's a problem here - swagger is emitting the "advertise address" flag here, but that's going to be different for every person since this PR makes 127.0.0.1 an invalid address to advertise. Thoughts on how hard it would be to emit a different address in swagger files?

@k8s-bot
Copy link

k8s-bot commented Jul 4, 2015

GCE e2e build/test passed for commit a7f1a008f49e13cf19c877843bfab7bf7f48c858.

@thockin
Copy link
Member Author

thockin commented Jul 6, 2015

need 1.0 triage, I think this is (sadly) out for 1.0 at this point.

@bgrant0607 bgrant0607 added this to the v1.0-post milestone Jul 6, 2015
@bgrant0607 bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015
@bgrant0607 bgrant0607 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 30, 2015
@bgrant0607
Copy link
Member

Swagger needs to report an address that clients are likely going to be able to reach. We've had multiple bug reports about this. Can we strip out the hostname/address post-generation? @nikhiljindal: would validation and the UI work if we did that?

I'm fine with this change in principle. We need to be sure it ends up in the release notes, since it is a change of behavior.

@bgrant0607
Copy link
Member

Needs rebase.

@bgrant0607 bgrant0607 removed their assignment Jul 30, 2015
@bgrant0607
Copy link
Member

I'm overloaded with API reviews. I need someone else to wrap up the code review.

@thockin thockin force-pushed the no-localhost-endpoints branch from a7f1a00 to 120536c Compare July 31, 2015 04:44
@thockin
Copy link
Member Author

thockin commented Jul 31, 2015

@nikhiljindal Can we brainstorm some ideas on this?

@k8s-bot
Copy link

k8s-bot commented Jul 31, 2015

GCE e2e build/test passed for commit 120536c1e9eda7f986330f3fc014c6f81f3e163c.

@nikhiljindal
Copy link
Contributor

Hey apologies for the delay in getting to this.

How about passing "advertise-address" flag in update-swagger-spec: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/hack/update-swagger-spec.sh#L52
That ensures that it is the same for everyone?

@thockin thockin force-pushed the no-localhost-endpoints branch from 120536c to dc0eb41 Compare August 8, 2015 20:42
@thockin
Copy link
Member Author

thockin commented Aug 8, 2015

I updated this with a fixed address for swagger. It's not worse than before (in that it will still confuse users), but not better either.

@k8s-bot
Copy link

k8s-bot commented Aug 8, 2015

GCE e2e build/test passed for commit dc0eb4134b2f89d2823045bb8fdc0c40a1c7ec6d.

@nikhiljindal
Copy link
Contributor

Thanks!
The swagger-spec changes LGTM.

@bprashanth to give final LGTM.

@thockin thockin force-pushed the no-localhost-endpoints branch from dc0eb41 to 698b96a Compare August 18, 2015 04:38
@k8s-bot
Copy link

k8s-bot commented Aug 18, 2015

GCE e2e build/test passed for commit 698b96a367034624e7e8f13b2ac8ec2f01ced222.

@thockin thockin closed this Aug 18, 2015
@thockin thockin reopened this Aug 18, 2015
@bprashanth
Copy link
Contributor

LGTM, please file bug or do doc change when you have time

@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2015
@thockin
Copy link
Member Author

thockin commented Aug 19, 2015

Updating docs now, fair point.

thockin added 2 commits August 18, 2015 21:50
@thockin
Check loopback and link-local multicast endpoints
86f4535 
Previously we just disallowed link-local (unicast).  This disallows loopback
and link-local multicast.
@thockin
Use a valid address for swagger
16a3331
@thockin thockin force-pushed the no-localhost-endpoints branch from 698b96a to 16a3331 Compare August 19, 2015 04:50
@thockin
Copy link
Member Author

thockin commented Aug 19, 2015

re-pushed with API and services docs updated

@k8s-bot
Copy link

k8s-bot commented Aug 19, 2015

GCE e2e build/test passed for commit 16a3331.

saad-ali added a commit that referenced this pull request Aug 19, 2015
@saad-ali
Merge pull request #10713 from thockin/no-localhost-endpoints
c1a2c6d 
Check loopback and link-local multicast endpoints
@saad-ali saad-ali merged commit c1a2c6d into kubernetes:master Aug 19, 2015
@thockin thockin deleted the no-localhost-endpoints branch August 27, 2015 17:58
@nikhiljindal nikhiljindal mentioned this pull request Sep 16, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bprashanth bprashanth

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@thockin @k8s-bot @bprashanth @bgrant0607 @nikhiljindal @googlebot @saad-ali