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

NAME port is not sent to the Registry in non selfhost environments #7

Open
Symbianx opened this issue Jul 19, 2017 · 5 comments
Open

Comments

@Symbianx
Copy link
Contributor

Symbianx commented Jul 19, 2017

As is, the port in which NAME is listening is sent to the Registry as null when using the WebApi and AspNetCore packages.

Getting the port is not trivial, and some discussion may be needed.

For apps hosted in IIS we can use the Microsoft.Web.Administration namespace to get this information, so at least the NAME.WebApi package should send the port.

@Symbianx Symbianx added the bug label Jul 19, 2017
@Symbianx Symbianx added this to the 1.0.1 milestone Jul 19, 2017
@Symbianx
Copy link
Contributor Author

Symbianx commented Aug 7, 2017

@suvl, after some discussion with @ctorrao we came to the conclusion that a better approach may be to have a KnownAddresses list which would be sent to the Registry in the Ping stage. This list would be populated with the port and possibly the IP from the requested URL, effectively using the information that the client used to make the request to the application.

Because, even if we went for the IIS approach, there may be more WebSites hosting the application, and in OWIN/Asp.Net Core self-hosted scenarios it would be impossible to know the port, making it inconsistent.

This would, of course, be more complex, and probably move this to the 1.1.0 milestone.

What's your toughts?

@suvl
Copy link
Member

suvl commented Aug 10, 2017

@Symbianx you are proposing to maintain an ISet<> of known addresses, built from capturing the HttpContext.Request on every request and check hostnames, ports, whatnot for novel addresses?

I know it is perfect from the NAME Registry requirements' point of view. I'm concerned about the possible impact on the pipeline we cannot foresee. Shall we first measure the impact of such an interception? Is there any way it can be offloaded or maybe a way to O(1) detect what does not require processing?

Plus, from that point on, the Registry would have to support multi-addresses, maybe?
/cc @ctorrao

@Symbianx
Copy link
Contributor Author

Symbianx commented Aug 10, 2017

The performance impact is a great point, regarding the collection used we could use an HashSet<T> implementation which provides O(1) for both manipulation AND lookup.
To improve performance further we could store the strings containing the full left part of the URL, without any transformations.

Also, we can consider only taking into the account the URLs of the request against the /manifest endpoint, which would not create an impact on the existing app flows, but could reduce the number of 'known addresses'.

Edit:

Yes, we could improve the Central Registration protocol to support the 'Known Addresses' flow. Keeping the existant 'Port' field for the NAME self-hosted solution that knows the specific port on which it is listening, or we could deprecate the Port field.

@suvl
Copy link
Member

suvl commented Aug 10, 2017

I am not in favor of solely consider the /manifest requests. In a real-world scenario with potentially hundreds of services in a massive microservice thingie, one would use the Registry to find out the /manifest url to the service, not the other way around. Is this a classic chicken and the egg thing?

We must run some numbers to assess real impact from this. There's already a fragment of impact from adding the X-Name header, maybe we could use exactly that pipeline execution step? (afraid to break SOLID here)

About the protocol, I think it should move to version 2 with support for Known Addresses and deprecation of Port.

@Symbianx
Copy link
Contributor Author

I'm not too fond of considering only the /manifest request as well, so let's keep that option out for now.

Yes you can use the same extension point, and I agree that we need to assess the performance impact it will have.

I agree with the protocol.

@suvl suvl modified the milestones: 1.0.1, 1.1.0 Oct 27, 2017
@Symbianx Symbianx modified the milestones: 1.1.0, 1.2.0 Nov 8, 2017
@Symbianx Symbianx removed this from the 1.2.0 milestone Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants