nosinovacao / name-sdk

A dependencies management library designed to expose and access dependencies information.
BSD 3-Clause "New" or "Revised" License
14 stars 4 forks source link

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

Open Symbianx opened 7 years ago

Symbianx commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.