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

net/http: update docs for LocalAddrContextKey #21603

Closed
sabey opened this issue Aug 25, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@sabey
Copy link

commented Aug 25, 2017

What version of Go are you using (go version)?

1.9

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

amd64

What did you do?

r is a *http.Request
r.Context().Value(http.LocalAddrContextKey)

What did you expect to see?

Listener Addr

What did you see instead?

Connection Local Addr

I replied to an older closed issue, I'm not sure if it will be seen or not so I am posting again: #18686
It seems that the functionality of LocalAddrContextKey has changed to what it was possibly intended to do, if so it should be mentioned in the patch notes

source change: https://go-review.googlesource.com/c/go/+/35490/10/src/net/http/server.go

@ianlancetaylor ianlancetaylor changed the title Update Go Doc for net/http LocalAddrContextKey net/http: update docs for LocalAddrContextKey Aug 25, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

What do you mean by the "patch notes"? Do you mean the contents of https://golang.org/doc/go1.9? Or do you mean the documentation of LocalAddrContextKey at https://golang.org/pkg/net/http/#pkg-variables ?

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 25, 2017

@sabey

This comment has been minimized.

Copy link
Author

commented Aug 25, 2017

sorry, I meant release notes, and yes http.LocalAddrContextKey

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

Thanks. Not every change is mentioned in the release notes, only the ones that seem important or significant.

CC @tombergan Should this change be in the release notes?

@sabey

This comment has been minimized.

Copy link
Author

commented Aug 25, 2017

ya I figure, it was a shock to me however to find that I now had access to it

@elico

This comment has been minimized.

Copy link

commented Aug 25, 2017

@ianlancetaylor sure! it should be documented and also mentioned in the release notes.
It's important for many web server's for stats or the ability to apply ACL's for services that apply different policies for different networks their clients have access from\to.
For example I have a service which face two different public ip addresses behind nat(each has it's own router) and I want to apply rate limiting only for connections coming on a specific internal ip address without running two different listening ports\services.
(the reverse path route consistency is being handled by iptables)

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

Here is the documentation for LocalAddrContextKey:

// LocalAddrContextKey is a context key. It can be used in
// HTTP handlers with context.WithValue to access the address
// the local address the connection arrived on.
// The associated value will be of type net.Addr.
LocalAddrContextKey = &contextKey{"local-addr"}

@sabey, that looks correct to me? #18686 fixed a bug where LocalAddrContextKey used the listen address instead of the actual accepted address (where they could differ if the listen address was a wildcard). The fixed behavior seems to match the above comment.

As to whether or not this belongs in the release notes, I don't feel strongly and am fine with adding it. It seems roughly at the same level as the ServeMux change called out in the release notes:
https://golang.org/doc/go1.9#net/http

@gopherbot

This comment has been minimized.

Copy link

commented Aug 28, 2017

Change https://golang.org/cl/59530 mentions this issue: doc/1.9: add mention of net/http.LocalAddrContextKey

@gopherbot gopherbot closed this in 75d7a02 Aug 28, 2017

@sabey

This comment has been minimized.

Copy link
Author

commented Aug 28, 2017

@tombergan looks correct

@gopherbot

This comment has been minimized.

Copy link

commented Aug 28, 2017

Change https://golang.org/cl/59670 mentions this issue: [release-branch.go1.9] doc/1.9: add mention of net/http.LocalAddrContextKey

gopherbot pushed a commit that referenced this issue Aug 28, 2017

[release-branch.go1.9] doc/1.9: add mention of net/http.LocalAddrCont…
…extKey

Fixes #21603

Change-Id: I42fb7ea2dd7f6d6a201171055beaeda68c26b823
Reviewed-on: https://go-review.googlesource.com/59530
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/59670
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 4, 2017

Change https://golang.org/cl/68232 mentions this issue: [release-branch.go1.9] doc/1.9: add mention of net/http.LocalAddrContextKey

gopherbot pushed a commit that referenced this issue Oct 4, 2017

[release-branch.go1.9] doc/1.9: add mention of net/http.LocalAddrCont…
…extKey

Fixes #21603

Reviewed-on: https://go-review.googlesource.com/59530
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/59670
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>

Change-Id: Ie9732d57948593dc0306a4a649664eedb3de370c
Reviewed-on: https://go-review.googlesource.com/68232
Reviewed-by: Chris Broadfoot <cbro@golang.org>

@golang golang locked and limited conversation to collaborators Oct 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.