Meshync Dynamic Configuration from Meshsync #258

KiptoonKipkurui · 2023-10-11T08:33:19Z

Description
This PR updates Meshsync to receive configuration for Meshsync CRD
This PR fixes #257

Notes for Reviewers

Signed commits

Yes, I signed my commits.

CRD Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

meshsync crd Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

CRD if available Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

codecov · 2023-10-11T08:39:17Z

Codecov Report

Attention: 56 lines in your changes are missing coverage. Please review.

Comparison is base (dbcef1e) 2.77% compared to head (988de5b) 11.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage    2.77%   11.53%   +8.76%     
==========================================
  Files           6        9       +3     
  Lines         397      529     +132     
==========================================
+ Hits           11       61      +50     
- Misses        385      461      +76     
- Partials        1        7       +6

Flag	Coverage Δ
unittests	`11.53% <47.16%> (+8.76%)`	⬆️

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

Files	Coverage Δ
internal/config/crd_config.go	`47.16% <47.16%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Aisuko

Thanks for your contributing. We need a test case here.

Aisuko · 2023-10-14T00:54:14Z

internal/config/crd_config.go

+var (
+	namespace = "meshery"           // Namespace for the Custom Resource
+	crName    = "meshery-meshsync"  // Name of the custom resource
+	version   = "v1alpha1"          // Version of the Custom Resource


Maybe you can import it rather than hard coded it https://github.com/meshery/meshery-operator/blob/master/api/v1alpha1/meshsync_types.go. Same like metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

I will update meshsync with this change immediately the meshery operator is approved with the CRD changes

internal/config/crd_config.go

internal/config/types.go

Aisuko · 2023-10-14T01:28:48Z

main.go

+	crdConfigs, err := config.GetMeshsyncCRDConfigs()
+
+	if err != nil {
+		// no configs found from meshsync CRD log warning


I am not sure the log level should be warning level, please get confirm from @leecalcote @theBeginner86. I assume that if we cannot get configs, it should be the Info level? Because we have the default configuraiton.

The choice of using a warning log is because the desired state of configuration for Meshsync from now and in future lies is utilizing the crd as per conversations with @leecalcote and @theBeginner86 so this is an extra speed bump to notify one that the desired approach has not been utilized

Co-authored-by: Aisuko <urakiny@gmail.com> Signed-off-by: Kiptoon Kipkurui <50325211+KiptoonKipkurui@users.noreply.github.com>

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

KiptoonKipkurui · 2023-10-23T20:10:32Z

@Aisuko sorry for the delayed resolutions, I was unwell, I am now better and will work on the proposed changes

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

leecalcote · 2023-10-26T18:27:58Z

Some notes from today's Meshery Build and Release meeting:

A new attribute inside of the meshsync CRD on the order of something like watchList, which is configurable with both the ability to explicitly name Resources and Event types of interest and explicitly those that are not.

My intuition leans into suggesting that this positive/negative tuple is ideally/conveniently defined in a ConfigMap, which is assigned to the watchList attribute.

Clarification of the terms that I've just used:

positive:whitelist
negative:blacklist
tuple:{"ResourceName":"EventType"}

The recording of today's discussion is an excellent reference - https://discuss.layer5.io/t/meshery-build-release-meeting-oct-26th-2023/4103/3

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

publishing item Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

listeners before using them Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

MUzairS15

In case you missed.
meshery/meshery-operator#533 (review)

KiptoonKipkurui · 2023-11-01T14:15:17Z

Some notes from today's Meshery Build and Release meeting:

A new attribute inside of the meshsync CRD on the order of something like watchList, which is configurable with both the ability to explicitly name Resources and Event types of interest and explicitly those that are not.

My intuition leans into suggesting that this positive/negative tuple is ideally/conveniently defined in a ConfigMap, which is assigned to the watchList attribute.

Clarification of the terms that I've just used:
* positive:whitelist

* negative:blacklist

* tuple:`{"ResourceName":"EventType"}`
The recording of today's discussion is an excellent reference - https://discuss.layer5.io/t/meshery-build-release-meeting-oct-26th-2023/4103/3

@MUzairS15 I did not miss your review, it was discussed extensively in the above quoted recording , and what I have pushed is an update from the meeting

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

from main to enable smoother unit testing Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

MUzairS15

@KiptoonKipkurui How we handle conflicting values in WhiteList and BlackList.
If one has priority over other, do mention it.

Looks good overall.

MUzairS15 · 2023-11-07T16:32:18Z

internal/config/crd_config.go

+		// Handle listeners
+		listerners := make(ListenerConfigs, 0)
+		for _, v := range Listeners {
+			if idx := slices.IndexFunc(meshsyncConfig.BlackList, func(c string) bool { return c == v.Name }); idx != -1 {


Suggested change

if idx := slices.IndexFunc(meshsyncConfig.BlackList, func(c string) bool { return c == v.Name }); idx != -1 {

if idx := slices.IndexFunc(meshsyncConfig.BlackList, func(c string) bool { return c == v.Name }); idx == -1 {

== I believe

Resolved by removing listeners till further conversations

KiptoonKipkurui · 2023-11-07T17:24:40Z

@KiptoonKipkurui How we handle conflicting values in WhiteList and BlackList. If one has priority over other, do mention it.

Looks good overall.

@MUzairS15 Ideally as a start we support one or the other, so we either blacklist resources(allowing the rest) or whitelist a few and ignore the rest. Then this validation can be done when creating the CRD, thoughts?.

MUzairS15 · 2023-11-07T18:28:45Z

@KiptoonKipkurui How we handle conflicting values in WhiteList and BlackList. If one has priority over other, do mention it.
Looks good overall.

@MUzairS15 Ideally as a start we support one or the other, so we either blacklist resources(allowing the rest) or whitelist a few and ignore the rest. Then this validation can be done when creating the CRD, thoughts?.

Sounds good either way.

Then this validation can be done when creating the CRD,
Only when creating the CRD? Since, the resources would be sourced from ConfigMap, shouldn't we validate at that instant as well? (You can skip the validation part in this PR, so that we can get going with these changes atleast).

MUzairS15

Merge Conflicts and a comment to resolve.

KiptoonKipkurui · 2023-11-07T18:48:20Z

Merge Conflicts and a comment to resolve.

@MUzairS15 Finalizing on the fixes, then I think we should have another conversation for the ListenerConfigs, some of the values are part of the code case in point and also we have ListenerConfigs as in Meshync but mapping to broker.RequestEntity types, Ill explain, from switch case, we are using the switch case on a broker entity type, but mapping to a Listener Config. So for now I would like to remove handling ListenerConfigs until we have a more conversations on how best to handle them

MUzairS15 · 2023-11-07T18:51:45Z

Also, we should look for getting reid of Pipelien configs defined i.e. In this PR we are using blacklist & whitelst and comapring with the CR config based on that watch for the resources.

As Lee also mentioned, we can externalise this configiguration in a ConfigMap. Whenever the configmap is updated we would reconcile the CR and the watch.

KiptoonKipkurui · 2023-11-07T19:04:13Z

Also, we should look for getting reid of Pipelien configs defined i.e. In this PR we are using blacklist & whitelst and comapring with the CR config based on that watch for the resources.

As Lee also mentioned, we can externalise this configiguration in a ConfigMap. Whenever the configmap is updated we would reconcile the CR and the watch.

Sorry Im not understanding well, the whitelist and blacklist here are supplied from a ConfigMap from the CR as from this other PR yet to be approved, the default configurations are maintained here as a backup. In this case the config map is updated together with the CR then the meshsync can watch for changes, Are you speaking along the same lines or something different?

MUzairS15 · 2023-11-07T19:08:03Z

Yes, I am on same line. But I was thinking if we could externalise it, i.e. instead of having an attribute of type ConfigMap in the CR we would have a ConfigMap resource which would contain the configs and instead of updating CR we could update ConfigMap.

MUzairS15 · 2023-11-07T19:08:27Z

Makes sense?

leecalcote · 2023-11-07T19:10:55Z

Makes sense?

Yes. +1

and remove listener configuration from whiteliest and blacklist resources Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

KiptoonKipkurui · 2023-11-07T19:20:31Z

Makes sense?

Now i get it, in that case, what would the need for having the ConfigMap in the CR? It can work as an independent component that the meshery server can independently configure it

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

KiptoonKipkurui · 2023-11-07T19:38:56Z

@MUzairS15 I have resolved the issues, pending extraction of the configs to a ConfigMap independent of the CRD, if in agreement we can scrap off updating the CRD and dealing sorely with an independent config map @leecalcote

MUzairS15 · 2023-11-08T12:48:36Z

Thanks @KiptoonKipkurui 🚀
Is your PR in meshery operator ready?
I am merging this now.

KiptoonKipkurui added 3 commits October 11, 2023 10:42

add MeshsyncConfig used to receive config from meshync

63ab2a3

CRD Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

add functionality to process config from

8fecc42

meshsync crd Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

refactor to obtain incoporate configs from meshsync

c2f557e

CRD if available Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

Merge branch 'master' into feat/kiptoonkipkurui/meshsync_crd

80d8263

KiptoonKipkurui requested review from leecalcote, theBeginner86 and MUzairS15 October 11, 2023 12:50

KiptoonKipkurui added pr/do-not-merge PRs not ready to be merged component/meshsync MeshSync related labels Oct 11, 2023

Aisuko reviewed Oct 14, 2023

View reviewed changes

KiptoonKipkurui and others added 4 commits October 23, 2023 21:52

ensure the cr is present before using it

439496a

Co-authored-by: Aisuko <urakiny@gmail.com> Signed-off-by: Kiptoon Kipkurui <50325211+KiptoonKipkurui@users.noreply.github.com>

check conversion of spec for success

2ee712f

Co-authored-by: Aisuko <urakiny@gmail.com> Signed-off-by: Kiptoon Kipkurui <50325211+KiptoonKipkurui@users.noreply.github.com>

Merge branch 'meshery:master' into feat/kiptoonkipkurui/meshsync_crd

543b383

refactor to add checks before using spec map

55d3cc3

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

update variable namings for better understandability

3dcd825

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

KiptoonKipkurui added 6 commits October 29, 2023 22:22

add slice package

042eb80

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

add watched resource model

46256a9

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

refactor to allow configuration of resources

c82b97c

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

check whether an event type is supported before

4837992

publishing item Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

check for presence of configs from piplines and

95c9d15

listeners before using them Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

safe initialize pipeline

0e99363

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

MUzairS15 reviewed Nov 1, 2023

View reviewed changes

KiptoonKipkurui added 4 commits November 4, 2023 10:07

go mod tidy package management

d1b0c76

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

refactor to supply dynamic kubernetes client

a01a14a

from main to enable smoother unit testing Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

add unit tests for crd configuration

6d68702

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

add default events to be tracked

be975d8

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

refactor for easier testing

077384f

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

MUzairS15 reviewed Nov 7, 2023

View reviewed changes

MUzairS15 approved these changes Nov 7, 2023

View reviewed changes

KiptoonKipkurui added 2 commits November 7, 2023 22:13

ensure either whitelist or blacklisted resources allowed

99d89b2

and remove listener configuration from whiteliest and blacklist resources Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

Merge branch 'master' into feat/kiptoonkipkurui/meshsync_crd

89d41ad

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

update packages to get dynamic kubernetes go client

988de5b

Signed-off-by: Daniel Kiptoon <kiptoonkipkurui@gmail.com>

MUzairS15 merged commit e1545b9 into meshery:master Nov 8, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meshync Dynamic Configuration from Meshsync #258

Meshync Dynamic Configuration from Meshsync #258

KiptoonKipkurui commented Oct 11, 2023

codecov bot commented Oct 11, 2023 •

edited

Loading

Aisuko left a comment

Aisuko Oct 14, 2023

KiptoonKipkurui Oct 23, 2023

Aisuko Oct 14, 2023

KiptoonKipkurui Oct 23, 2023

KiptoonKipkurui commented Oct 23, 2023 •

edited

Loading

leecalcote commented Oct 26, 2023 •

edited

Loading

MUzairS15 left a comment

KiptoonKipkurui commented Nov 1, 2023 •

edited

Loading

MUzairS15 left a comment

MUzairS15 Nov 7, 2023

KiptoonKipkurui Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

MUzairS15 left a comment

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

leecalcote commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 8, 2023

	if idx := slices.IndexFunc(meshsyncConfig.BlackList, func(c string) bool { return c == v.Name }); idx != -1 {
	if idx := slices.IndexFunc(meshsyncConfig.BlackList, func(c string) bool { return c == v.Name }); idx == -1 {

Meshync Dynamic Configuration from Meshsync #258

Meshync Dynamic Configuration from Meshsync #258

Conversation

KiptoonKipkurui commented Oct 11, 2023

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

Aisuko left a comment

Choose a reason for hiding this comment

Aisuko Oct 14, 2023

Choose a reason for hiding this comment

KiptoonKipkurui Oct 23, 2023

Choose a reason for hiding this comment

Aisuko Oct 14, 2023

Choose a reason for hiding this comment

KiptoonKipkurui Oct 23, 2023

Choose a reason for hiding this comment

KiptoonKipkurui commented Oct 23, 2023 • edited Loading

leecalcote commented Oct 26, 2023 • edited Loading

MUzairS15 left a comment

Choose a reason for hiding this comment

KiptoonKipkurui commented Nov 1, 2023 • edited Loading

MUzairS15 left a comment

Choose a reason for hiding this comment

MUzairS15 Nov 7, 2023

Choose a reason for hiding this comment

KiptoonKipkurui Nov 7, 2023

Choose a reason for hiding this comment

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

MUzairS15 left a comment

Choose a reason for hiding this comment

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

MUzairS15 commented Nov 7, 2023

leecalcote commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

KiptoonKipkurui commented Nov 7, 2023

MUzairS15 commented Nov 8, 2023

codecov bot commented Oct 11, 2023 •

edited

Loading

KiptoonKipkurui commented Oct 23, 2023 •

edited

Loading

leecalcote commented Oct 26, 2023 •

edited

Loading

KiptoonKipkurui commented Nov 1, 2023 •

edited

Loading