make install operation compliant with SMP protos

[DelusionalOptimist]

Background We're trying to enforce SMP protos wherever possible in all the adapters so as to ensure there is only a single source of truth. This is really helpful for maintaining multiple clients efficiently (i.e Meshery UI and mesheryctl).

Current Behavior The adapter has 2 keys specifying Consul installation operation. https://github.com/meshery/meshery-consul/blob/beb7f5a3f101cf4641065ce4ad95d749b9f82d02/internal/config/keys.go#L19-L20

These are not compliant with SMP protos.

Desired Behavior We should prolly modify this operation name to be in accordance with Consul's SMP name https://github.com/service-mesh-performance/service-mesh-performance/blob/1de8c93d8cba4ba8c1120fe09b7bf6ce0aa48c83/spec/service_mesh.pb.go#L33

[DelusionalOptimist]

@mgfeller thoughts?

[mgfeller]

@DelusionalOptimist I had a look (finally) and compared to the Linkerd code. I see the point of using SMP protos, of course. Is there a design document or similar that documents how adapter.Operation and specifically its fields are intended to be used?

In the Consul adapter, templates and AdditionalProperties are different for each version. How would that be handled if following the Linkerd code example? I still think it should be possible to specify these details per version, imho it cannot be expected that these are in general the same for all versions of a specific service mesh, or that all service meshes follow this assumption.

From that example, I deduce that there should only be one operation to install different service mesh versions. Is my understanding correct?

[DelusionalOptimist]

@DelusionalOptimist I had a look (finally) and compared to the Linkerd code. I see the point of using SMP protos, of course. Is there a design document or similar that documents how adapter.Operation and specifically its fields are intended to be used?

@mgfeller there is the meshery-adapters spec (look whom I'm pointing to it XD) but I doubt its depth covers this.

In the Consul adapter, templates and AdditionalProperties are different for each version. How would that be handled if following the Linkerd code example? I still think it should be possible to specify these details per version, imho it cannot be expected that these are in general the same for all versions of a specific service mesh, or that all service meshes follow this assumption.

Hmmm... I see versions are hard coded. There is no way to install a version other than the two specified. We'll also need to support installing consul based on GH release tag.

I think we're mainly using additionalProperties for storing info on helm charts to use? If 1.8.2 is a special case, instead of storing the chart in a different template, can we fetch the chart from the helm repo and override some values (values.yaml) on the fly, i.e before applying it? (meshkit supports this I think)

From that example, I deduce that there should only be one operation to install different service mesh versions. Is my understanding correct?

In nutshell, yes. We shouldn't be hard coding versions anywhere.

ICYMI, in current state of adapters, OSM, Istio and some others, the installation operation supports supplying a custom version. However, this isn't still exposed to users and working on meshops to for this is not what we're planning to do right now. For now, we fetch the latest release tag from github and install that version. Also, we're moving away from using their CLIs as the primary method. Instead, we fetch the version specific chart from their helm repo and apply it to install the service mesh. We plan to present users the ability to specify their service mesh version using patterns. Let's try to bring in these abilities in this adapter as well as a step towards supporting service mesh patterns for Consul?

[mgfeller]

@DelusionalOptimist it got quite a bit clearer after today's meeting :+1:

v1.8.2 is not special, when I upgraded the adapter to support this version, there was no meshery-adapter-library, no meshkit, and thus no Helm-support in Meshery. v1.9.1 was implement when all of that was in place, and I didn't prioritize changing the installation method of v1.8.2 to Helm. Current Consul version is 1.10.

Namespace is handled explicitly and merged into templates on the fly, this was necessary due to how namespaces were used in the chart/manifest (as far as I can remember), maybe this has changed by now, I haven't had a chance to look at this.

I still think it cannot be excluded that we need to be able to set different variable values for different versions eventually, or override our default values, in a good way of course.

Also, it would be nice to have more detailed description of the operation attributes, how they are intended to be used, and the assumptions behind them. Note to self as well, of course :smile:

As discussed today, I'll have a look at the Istio adapter to gain more insight (might take a while, though). Of course, service mesh pattern support is desirable also for Consul.

Anyone is of course welcome to pick this issue.

[mgfeller]

Actually, some of my concerns from yesterday are addressed in this adapter, namely testing of new releases that are downloaded on the fly. There are BATS end-to-end tests here that can be leveraged to test e.g. the 2 latest releases. The workflow could be triggered by a cron job and run daily.

// @DelusionalOptimist @leecalcote @sayantan1413 @utkarsh-pro

[DelusionalOptimist]

@DelusionalOptimist it got quite a bit clearer after today's meeting

Nice!! Thanks for being on the call :raised_hands:

Also, it would be nice to have more detailed description of the operation attributes, how they are intended to be used, and the assumptions behind them. Note to self as well, of course smile

Yes it would. I'll make sure we have a section regarding this in the spec :smile:

As discussed today, I'll have a look at the Istio adapter to gain more insight (might take a while, though). Of course, service mesh pattern support is desirable also for Consul.

Sounds good : )

[DelusionalOptimist]

Actually, some of my concerns from yesterday are addressed in this adapter, namely testing of new releases that are downloaded on the fly. There are BATS end-to-end tests here that can be leveraged to test e.g. the 2 latest releases. The workflow could be triggered by a cron job and run daily.

Yes @mgfeller, I agree. Testing adapters is something we need to lay more emphasis on as we add new functionality.

Other than running tests daily, one thing to do at build time might be modifying the workflow which generates components to also take care of running tests to ensure that any of those generated components are not faulty and if so, they're not committed.

Other than this we would also need to be able to validate that the components for older versions, added in bulk in PRs like https://github.com/meshery/meshery-istio/pull/266 are accurate.

A question though. Will we also need to validate the components that the user fetches during run-time? If so, would the mechanism of validation will be testing using BATS in that case as well?

[mgfeller]

@DelusionalOptimist I think some sort of validation would be very useful, not quite sure which ones, though. By "components the user fetches during runtime" do you refer to service mesh versions that have not been baked into the adapter at build time?

BATS deploys Consul mesh and applications to a cluster (KinD in the GH workflow) so that would not be suitable for runtime checks.

If these BATS tests are run daily and test the latest versions (two, for instance, or all since the last time the adapter was built) then there is potentially only a small window where a user could fetch a newer version that hasn't been tested yet.

[DelusionalOptimist]

By "components the user fetches during runtime" do you refer to service mesh versions that have not been baked into the adapter at build time?

Yes, precisely.

BATS deploys Consul mesh and applications to a cluster (KinD in the GH workflow) so that would not be suitable for runtime checks.

If these BATS tests are run daily and test the latest versions (two, for instance, or all since the last time the adapter was built) then there is potentially only a small window where a user could fetch a newer version that hasn't been tested yet.

Ooh... yes, makes much sense :heavy_check_mark: