Ignore clusters with invalid kubeconfig #1956

nevalla · 2021-01-14T11:06:38Z

This PR will provide simplified version of #1453 to ignore clusters that have invalid/corrupted kubeconfig on cluster startup. With this PR those clusters are marked as dead and dead clusters are not enabled.

fixes #1316, fixes #1561

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

jim-docker · 2021-01-14T15:46:47Z

If it's just invalid kubeconfig we're concerned with here can't we test it for validity before creating a cluster in the clusterStore? The Add cluster dialog could even filter out bad ones before building the context list (and show notification(s) indicating which contexts are bad). I never really liked the idea of carrying around dead clusters

Nokel81 · 2021-01-14T16:35:09Z

There should be some sort of notification to the user, otherwise we will have bug reports stating that lens removes their clusters.

nevalla · 2021-01-14T18:54:09Z

If it's just invalid kubeconfig we're concerned with here can't we test it for validity before creating a cluster in the clusterStore? The Add cluster dialog could even filter out bad ones before building the context list (and show notification(s) indicating which contexts are bad). I never really liked the idea of carrying around dead clusters

We have some validation when adding a cluster and this PR will improve that even. This "dead cluster" issue occurs when some process outside of lens app is modifying kubeconfig file for the cluster that is already added to Lens.

nevalla · 2021-01-14T18:54:49Z

There should be some sort of notification to the user, otherwise we will have bug reports stating that lens removes their clusters.

Yes, that would be nice. Let me see what I can do for it.

nevalla · 2021-01-14T19:40:32Z

One option could be to send notifications from all dead clusters on start up? Is it possible to have some custom action on notification, for example remove the cluster completely?

Nokel81 · 2021-01-14T19:41:26Z

One option could be to send notifications from all dead clusters on start up? Is it possible to have some custom action on notification, for example remove the cluster completely?

Yeah that is a good idea. "Ignore" and "Remove" maybe?

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla · 2021-01-15T09:59:28Z

Notifications would benefit from #1941, so I propose that should be done in a future PR.

nevalla · 2021-01-15T10:24:55Z

Added unit tests for kubeconfig validation. I think this is now ready for the review, so please take a look.

Nokel81 · 2021-01-15T16:47:24Z

src/common/kube-helpers.ts

+ * Validates Context, User and Cluster sructs in given kubeconfig. Additionally this will validate
+ * the command passed to the exec substructure.
+ */
+export function validateKubeConfig (config: KubeConfig, contextName: string, validationOpts?: { validateCluster?: boolean, validateUser?: boolean, validateExec?: boolean}) {


Can you extract the this out to ValidationOptions?

And provided a default {} for validationOpts

Nokel81 · 2021-01-15T16:49:39Z

src/main/cluster.ts

+   *
+   * @observable
+   */
+  @observable isDead = false;


This doesn't need to be observable since it is only set in the constructor.

changed as public property now.

Nokel81 · 2021-01-15T21:29:16Z

src/main/cluster.ts

+  /**
+   * Is cluster marked as dead, for example due the invalid kubeconfig
+   *
+   * @observable
+   */
+  @observable isDead = false;


And as I mentioned in my previous PR, I really think that adding this field is conflating what a cluster is.

matti · 2021-01-21T15:12:28Z

ping - my lens corrupts daily and I need to remove applicationsupport/lens

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Nokel81 · 2021-01-22T14:35:01Z

src/common/kube-helpers.ts

+ * Validates Context, User and Cluster sructs in given kubeconfig. Additionally this will validate
+ * the command passed to the exec substructure.


Suggested change

* Validates Context, User and Cluster sructs in given kubeconfig. Additionally this will validate

* the command passed to the exec substructure.

* Checks if `config` has valid `Context`, `User`, `Cluster`, and `exec` fields (if present when required)

Nokel81 · 2021-01-22T14:35:55Z

src/main/cluster.ts

@@ -76,6 +76,7 @@ export class Cluster implements ClusterModel, ClusterState {
   * If extension sets this it needs to also mark cluster as enabled on activate (or when added to a store)
   */
  public ownerRef: string;
+  public isDead = false;


I still don't think that a cluster should be created at all if the kube config is invalid or non-existant.

nevalla · 2021-02-25T14:17:13Z

I've created separate PR (#2233) to display a notification when cluster with invalid kubeconfig is detected.

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Nokel81 · 2021-02-25T14:22:17Z

That notification looks good, but the trigger is still wrong IMO. It should be triggered from ClusterStore when a constructor fails. And when it fails it shouldn't be treated as an "alive" cluster (with its icon and such).

nevalla · 2021-02-25T14:24:28Z

That notification looks good, but the trigger is still wrong IMO. It should be triggered from ClusterStore when a constructor fails. And when it fails it shouldn't be treated as an "alive" cluster (with its icon and such).

It's not "alive" cluster, Lens already ignores clusters that are not enabled.

Nokel81 · 2021-02-25T14:28:10Z

But why add it to the list then in the first place?

nevalla · 2021-02-25T14:29:53Z

But why add it to the list then in the first place?

If it's not in the list, it will get lost when persisting the data to cluster store.

Nokel81 · 2021-02-25T14:33:41Z

Then that implies that there should be another field for kubeconfigs that are no longer valid (since we validate them on the add step).

nevalla · 2021-02-25T15:03:09Z

IMHO this is still improvement to the current situation where Lens app is crashing if there are corrupted kubeconfigs associated. What is also good with this PR, it's not changing existing functionality too much.

Nokel81 · 2021-02-26T13:27:16Z

I agree that it is an improvement (but then again so was my PR (which I will no close)). But given that the imo better solution isn't that much extra work. And doesn't future complicate an already very complicated class (namely Cluster). I think it is worth it do it that way the first time around.

Yes it adds another field, that rarely if ever gets populated, to ClusterStore but I still think that from a design perspective we shouldn't be having these sorts of try {} catch {} blocks within a constructor.

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla · 2021-03-01T10:05:56Z

I've now removed isDead property so extensions API won't have any changes by this change. Cluster.apiUrl is now checked instead when setting cluster as enabled.

* Display warning notification if invalid kubeconfig detected Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Nokel81

This is definitely better though I still would like to see in the future removing that try { ... } catch { ... } from the constructor and have these clusters be part of a separate list to keep the conceptual size of what a Cluster instance "is" as small as necessary.

* Ignore clusters with invalid kubeconfig Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Improve error message Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Mark cluster as dead if kubeconfig loading fails Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Fix tests Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Validate cluster object in kubeconfig when constructing cluster Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Add unit tests for validateKubeConfig Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Refactor validateKubeconfig unit tests Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Extract ValidationOpts type Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Add default value to validationOpts param Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Change isDead to property Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Fix lint issues Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Add missing new line Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Update validateKubeConfig in-code documentation Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Remove isDead property Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> * Display warning notification if invalid kubeconfig detected (#2233) * Display warning notification if invalid kubeconfig detected Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Ignore clusters with invalid kubeconfig

a33327d

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla added the bug Something isn't working label Jan 14, 2021

nevalla added 4 commits January 14, 2021 13:16

Improve error message

be0ce02

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Mark cluster as dead if kubeconfig loading fails

7baaec5

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Fix tests

6dd7374

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Validate cluster object in kubeconfig when constructing cluster

1cbac44

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla added 2 commits January 15, 2021 11:31

Add unit tests for validateKubeConfig

7c8ca40

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Refactor validateKubeconfig unit tests

1b963b4

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla requested a review from a team January 15, 2021 09:45

Nokel81 reviewed Jan 15, 2021

View reviewed changes

jakolehm added this to the 4.0.9 milestone Jan 21, 2021

nevalla added 5 commits January 22, 2021 13:39

Extract ValidationOpts type

a88acc1

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Add default value to validationOpts param

c4d029b

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Change isDead to property

082a451

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Fix lint issues

b5e2749

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

Add missing new line

615627c

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla requested a review from a team January 22, 2021 13:09

Nokel81 reviewed Jan 22, 2021

View reviewed changes

jakolehm previously approved these changes Jan 29, 2021

View reviewed changes

jakolehm requested a review from a team February 4, 2021 18:17

jakolehm added this to the 4.1.4 milestone Feb 24, 2021

Merge branch 'master' into fix/ignore-clusters-with-corrupted-kubeconfig

607ebb5

nevalla mentioned this pull request Feb 25, 2021

Display warning notification if invalid kubeconfig detected #2233

Merged

Update validateKubeConfig in-code documentation

f676429

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla dismissed stale reviews from aleksfront and jakolehm via f676429 February 25, 2021 14:21

lensapp deleted a comment Feb 26, 2021

Remove isDead property

ed6e007

Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla requested review from jakolehm and aleksfront March 1, 2021 10:06

jakolehm previously approved these changes Mar 1, 2021

View reviewed changes

Display warning notification if invalid kubeconfig detected (#2233)

20c1b06

* Display warning notification if invalid kubeconfig detected Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>

nevalla dismissed jakolehm’s stale review via 20c1b06 March 1, 2021 11:15

jakolehm approved these changes Mar 1, 2021

View reviewed changes

Nokel81 mentioned this pull request Mar 1, 2021

mark uncreatable Cluster objects in cluster-store as dead #1453

Closed

Nokel81 approved these changes Mar 1, 2021

View reviewed changes

jakolehm merged commit 4f74b9a into master Mar 1, 2021

jakolehm deleted the fix/ignore-clusters-with-corrupted-kubeconfig branch March 1, 2021 15:30

Nokel81 mentioned this pull request Mar 2, 2021

Can't startup Lens because of error #1959

Closed

jakolehm mentioned this pull request Mar 3, 2021

Release v4.1.4 #2274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore clusters with invalid kubeconfig #1956

Ignore clusters with invalid kubeconfig #1956

nevalla commented Jan 14, 2021 •

edited

jim-docker commented Jan 14, 2021

Nokel81 commented Jan 14, 2021

nevalla commented Jan 14, 2021

nevalla commented Jan 14, 2021

nevalla commented Jan 14, 2021

Nokel81 commented Jan 14, 2021

nevalla commented Jan 15, 2021

nevalla commented Jan 15, 2021

Nokel81 Jan 15, 2021

Nokel81 Jan 15, 2021

nevalla Jan 22, 2021

Nokel81 Jan 15, 2021

nevalla Jan 22, 2021

Nokel81 Jan 15, 2021

matti commented Jan 21, 2021

Nokel81 Jan 22, 2021

Nokel81 Jan 22, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 26, 2021

nevalla commented Mar 1, 2021

Nokel81 left a comment

		* Validates Context, User and Cluster sructs in given kubeconfig. Additionally this will validate
		* the command passed to the exec substructure.

	* Validates Context, User and Cluster sructs in given kubeconfig. Additionally this will validate
	* the command passed to the exec substructure.
	* Checks if `config` has valid `Context`, `User`, `Cluster`, and `exec` fields (if present when required)

Ignore clusters with invalid kubeconfig #1956

Ignore clusters with invalid kubeconfig #1956

Conversation

nevalla commented Jan 14, 2021 • edited

jim-docker commented Jan 14, 2021

Nokel81 commented Jan 14, 2021

nevalla commented Jan 14, 2021

nevalla commented Jan 14, 2021

nevalla commented Jan 14, 2021

Nokel81 commented Jan 14, 2021

nevalla commented Jan 15, 2021

nevalla commented Jan 15, 2021

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

Choose a reason for hiding this comment

matti commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 25, 2021

nevalla commented Feb 25, 2021

Nokel81 commented Feb 26, 2021

nevalla commented Mar 1, 2021

Nokel81 left a comment

Choose a reason for hiding this comment

nevalla commented Jan 14, 2021 •

edited