Add custom endpoint capability

[JohnStarich]

I've recently come across convoy and I really love it's simplicity! ❤️

I've also started using Minio for S3 backups, but unfortunately convoy didn't support custom endpoint URLs. This PR adds the ability to change the endpoint to one's own Minio S3 server, or any other.

~This solution uses an awkwardly named environment variable so that will likely need to be implemented differently.~(fixed) Additionally, I couldn't think of a good way to change the current s3:// URL parsing logic that would be backward-compatible.

I'm open to any suggestions/questions and happy to adapt this to your style 😄

Edit: Looks like this could resolve https://github.com/rancher/convoy/issues/46

[JohnStarich]

Here's my work thus far! (Looks like GitHub is in the middle of a partial outage or something, hence the build failure.)

I noticed some interesting CLI behavior, looks like the --s3-endpoint flag must be specified like convoy backup --s3-endpoint <URL> list and not convoy backup list --s3-endpoint <URL>. I suppose this makes sense from a CLI library perspective, but it's a little counter-intuitive.

I still need to work on the unit and integration tests. Thoughts on the changes so far?

[yasker]

Good work!

Yeah, that endpoint setting looks a bit weird. But since we've embedded the URL as a part of backup identity, it's maybe better in this way.

And can you rearrange the patches according to the functionality? Say enable s3_endpoint, and fix typo can be two commits, instead of 4. It's better to keep the commit logically sound and neat by itself.

Other than that, it looks good.

[JohnStarich]

Absolutely. I plan to rearrange the commits prior to final review to clean things up, but I've gone ahead and squashed it down a little bit.

Here are a couple updates, including some unit tests for s3.initFunc and the initial workings of the custom endpoint integration tests. I'm having some Python environment issues at the moment so these tests could take some time to get working 😛

[yasker]

In general, it looks good. Looking forward to the final version!

[JohnStarich]

Another update, since it's been a while 😅

These changes have some working tests for custom S3 endpoints now. Mainly it requires you have an empty S3 bucket available and the endpoint set in the endpoint environment variable.

Now I'm working on getting Minio setup automatically so these tests could run without any special instructions. 😄

[JohnStarich]

Aaaand there's the final bit ^ Now if all of the S3 env vars are not set, then it will automatically spin up a Minio S3 server and send all S3 requests there.

Edit: Also, I promise the commits are in a sensible order but you may need to pull it down locally to see that. GitHub is displaying them in a very strange order for some reason.

[JohnStarich]

After looking into the awscli endpoint URL support some more, it looks like my PR might be too tailored for Minio because I'm using .WithS3ForcePathStyle(true).

Do you guys have a way to test if this works okay with non-Minio/Amazon S3 servers? I may be able to force Minio to recognize the subdomain-style endpoint URLs, but I expect it could be a more complicated setup (which could potentially discourage new users). I'm tempted to just leave this sort of support for a later PR if somebody wants it. 🤷‍♂️

[yasker]

@JohnStarich Thanks for the update. Nice work!

1. Yeah, we have S3 for testing. I will test S3 before we merge.
2. Please continue to use gocheck (see e.g. s3/s3_service_test.go) instead of introducing another Go unit test framework (testify)
3. Yeah, the separation of commits mostly looks good. Though, can you separate integration test python script into a separate commit? Normally python changes should be separate from Go changes (since it shouldn't break the build). Unit test change should go with the functional change.

[JohnStarich]

1. Sounds good 👍
2. testify/assert only provides assertions for Go's testing.T rather than an entire framework, but I can definitely try and switch to gocheck's assertions. After some investigation, it looks like the switch will be non-trivial though. My tests make use of Go's built-in testing.T.Run which does not have an equivalent in gocheck. 😱 I'll see what I can do.
3. Sure thing. I initially had them separated and then opted for fewer commits 😆 I'll split 'em back out

[JohnStarich]

Okay, I've got the tests using gocheck and dropped the testify dependency. Unfortunately, now the s3 unit tests are quite gross (in my opinion) but they continue to cover 100% of initFuncWithConnectionCheck. (I really wish gocheck supported t.Run!)

Commits are split up now, as requested 👍

[yasker]

@JohnStarich The build failed. There was a known issue on the integration test but seems the unit test failed in this case. https://drone8.rancher.io/rancher/convoy/11/3

[JohnStarich]

@yasker Should be good to go now, thanks for pointing that out!

[yasker]

Merged, thanks!

[yasker]

@JohnStarich Can you add a few documents on how to use this new feature in Convoy README.md? Then I can make a new release and point the users to the new feature.

[JohnStarich]

You bet! Opened a new PR here: https://github.com/rancher/convoy/pull/229