Add Helm Clustermesh disconnect command

[derailed]

• Surface new command to match cluster disconnect when CLI helm mode in enabled

Signed-off-by: Fernand Galiana fernand.galiana@isovalent.com

[asauber]

The current build failure indicates that the new semver logic is treating the Helm version under test (a 1.13.x version) not as classic mode.

Note that other recent runs of the test "Kind / helm-upgrade-clustermesh (pull_request)" show the log message ⚠️ Cilium Version is less than 1.14.0. Continuing in classic mode.

[asauber]

Yes, the failure is caused by the modifications here to the version checking logic. semver.Compare was specifically avoided previously because it did not handle prefixes and suffixes present in the version strings that are used by Cilium for pre-release versions. I would advise debugging the code using the values present in the CI context, or reverting to the previous implementation.

…

On Tue, Jun 20, 2023 at 11:17 PM Fernand Galiana ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clustermesh/clustermesh.go <#1739 (comment)>: > @@ -2082,6 +2073,107 @@ func (k *K8sClusterMesh) ConnectWithHelm(ctx context.Context) error { return nil } +// Helm-based clustermesh enable is only supported on Cilium +// v1.14+ due to a lack of support in earlier versions for +// autoconfigured certificates (tls.{crt,key}) for cluster +// members when running in certgen (cronJob) PKI mode +func (k *K8sClusterMesh) inClassicMode(r *release.Release) (bool, error) { Also do you have an idea on what's up with the tests failures? When I've first checked in this am, all tests were green and now 2 suites are failing consistently relating to a hosed tls secret key? Do you have a sense on why they started failing? — Reply to this email directly, view it on GitHub <#1739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPTWHW2RMCL5UFICSXN3XDXMJRV7ANCNFSM6AAAAAAZNQAGJM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[asauber]

Correct, this is a semantic nitpick from me. This boolean predicate function is returning whether or not we “needClassicMode”, the value of the environment variable which causes us to reach this function means we must already be in Helm mode. In other words, a Helm version is not “isClassicMode”, but it might “needClassicMode”. It’s really a nitpick.

…

On Tue, Jun 20, 2023 at 11:12 PM Fernand Galiana ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clustermesh/clustermesh.go <#1739 (comment)>: > @@ -2082,6 +2073,107 @@ func (k *K8sClusterMesh) ConnectWithHelm(ctx context.Context) error { return nil } +// Helm-based clustermesh enable is only supported on Cilium +// v1.14+ due to a lack of support in earlier versions for +// autoconfigured certificates (tls.{crt,key}) for cluster +// members when running in certgen (cronJob) PKI mode +func (k *K8sClusterMesh) inClassicMode(r *release.Release) (bool, error) { Thanks Andrew! I thought ClassicMode meant: "run the standard Clustermesh connect/disconnect as if helm upgrade mode is disabled". If that's the case and the cilium rev is < 1.14.0 then we are `in classic mode'. Am I wrong about that? — Reply to this email directly, view it on GitHub <#1739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPTWHRBEC6L6VLAVS467PDXMJRAVANCNFSM6AAAAAAZNQAGJM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[giorio94]

The current semver check does not seem to be correct, which is the cause for the tests failure. I'd also reverse the order of the commits, to reduce the churn.

[asauber]

Looking good, thanks for the additional tests and updates.

[giorio94]

The overall changes look good to me. I've left only a couple of minor cleanup comments and one question inline. I'd finally suggest to move the commit about 'needsClassicMode` first to reduce the churn and ensure that they all result in a working version.

[giorio94]

Looks good to me, thanks!