Migrate from plain manifests to helm charts in mesheryctl #4318

panyuenlau · 2021-10-03T17:12:14Z

Description

This PR fixes the helm chart part of #4103
(The mesheryctl changes will be included in another PR to separate from the helm chart changes)

Notes for Reviewers
The current PR only fixes the existing issues related to serviceAccount, clusterRole, and clsuterRoleBInding within the helm charts, I've tested the basic installations for each adapter but need some help on testing the other functionalities

Note: all the adapters should just use the service account meshery-server to have the permission to perform certain operations, but some of the adapters had their own service accounts created in the helm charts

Signed commits

Yes, I signed my commits.

~~>TODO: will be adding changes to the mesheryctl implementation to use the helm chart after making sure the charts are functioning properly~~

Signed-off-by: Pan Yuen Lau <panyuenlau@gmail.com>

codecov-commenter · 2021-10-03T17:15:47Z

Codecov Report

Merging #4318 (e4c6cd7) into master (8c224b9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4318   +/-   ##
=======================================
  Coverage   21.36%   21.36%           
=======================================
  Files          59       59           
  Lines        5046     5046           
=======================================
  Hits         1078     1078           
  Misses       3653     3653           
  Partials      315      315

Flag	Coverage Δ
e2etests	`∅ <ø> (?)`
unittests	`21.36% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c224b9...e4c6cd7. Read the comment docs.

Aisuko

Thanks, @panyuenlau, so cleanly work. Please add the code comment to the ClusterRole in order to clearly describe the permission we need to apply.

Aisuko · 2021-10-04T10:23:46Z

install/kubernetes/helm/meshery/charts/meshery-operator/templates/clusterrole.yaml

+  - meshery.layer5.io
+  resources:
+  - brokers/status
+  verbs:


Please give any code comment for apiGroups' verbs if could what I mean we should sure which permissions we need to get.

Also, should the templates/NOTES.txt be modified to capture the recent changes?

@Aisuko @hexxdump TBH, I am not familiar enough with the system to give a good explanation on why we need those permissions for the cluster role, I made the changes just based on the plain yaml manifests that we're currently using... reference: https://github.com/meshery/meshery-operator/blob/master/config/manifests/default.yaml#L273-L288

Good discussion. Given that the scope of these verbs is restricted to meshery.layer5.io resource groups, we shouldn't see any pushback from users given that the Meshery Operator should have full control over the lifecycle of its custom controllers.

install/kubernetes/helm/meshery/charts/meshery-consul/values.yaml

Aisuko · 2021-10-04T10:51:30Z

Hi, @navendu-pottekkat, I remember the CI check was only triggered by the specific folder, right? Is that strategy keep working well? I saw there trigger so many CI includes UI, DOC, golangci-lint. please help us check the strategy if possible.

s1ntaxe770r · 2021-10-04T13:08:27Z

install/kubernetes/helm/meshery/charts/meshery-cpx/values.yaml

@@ -11,10 +11,10 @@ image:
 env: {}

 probe:
-  #TODO: Need to confirm the health check path of meshery. 
+  #TODO: Need to confirm the health check path of meshery.


@navendu-pottekkat what's Meshery's health check path?

There is no specific health-check path but we can try hitting the version endpoint to get the surety that meshery is running properly.

A good item to create a new issue for follow up here.

hexxdump · 2021-10-04T15:12:16Z

Description

This PR fixes #4103

Notes for Reviewers The current PR only fixes the existing issues related to serviceAccount, clusterRole, and clsuterRoleBInding within the helm charts, I've tested the basic installations for each adapter but need some help on testing the other functionalities

Note: all the adapters should just use the service account meshery-server to have the permission to perform certain operations, but some of the adapters had their own service accounts created in the helm charts

Signed commits

Yes, I signed my commits.

TODO: will be adding changes to the mesheryctl implementation to use the helm chart after making sure the charts are functioning properly

I feel, the mesheryctl changes(to move away from manifests) should be handled through a separate PR

hexxdump · 2021-10-04T15:31:25Z

install/kubernetes/helm/meshery/values.yaml

+meshery-nginx-sm:
+  enabled: true
+  fullnameOverride: meshery-nginx-sm
+


@leecalcote, we do not have helm charts for meshery-nsm, meshery-tanzu-sm and meshery-app-mesh yet. Is there some limitation for these service meshes?

@hexxdump I believe we do have the meshery-nsm helm chart: https://github.com/meshery/meshery/tree/master/install/kubernetes/helm/meshery/charts/meshery-nsm

But not the other two that you mentioned

Good question and answer. The other two adapters are alpha stage adapters, and so, charts haven't been created yet. We can go ahead and create those charts now, so long as we don't include their deployment by default.

panyuenlau · 2021-10-04T17:39:05Z

Description
This PR fixes #4103
Notes for Reviewers The current PR only fixes the existing issues related to serviceAccount, clusterRole, and clsuterRoleBInding within the helm charts, I've tested the basic installations for each adapter but need some help on testing the other functionalities

Note: all the adapters should just use the service account meshery-server to have the permission to perform certain operations, but some of the adapters had their own service accounts created in the helm charts

Signed commits

Yes, I signed my commits.

TODO: will be adding changes to the mesheryctl implementation to use the helm chart after making sure the charts are functioning properly

I feel, the mesheryctl changes(to move away from manifests) should be handled through a separate PR

I agree, since separating the two into different PRs would help us identify potential issues more easily

…level chart Signed-off-by: Pan Yuen Lau <panyuenlau@gmail.com>

leecalcote

@panyuenlau confirming: with this update, two service accounts will be created: meshery-server and meshery-operator?

leecalcote · 2021-10-06T17:46:41Z

install/kubernetes/helm/meshery/charts/meshery-nginx-sm/values.yaml

@@ -49,7 +52,7 @@ securityContext: {}
 service:
  type: ClusterIP
  port: 10010
-  targetPOrt: 10010


leecalcote · 2021-10-06T17:49:28Z

install/kubernetes/helm/meshery/charts/meshery-operator/templates/clusterrole.yaml

+  - meshery.layer5.io
+  resources:
+  - brokers/status
+  verbs:


Good discussion. Given that the scope of these verbs is restricted to meshery.layer5.io resource groups, we shouldn't see any pushback from users given that the Meshery Operator should have full control over the lifecycle of its custom controllers.

leecalcote

Unless I've missed something, this is a great step forward.

panyuenlau · 2021-10-06T18:56:11Z

@panyuenlau confirming: with this update, two service accounts will be created: meshery-server and meshery-operator?

@leecalcote correct, the meshery and all the adapters should share the meshery-server service account in order to use the UI to manage the corresponding service mesh; and meshery-operator service account is for the meshery-operator pod

panyuenlau added 2 commits October 3, 2021 11:04

Fix NSM adapter and meshery-operator clusterrole

061f88f

Signed-off-by: Pan Yuen Lau <panyuenlau@gmail.com>

Fix permission related issues for all the other adapters

97d23ab

Signed-off-by: Pan Yuen Lau <panyuenlau@gmail.com>

panyuenlau changed the title ~~Mesheryctl helm~~ Migrate from plain manifests to helm charts in mesheryctl Oct 3, 2021

pottekkat requested review from a team October 3, 2021 17:19

Aisuko requested changes Oct 4, 2021

View reviewed changes

Aisuko reviewed Oct 4, 2021

View reviewed changes

install/kubernetes/helm/meshery/charts/meshery-consul/values.yaml Outdated Show resolved Hide resolved

s1ntaxe770r reviewed Oct 4, 2021

View reviewed changes

hexxdump reviewed Oct 4, 2021

View reviewed changes

leecalcote added area/devops area/lifecycle Lifecycle management (install, uninstall, configure) related labels Oct 5, 2021

Make service account name for each sub-chart modifiable from the top-…

44feb0b

…level chart Signed-off-by: Pan Yuen Lau <panyuenlau@gmail.com>

panyuenlau requested a review from Aisuko October 6, 2021 02:06

panyuenlau marked this pull request as ready for review October 6, 2021 12:58

Merge branch 'master' into mesheryctl-helm

e4c6cd7

leecalcote reviewed Oct 6, 2021

View reviewed changes

leecalcote approved these changes Oct 6, 2021

View reviewed changes

leecalcote merged commit 64038fe into meshery:master Oct 6, 2021

panyuenlau deleted the mesheryctl-helm branch October 8, 2021 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from plain manifests to helm charts in mesheryctl #4318

Migrate from plain manifests to helm charts in mesheryctl #4318

panyuenlau commented Oct 3, 2021 •

edited

codecov-commenter commented Oct 3, 2021 •

edited

Aisuko left a comment

Aisuko Oct 4, 2021

hexxdump Oct 4, 2021

panyuenlau Oct 6, 2021

leecalcote Oct 6, 2021

Aisuko commented Oct 4, 2021

s1ntaxe770r Oct 4, 2021

piyushsingariya Oct 5, 2021

leecalcote Oct 6, 2021

hexxdump commented Oct 4, 2021

hexxdump Oct 4, 2021

panyuenlau Oct 6, 2021 •

edited

leecalcote Oct 6, 2021

panyuenlau commented Oct 4, 2021

leecalcote left a comment

leecalcote Oct 6, 2021

leecalcote Oct 6, 2021

leecalcote left a comment

panyuenlau commented Oct 6, 2021 •

edited

Migrate from plain manifests to helm charts in mesheryctl #4318

Migrate from plain manifests to helm charts in mesheryctl #4318

Conversation

panyuenlau commented Oct 3, 2021 • edited

codecov-commenter commented Oct 3, 2021 • edited

Codecov Report

Aisuko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aisuko commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexxdump commented Oct 4, 2021

Choose a reason for hiding this comment

panyuenlau Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panyuenlau commented Oct 4, 2021

leecalcote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leecalcote left a comment

Choose a reason for hiding this comment

panyuenlau commented Oct 6, 2021 • edited

panyuenlau commented Oct 3, 2021 •

edited

codecov-commenter commented Oct 3, 2021 •

edited

panyuenlau Oct 6, 2021 •

edited

panyuenlau commented Oct 6, 2021 •

edited