Fix port/targetport fallback logic in proxyless path

[davidebbo]

Fixes #4225

Microsoft Reviewers: Open in CodeFlow

[karolz-ms]

I am not sure about this change... this is going to result in pretty weird behavior for a settable property:

var e = new EndpointAnnotation(ProtocolType.Tcp, port: 1234, targetPort: null, isProxied: false);
Console.WriteLine(e.TargetPort); // prints 1234
e.TargetPort = 4567;
Console.WriteLine(e.TargetPort); // prints 4567
e.TargetPort = null;
Console.WriteLine(e.TargetPort); // prints 1234
e.IsProxied = true;
Console.WriteLine(e.TargetPort); // prints null

[davidfowl]

I was thinking the same thing. This is basically a way to get torn state. Maybe we need a gross internal method that used in the WithEndpoint callback...

[davidebbo]

Internal method was my first thought, but it felt a bit dirty to have logic outside of EndpointAnnotation. And it opens the possibility that some new scenario creates an EndpointAnnotation and forgets to call it, leading to inconsistent fallback behavior.

An alternative is to add _portSettingCalled/_targetPortSettingCalled fields to disable the fallback if the setter was ever called.

Let me know which way you prefer, I can do either.

[davidebbo]

I made a change along those lines. It's a bit more convoluted than I would have liked, but maybe still better than an internal method called from the outside. I also extended the tests to cover setting to null. PTAL.

[karolz-ms]

I can live with this, thanks @davidebbo

[mitchdenny]

I don't think we can easily change this now, but one alternative we could have taken here is have ProxiedEndpointAnnotation and EndpointAnnotation since they can have slightly different shapes and then use the type system to define the behavior.

[mitchdenny]

Its ugly but I can't think of a way that is necessarily better.

[davidebbo]

I don't think we can easily change this now, but one alternative we could have taken here is have ProxiedEndpointAnnotation and EndpointAnnotation since they can have slightly different shapes and then use the type system to define the behavior.

That would mean not allowing IsProxied to be settable, though that would probably not be a bad thing. It seems strange to allow it to change on a given instance. Anyway, as you say it seems too drastic a change now.

[davidebbo]

So it turns out that this change breaks the test ProxylessContainerWithoutPortThrows. This test has:

        var redis = builder.AddRedis("redis").WithEndpoint("tcp", endpoint =>
        {
            endpoint.IsProxied = false;
        });

Note that it sets a TargetPort in AddRedis, but no Port. With the old logic, the fallback from TargetPort to Port would not happen, because IsProxied is set later. With the new logic, the fallback happens, causing the test to not throw, and hence fail.

My take is that the new behavior is more correct, and that we should just adjust the test to have:

        var redis = builder.AddRedis("redis").WithEndpoint("tcp", endpoint =>
        {
            endpoint.IsProxied = false;
            endpoint.Port = null;
        });

Since clearly the intent of the test is to have no Port.

@davidfowl @karolz-ms @mitchdenny do you agree?

Also, kind of concerning that this was not caught earlier. I guess this test doesn't really run anywhere in an automated way?

[davidfowl]

Also, kind of concerning that this was not caught earlier. I guess this test doesn't really run anywhere in an automated way?

This is what we're working on 😄.

[davidfowl]

My take is that the new behavior is more correct, and that we should just adjust the test to have:

This sounds about right.