-
Notifications
You must be signed in to change notification settings - Fork 319
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
Fix port/targetport fallback logic in proxyless path #4226
Conversation
// It also depends on what the EndpointAnnotation is applied to. | ||
// In the Container case the TargetPort is the port that the process listens on inside the container, | ||
// and the Port is the host interface port, so it is fine for them to be different. | ||
get => _port ?? (IsProxied ? null : _targetPort); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with this, thanks @davidebbo
I don't think we can easily change this now, but one alternative we could have taken here is have |
Its ugly but I can't think of a way that is necessarily better. |
That would mean not allowing |
* Fix port/targetport fallback logic in proxyless path Fixes dotnet#4225 * Add tests for port/targetport logic * Allow caller to set Port/TargetPort to null * Make the tests cover more scenarios
So it turns out that this change breaks the test var redis = builder.AddRedis("redis").WithEndpoint("tcp", endpoint =>
{
endpoint.IsProxied = false;
}); Note that it sets a TargetPort in My take is that the new behavior is more correct, and that we should just adjust the test to have:
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? |
This is what we're working on 😄. |
This sounds about right. |
Fixes #4225
Microsoft Reviewers: Open in CodeFlow