Open MatthiasScholz opened 3 years ago
Thank you for the PR and apologies for the delay. We've been super buried, but will try to get to this as soon as we can! In the meantime, could you pull in the latest from master
?
Thanks for coming back to this issue!
Will integrate the last changes soonish. At the moment I try to evaluate how this can be properly tested.
The latest idea would be to check if the configuration file is properly generate. Having the configuration in place I would assume Consul will do the right thing. The purpose of the test - from my perspective - should not be testing the Consul implementation. This approach will as well ease the testing.
Will this be sufficient?
At the moment I try to evaluate how this can be properly tested.
The latest idea would be to check if the configuration file is properly generate. Having the configuration in place I would assume Consul will do the right thing. The purpose of the test - from my perspective - should not be testing the Consul implementation. This approach will as well ease the testing.
Will this be sufficient?
The approach we typically follow, and the one recommended with Terratest is to actually check the feature works, rather than whether the config looks like it would work. That gives us much more confidence in the code.
Is it possible, for example, to fire up some minimal "service" (e.g., Hello World web app), register it in the service mesh, and make a request to it, via the mesh, from another node? That would give us a lot of confidence it's all working.
It will take me some time to make all this happen.
I can understand the request. Just want to line out the effort to make this happen and explain why it could be nice for demoing Consul Connect, but quite a ask for testing.
To make Consul Connect work one need to add this into the server configuration:
connect {
enabled = true
}
The script provide has lot's of boilerplate code to add these 3 lines. A test should verify once the configuration option is given these lines are added and when they are not given the lines are not added.
Verifying, if Consul Connect works compares to checking if the implementation of HashiCorp regarding reading the configuration file works. I assume this is tested at their end.
It would be create if the Consul API provides a functionality to check if Consul Connect is activate, but I did not find any suitable call. Maybe I missed something.
Adding an integration test adds layers of complexity to make sure the networking is setup properly in AWS which is unrelated to the feature itself. For this feature it would mean:
Quite some effort in comparison of checking if a bash script generates the correct configuration file which would not even involve spinning up infrastructure in AWS and hence would have a shorter feedback cycle at a lower costs. A drawback I can currently see: it comes with a risk not being notified by the test when the configuration option changes.
Having said all this since the purpose of having Consul Connect configured is to activate the service mesh functionality. It would be great to have a demo covering all the steps lined out beforehand to show how to make use of it.
Agreed it's a fair bit of work, but without an example + test:
We already have plenty of examples that spin up clients and servers: https://github.com/hashicorp/terraform-aws-consul/tree/master/examples. Running a "Hello, World" web server is roughly 3 lines of code in a User Data script (example). So the only thing left is to register the web server in Consul Connect and make sure it is accessible.
Up! @robmorgan any chance you could review this PR ? This feature would be such a blast! Thanks!
Summary
It the seems the PR #173 is abounded.
Taking the work previously provided by @7hacker and adding the suggestion provide by @brikis98 plus fixing some minor issues regarding configuration parameter forwarding and test naming.
I am not completely sure how to test the functionality. The provided tests are just using the standard functionality checkConsulClusterIsWorking.
Relates to #165.