[MM-61277] Allow selecting between private and public connection types

[fmartingr]

Summary

Adds a new ConnectionType deployer configuration option to allow the user to choose between using public/private IPs and DNS. This is necessary when a load test is deployed under private subnets.

Ticket Link

https://mattermost.atlassian.net/browse/MM-61277

[fmartingr]

@agnivade @agarciamontoro Can you guys take a look at this and let me know if the approach makes sense?

[agarciamontoro]

Self-requested a review so I don't forget!

[agnivade]

Let's make these private fields to prevent mistakes of accessing them in the future.

[agnivade]

I realized that making these fields private would stop JSON unmarshal. In that case, I'd suggest to have a separate struct to unmarshal InstanceToParse and then create a new Instance by calling the Set methods.

[fmartingr]

I don't think this is necessary, there may be situations where we want to use the private or public ips directly. For example for intra-server communication we favor Private IPs over Public ones. One example is on the proxy IP in the getServerURL method. I'm currently reviewing usages and making sure I'm not breaking the services. Will re-request review once done and we can discuss in there, there is some tooling that I don't know how it would behave with this change.

[agnivade]

For example for intra-server communication we favor Private IPs over Public ones. One example is on the proxy IP in the getServerURL method. I'm currently reviewing usages and making sure I'm not breaking the services.

Ok, this is tricky. I'd prefer to have clear APIs that cannot be misused.

[agarciamontoro]

Thanks for this! The approach makes sense to me. Left a couple of comments below.

[agarciamontoro]

Looks good to me now! Just a few nits below and a typo. I haven't tested this, but if you want me to, I can give it a try before merging.

[fmartingr]

Looks good to me now! Just a few nits below and a typo. I haven't tested this, but if you want me to, I can give it a try before merging.

Hey, I made two small tests myself, but if you want to check it out just in case I missed something I'm not stopping you. The more 👀 the better.

[agnivade]

Given that we still have the fields exposed, and needed in cases for private communication, I am still a bit concerned that this highly increases the chances of a mistake happening in the future.

Anything we can do to prevent the user making this mistake, or atleast guide them in the right direction?

[fmartingr]

Given that we still have the fields exposed, and needed in cases for private communication, I am still a bit concerned that this highly increases the chances of a mistake happening in the future.

Anything we can do to prevent the user making this mistake, or atleast guide them in the right direction?

By that you mean adding a few docstrings to Instance or some change in the code?

[agnivade]

I was thinking of some changes in the code. But if adding docs is the best we can do, then let's do that atleast.

[fmartingr]

I was thinking of some changes in the code. But if adding docs is the best we can do, then let's do that atleast.

Don't know how to properly solve that at the moment and I don't want this PR to become bigger, added some comments, please check them out: 2ba1e8f

We can discuss further modifications for this on a follow up effort.

[agarciamontoro]

Tested this using private first, then re-running deployment create after having changed it to public and all looks good to me! Thanks, @fmartingr :)

[agnivade]

Looks good. Sorry this took longer than usual. I fear that the tool is becoming too hard to maintain because of the need to support so many options.