Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Resurrection of #173 - Consul Service Mesh #202

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

MatthiasScholz
Copy link

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.

7hacker and others added 30 commits May 14, 2020 11:04
Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
@brikis98
Copy link
Collaborator

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?

@MatthiasScholz
Copy link
Author

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?

@brikis98
Copy link
Collaborator

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.

@MatthiasScholz
Copy link
Author

MatthiasScholz commented Apr 16, 2021

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:

  • setting up base infrastructure with Consul server(s)
  • deploying two instances with Consul running as clients
  • deploying two test services on the clients and register them at the server
  • configuring the intention to allow communication between the services
  • send a request to one of the test services and check the response is including the second service

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.

@brikis98
Copy link
Collaborator

Agreed it's a fair bit of work, but without an example + test:

  1. It'll be much tougher for users to figure out how to make Consul Connect work.
  2. It'll be more or less impossible for maintainers to have confidence Consul Connect actually works. I'm sure Consul code is tested on their side, but without an integration test, we can't know if (a) we generated the config correctly, (b) if the config was put in the right place for Consul to use it, (c) if we opened up the proper ports for the Service Mesh communication, (d) and about a dozen other things.

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.

@Nielsoux
Copy link

Up!
@robmorgan any chance you could review this PR ?
This feature would be such a blast!
Thanks!

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants