Add Support to customize the Host

[waelemara]

Hi, Can we add support to customize the host when we run pact-mock-service.
https://github.com/SEEK-Jobs/pact-net/blob/5fdaaf2eb87bcc1a39796408c1f1786a8679fd49/PactNet/Core/MockProviderHostConfig.cs#L14

I the prev class can we add an extra parameter "host" that we pass it down to pact-mock-service to fix this issue pact-foundation/pact-mock_service#79

I'm happy to send a PR.

Thanks

[neilcampbell]

@waelemara Hey! Yep sounds good to me. I think the reason I didn't add it, was it wasn't available at the time. I may have just missed it though.

A PR would be perfect. Let me know if you need anything.

[bethesque]

One thing to note is that the host that the mock service is bound on (eg. 0.0.0.0) is going to be different from the host it is contacted on (eg. 127.0.0.1). Perhaps a distinction should be made in the naming.

[waelemara]

@neilcampbell thanks, will raise a PR soon
@bethesque
I was going to change the constructor to accept a new parameter "host" with a default value "empty string" if the user did not pass it then I do not pass it and leave the pact library work with its defaults.

The name of the variable will be "host" do you have other suggestions?

[waelemara]

I went ahead with the option I mentioned before, It will not work because we pass these args to a script rather than calling the actual service. As @bethesque explained in this ticket pact-foundation/pact-mock_service#79
Not sure what is the next step that I can take right now.

Thanks

[neilcampbell]

@waelemara Sorry I don't understand. Are you able to elaborate? Are your talking about the pact-mock-service.sh script?

[waelemara]

@neilcampbell yes, the parameter needs to be passed to the pact-mock-service.sh, however, the solution I mentioned above is passing it to pact-mock-service.rb script.

[neilcampbell]

Why would it only be passed to the pact-mock-service.rb script?

[waelemara]

I think you are right after digging a little bit the process that is getting triggered is pact-mock-service.sh, the reason I thought it is only the pact-mock-service.rb is this error that I got after adding the host option

ERROR: "pact-mock-service.rb service" was called with arguments ["Website", "API"]
Usage: "pact-mock-service.rb service"

[neilcampbell]

@waelemara What version are you using? We fixed that issue last night. Try 2.0.21. See pact-foundation/pact-ruby-standalone#7 for details

[waelemara]

I was running an old version, just updated thanks.
@neilcampbell is running the integration tests enough, or there is another suggested testing approach?

[waelemara]

I have created an initial PR #123 to kick off the review process.

Thanks for your time today.

[neilcampbell]

@waelemara Added some comments to the PR

[waelemara]

@neilcampbell thanks for your comments, I have addressed them when you have the time it would be great to have a look at the changes.

And if you can advise on the approach of e2e test the changes that would be great.

Thanks

[neilcampbell]

@waelemara Merged. If all the tests pass, that is all that is needed. We run the samples on the build server, so that will pick up anything breaking changes.

[waelemara]

Awesome, let me know the results 👍 once it is ready.

[waelemara]

@neilcampbell, I have looked into the integration tests and found a place where I can test the code, and when I pass 0.0.0.0 it fails with the following error

Message: System.AggregateException : One or more errors occurred.
---- System.AggregateException : One or more errors occurred.
-------- System.Net.Http.HttpRequestException : An error occurred while sending the request.
------------ System.Net.WebException : Unable to connect to the remote server
---------------- System.Net.Sockets.SocketException : The requested address is not valid in its context 0.0.0.0:9222
---- The following constructor parameters did not have matching fixture data: ConsumerEventApiPact data

I reverted back to the original implementation I had where I pass the host to the RubyHost and this fixes it.

I think there is a difference between the base-url and host.

Host: is passed down to the pact-mock-service script so that the process can be accessible from outside the container
here is the difference between passing the host flag and not

Without
tcp 0 localhost:1234 0.0.0.0:* LISTEN
With passing --host=0.0.0.0 flag
tcp 0 0 0.0.0.0:1234 0.0.0.0:* LISTEN

baseUrl : is used to create an HttpClient in this class AdminHttpClient

[waelemara]

I will create another PR and let me know your thoughts. @neilcampbell

[neilcampbell]

@waelemara Oh right, I assumed you had already tested your changes.
Keep me posted.

[waelemara]

I will also add the Linux package into the Integration tests and test in a container just to be sure,
Stay tuned

[waelemara]

@neilcampbell I have finished testing on a Linux Container, and the pact server now can be accessed from outside the container. So it is safe now to merge the PR if you are happy with it.

as a side note, I was not able to update the tests to run on Linux because they are a full dotnet framework projects, so I had to create a test console application and ran it in the Linux container.

Thanks

[neilcampbell]

@waelemara I had a look at the change. There is one issue, what if the chosen host is something other than localhost (or variants of that), the AdminHttpClient will not be able to talk to the mock provider service, which is needed for everything to work.

Another option could be to revert back to using the Uri, however when passing it to the AdminHttpClient detect if it's 0.0.0.0, and change to localhost.

[neilcampbell]

Also can we make the default values for host null

[waelemara]

@neilcampbell from the docs of pact-mock-service this is what the host flag will do
-h, [--host=HOST] # Host on which to bind the service
# Default: localhost

And from the information that you just mentioned, exposing the host flag can cause issues, the consumer of the library has to know that when using localhost as a base url he needs to use 0.0.0.0 and if he/she uses different url they need to match.

I suggest that we do not expose this option and drive the it's value.

If the use did not customise the base url which means it will have a default value of localhost we default the host to 0.0.0.0 , if they customised the base url then we pass it's host to the pact-mock-service.

What do you think?

[neilcampbell]

And from the information that you just mentioned, exposing the host flag can cause issues, the consumer of the library has to know that when using localhost as a base url he needs to use 0.0.0.0 and if he/she uses different url they need to match.

I am a bit confused by what you mean here. Exposing the host flag is fine, we just need to make sure it works for any host that is provided and not just 0.0.0.0. I'm not suggesting the user needs to know this, it's something Pact-Net will detect and action appropriately.

If the use did not customise the base url which means it will have a default value of localhost we default the host to 0.0.0.0 , if they customised the base url then we pass it's host to the pact-mock-service.

Are you suggesting that the MockService call takes a uri rather than port, enableSsl and host? I think I would prefer to keep the API surface area compatible, so adding the host is more desirable IMO.

[waelemara]

My suggestion is to leave the API surface area intact, and drive the value of the host flag from the base URI that the user can pass to MockProviderServiceBaseUri , if the user passes localhost then we use 0.0.0.0 , if the user passes something else we use URI.Host to pass it down to pact-mock-service

Regarding what I mean in my prev comment,
If AdminHttpClient needs to be able to reach mock provider service, this means the baseURL and host values needs to be compatible, which can be a bit confusing for the consumer of the library.

So if we only expose one option in this case MockProviderServiceBaseUri and from that we drive the right values that we can pass to pact-mock-service.

[neilcampbell]

Where does the user pass a base URI to MockProviderServiceBaseUri? AFAIK the user can only pass a port on the current API surface area.

[bethesque]

Why not have a host and a bind host, as separate parameters? One is passed to the start up command, and the other is used by the client. The bind host can default to the host if not specified.

[neilcampbell]

Yeah could do, however I don't think we need to. If we get a host of 0.0.0.0, we use localhost for the admin http client calls. If we get anything else, we use what was sent as the host for both the bind and admin http client calls. I'm happy to get this working tomorrow.

[waelemara]

@neilcampbell and @bethesque, I have a suggestion, we can do something similar to ASP.NET core when it comes to configuring the IP address that the server can listen to
We can introduce an enum with two values for now IPAddress.Any and IPAddress.Loopback
IPAddress.Any option will pass down 0.0.0.0
IPAddress.Loopback will leave the underlying ruby package to its default.

[neilcampbell]

Is there any scenario where we want to bind a specific IP or hostname?

[waelemara]

To my knowledge no.

[waelemara]

@neilcampbell I'm happy to go ahead and do the changes you recommended and update the PR

[neilcampbell]

@bethesque Can you think of any scenario where the user would bing a specific IP address or hostname that isn't 0.0.0.0 or 127.0.0.2/localhost?

[bethesque]

No. I've never come across it.

[neilcampbell]

@waelemara Yeah I think your idea of the enum will work nicely. Go for it :D

[waelemara]

@neilcampbell can I pass this IPAddress flag to RubyHttpHost now? rather than driving it from baseUrl, they seem to be different things.

[neilcampbell]

Yep, sounds good.

[waelemara]

Changes made and PR ready for review.

[neilcampbell]

Merged and released in 2.1.1.
Also I changed listeningIpAddress back to host, so that it lines up with the ruby mock provider config.