net: Resolver.LookIPAddr() drops context values

[WinstonPrivacy]

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

v1.11.2

Does this issue reproduce with the latest release?

Yes

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

Linux, ARM64

What did you do?

Upgraded from Go 1.9.7 to 1.11.2. Functional and unit tests immediately threw a variety of errors relating to DNS lookups. This was isolated down to a routine in net/lookup.go/LookupIPAddr().

What did you expect to see?

We use WithValue to attach values to the context object which are then consumed by our custom DNS resolvers. Any values attached to the context object sent to this routine should be passed through it.

What did you see instead?

Context values are correctly sent to LookupIPAddr() but do not leave it. As a result, our custom DNS resolvers are misbehaving because they never see the expected values in the context object which is passed on to them.

It appears that a new context object is now being created using context.Background() so the passed in values are not retained.

func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]IPAddr, error) {

...

// We don't want a cancelation of ctx to affect the

// lookupGroup operation. Otherwise if our context gets

// canceled it might cause an error to be returned to a lookup

// using a completely different context.

fmt.Println("[DEBUG] net/lookup.go/LookupIPAddr() 3 - ctx", ctx, host)

lookupGroupCtx, lookupGroupCancel := context.WithCancel(context.Background())

[odeke-em]

Thank you for this report @WinstonPrivacy!

As the comment in

go/src/net/lookup.go

Lines 231 to 235 in ac277d9

	// We don't want a cancelation of ctx to affect the
	// lookupGroup operation. Otherwise if our context gets
	// canceled it might cause an error to be returned to a lookup
	// using a completely different context.
	lookupGroupCtx, lookupGroupCancel := context.WithCancel(context.Background())

states we are avoiding context cancelation.

I believe that the solution to this issue could benefit from @johanbrandhorst's #28279 and @rsc's recommendation #28279 (comment): since context.Context is an interface, just having a front facing context that falls back to the old context for value look up, but will return a fresh channel from Done which will solve the quagmire of avoiding cancelation

[WinstonPrivacy]

That sounds like an elegant solution.

…

On Wed, Nov 7, 2018 at 7:56 AM Emmanuel T Odeke ***@***.***> wrote: Thank you for this report @WinstonPrivacy <https://github.com/WinstonPrivacy>! As the comment in https://github.com/golang/go/blob/ac277d9234a89070a80dcbd03ef580566480fe42/src/net/lookup.go#L231-L235 states we are avoiding context cancelation. I believe that the solution to this issue could benefit from @johanbrandhorst <https://github.com/johanbrandhorst>'s #28279 <#28279> and @rsc <https://github.com/rsc>'s recommendation #28279 (comment) <#28279 (comment)>: since context.Context is an interface, just having a front facing context that falls back to the old context for value look up, but will return a fresh channel from Done which will solve the quagmire of avoiding cancelation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ai89gfA3sizKXFymgzf02i6Ud9QdCvu5ks5usuacgaJpZM4YOykG> .

[johanbrandhorst]

Interesting find and conclusions @odeke-em. I'm wondering now since the stdlib is plagued by this as well if the decision to exclude such an implementation from the context package should be revisited? Of course, a custom context implementation in net is the other solution.

[gopherbot]

Change https://golang.org/cl/148698 mentions this issue: net: LookupIPAddr should preserve context values for Resolver

[odeke-em]

I'm wondering now since the stdlib is plagued by this as well if the decision to exclude such an implementation from the context package should be revisited? Of course, a custom context implementation in net is the other solution.

@johanbrandhorst for now, I think we can present the custom implementation in net, it is a simple one that uses composition and falls back to the creating context for value lookups only.

type onlyValuesCtx struct {
   context.Context
   lookup context.Context
}

func (ovc *onlyValuesCtx) Value(key interface{}) interface{} { return ovc.lookup.Value(key) }
func WithOnlyValues(ctx context.Context) context.Context {
   return &onlyValuesCtx{Context: context.Background(), lookup: ctx}
}

In regards to adding it to the standard library I think if we can find another spot that needs this
context, you can then use that as a compelling reason to start a proposal with those concrete needs.

[ianlancetaylor]

@odeke-em has a fix but I'm concerned about the approach.

The newly created context can outlive the original context. This will happen if the original context has a timeout shorter than the time required for the DNS lookup. Canceling the original context may in some way invalidate the values. The result will be that the resolver code is looking at a context containing invalid values.

If there are multiple simultaneous lookups of a single IP address, and the lookups use different contexts with different values, then only one set of values will be passed to the DNS lookup. Whether this is correct or not depends on what those values mean.

So: @WinstonPrivacy what do the values in your contexts mean?

It's too bad there is no way to ask a context whether it contains any values at all. If there were such a way then we could avoid coalescing different lookups of the same IP address when using a context containing values.

One way to partially address the first problem by changing the CL to only return a value from the underlying context if it has not been canceled. But I don't know whether this is right or wrong.

[WinstonPrivacy]

We utilize contexts to determine which DNS servers to hit. Most queries will resolve to the local DNS service. However, we occasionally require upstream DNS servers to be queried directly. Context values are used to determine this.

…

On Fri, Nov 9, 2018 at 12:30 PM Ian Lance Taylor ***@***.***> wrote: @odeke-em <https://github.com/odeke-em> has a fix but I'm concerned about the approach. The newly created context can outlive the original context. This will happen if the original context has a timeout shorter than the time required for the DNS lookup. Canceling the original context may in some way invalidate the values. The result will be that the resolver code is looking at a context containing invalid values. If there are multiple simultaneous lookups of a single IP address, and the lookups use different contexts with different values, then only one set of values will be passed to the DNS lookup. Whether this is correct or not depends on what those values mean. So: @WinstonPrivacy <https://github.com/WinstonPrivacy> what do the values in your contexts mean? It's too bad there is no way to ask a context whether it contains any values at all. If there were such a way then we could avoid coalescing different lookups of the same IP address when using a context containing values. One way to partially address the first problem by changing the CL to only return a value from the underlying context if it has not been canceled. But I don't know whether this is right or wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ai89gc_Q9usMy9KXT5gCsMdlsYTnjgqAks5utcnVgaJpZM4YOykG> .

[ianlancetaylor]

@WinstonPrivacy Thanks. What is the correct behavior if goroutine 1 has a context that resolves to the local DNS service, and goroutine 2 has a context that queries the upstream DNS services directly, and goroutine 1 looks up 1.2.3.4, and, before that lookup completes, goroutine 2 also looks up 1.2.3.4?

[WinstonPrivacy]

That can't happen in our code because DNS resolutions are cached and are concurrency safe. So if goroutine1 is resolving DNS, goroutine2 will be blocked until it completes and then will use the cached value. DNS responses are cached for some TTL (10 minutes). Even if this were not the case, the impact would be minimal (you may end up with a conflicting IP address for the host). We protect against this but even if we didn't, the impact would be minimal. Perhaps it should be the responsibility of the caller to deal with this possibility?

…

On Fri, Nov 9, 2018 at 1:18 PM Ian Lance Taylor ***@***.***> wrote: @WinstonPrivacy <https://github.com/WinstonPrivacy> Thanks. What is the correct behavior if goroutine 1 has a context that resolves to the local DNS service, and goroutine 2 has a context that queries the upstream DNS services directly, and goroutine 1 looks up 1.2.3.4, and, before that lookup completes, goroutine 2 also looks up 1.2.3.4? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ai89gctUa0JxtoxQoq52qMkeg0Eq2solks5utdTvgaJpZM4YOykG> .

[ianlancetaylor]

Thanks. Making it the responsibility of the caller is fine but of course nobody will ever see the documentation for this, so it needs to behave reasonably regardless.

@odeke-em I think you should modify your CL so that after retrieving the key, if the context is canceled, we pretend that the key wasn't there. I don't see any fully correct approach that we can use, and that seems the most likely to do the right thing.

Happy to hear counter-arguments.

[odeke-em]

@odeke-em I think you should modify your CL so that after retrieving the key, if the context is canceled, we pretend that the key wasn't there. I don't see any fully correct approach that we can use, and that seems the most likely to do the right thing.

Thanks for the suggestion, raising and discussing the point about expiry! Sounds like a plan, I can definitely do that.

[WinstonPrivacy]

Fantastic! Can't wait to try it!

…

On Mon, Nov 12, 2018 at 5:37 PM GopherBot ***@***.***> wrote: Closed #28600 <#28600> via 5d39260 <5d39260> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ai89gWm_j-2J_Ipc-ieyKIv1GneOd0HAks5uugYugaJpZM4YOykG> .