Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added SMI test support #151

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dhruv0000
Copy link
Member

Description

This PR adds SMI Test support for consul mesh.

  • Added SMI test operation in the adapter.

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@dhruv0000 dhruv0000 marked this pull request as ready for review April 12, 2021 11:23
@leecalcote leecalcote requested review from a team, mgfeller, leecalcote and tangledbytes and removed request for a team April 12, 2021 14:43
@mgfeller
Copy link
Contributor

Does this fix #106 ?

@kumarabd
Copy link
Contributor

@dhruv0000 Did you get a chance to check #106. If not do ensure if this PR addresses the issue.

@dhruv0000
Copy link
Member Author

dhruv0000 commented Apr 12, 2021

@kumarabd @mgfeller I just reviewed the conversation. So for the first Issue,

  • Yes, Consul adapter is quite outdated, but so is SMI support for other SMI providers, like LinkerD and Traefik mesh . So it seems likely to go forward with the tests anyways and and show that they were failing the latest spec (partially).
  • For the issue of sidecar-proxy injection, I was intending to use connectInject.default: as true (ref) as a temporary solution until we have our own workflow engine up and running and rewrite the SMI-Conformance tool altogether. The configuration basically injects sidecar to all the pods automatically by default.

//cc @leecalcote

@mgfeller
Copy link
Contributor

@dhruv0000

  • the Consul SMI controller is quite outdated, it has neither been tested with Consul 1.8 (or later) nor a more recent version of Kubernetes as far as I know. On the other hand, the Meshery Consul adapter is quite up to date :-)
  • unless the SMI conformance test tool is redesigned, the sidecar will not be able to pick up the upstream service configuration. this is because the Consul sidecar proxy is not a transparent proxy and depends on annotations of the containers in the deployment descriptor. This requirement will be going away in Consul 1.10 with the introduction of transparent proxying. See the discussion here: https://docs.google.com/document/d/1HL8Sk7NSLLj-9PRqoHYVIGyU6fZxUQFotrxbmfFtjwc/edit?ts=603c4884

//cc @leecalcote @kumarabd

@leecalcote
Copy link
Member

// @nicholasjackson

@mgfeller
Copy link
Contributor

@dhruv0000

* the deployment descriptor.

... is part of the SMI testing tool and identical for all meshes, without the possibility to add annotations to the deployment spec.

@dhruv0000
Copy link
Member Author

dhruv0000 commented Apr 14, 2021

@mgfeller I see mistake in my assumption now. Even though we would be able to get the sidecar injected, we would still need to specify the upstream service in Pods annotations. At-least for Consul 1.8 and 1.9.

Since Consul 1.10 will support transparent proxy, Here is my proposal to support current versions for now,

  • Since we have the ability to define and pass a specific manifest file for each adapter to install the SMI-Tool, lets create a consul specific branch in learn-layer5, make consul 1.8/1.9 specific changes in YAML and publish the image with a tag like smi-v0.6.0-consul and use that in the consul manifest. (As a temp solution until v1.10 comes around)

//cc @leecalcote

@mgfeller
Copy link
Contributor

* Since we have the ability to define and pass a specific manifest file for each adapter to install the SMI-Tool, lets create a consul specific branch in learn-layer5, make consul 1.8/1.9 specific changes in YAML and publish the image with a tag like `smi-v0.6.0-consul` and use that in the consul manifest. (As a temp solution until v1.10 comes around)

@dhruv0000

Yes, that's what we considered earlier as well (not sure it is written down somewhere).
The alpha version of Consul 1.10 has been released almost a month ago, so I think it's worth waiting. (https://github.com/hashicorp/consul/releases). Afaik there are not corresponding Helm Chart releases for alpha / beta Consul releases.

@dhruv0000 dhruv0000 marked this pull request as draft July 28, 2021 08:34
@leecalcote
Copy link
Member

leecalcote commented Oct 7, 2021

@dhruv0000 where are we at with support for SMI Conformance against Consul?

@tangledbytes
Copy link

Are we still moving ahead with this PR?

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

Successfully merging this pull request may close these issues.

5 participants