helm mode: add clustermesh enable/disable #1527
Conversation
asauber
requested review from
joamaki,
YutaroHayakawa,
brlbil and
michi-covalent
asauber
force-pushed
the
pr/asauber/helm-clustermesh
branch
from
6c4aa3d to
1d674a9
Compare
asauber
force-pushed
the
pr/asauber/helm-clustermesh
branch
from
1d674a9 to
cc39c3a
Compare
asauber
force-pushed
the
pr/asauber/helm-clustermesh
branch
from
cc39c3a to
1ee3fe9
Compare
asauber
changed the title
cilium clustermesh {enable,disable} using Helmcilium clustermesh {enable,disable}
asauber
changed the title
cilium clustermesh {enable,disable}
asauber
force-pushed
the
pr/asauber/helm-clustermesh
branch
from
1ee3fe9 to
ea0fe4d
Compare
Implements a version of `clustermesh enable` and `clustermesh disable` using Helm actions run directly against the cluster Signed-off-by: Andrew Sauber <[email protected]>
Enables a matrix for "classic" and "helm" mode for multicluster tests using GKE. The recently added parameter for "cilium_cli_mode" in the cilium-cli-test-job-chart is used. Signed-off-by: Andrew Sauber <[email protected]>
asauber
force-pushed
the
pr/asauber/helm-clustermesh
branch
from
0e18420 to
a4bb01b
Compare
|
The new workflow was temporarily enabled on this PR for testing purposes. Logs for a successful run of the modified and new workflow can be found here: (classic) https://github.com/cilium/cilium-cli/actions/runs/4789575696/jobs/8517609808?pr=1527 |
Signed-off-by: Andrew Sauber <[email protected]>
internal/cli/cmd/clustermesh.go
Outdated
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(¶ms.ServiceType, "service-type", "LoadBalancer", "Type of Kubernetes service to expose control plane { ClusterIP | LoadBalancer | NodePort }") |
There was a problem hiding this comment.
This changes the default value from the old behavior. Did you have any reason you wanted to change the default?
There was a problem hiding this comment.
I believe I chose LoadBalancer here, because some version of the clustermesh setup docs (which I can't seem to find at the moment), specified --service-type NodePort, as though LoadBalancer was the default.
The default does appear to be NodePort https://github.com/cilium/cilium/blob/main/install/kubernetes/cilium/values.yaml#L2385
I'll change this to NodePort.
There was a problem hiding this comment.
Thanks for your consideration. I would also like to know which of the cilium-cli and Helm defaults we should preserve. I don't know the overall direction of this work, but if we use the old cilium-cli default, the default is an empty string, and cilium-cli does auto-detection depending on the environment (https://github.com/cilium/cilium-cli/blob/main/clustermesh/clustermesh.go#L111-L135). In your new logic, you cut out the auto-detection logic. Is it expected?
There was a problem hiding this comment.
Yes, the current direction for much of this autodetection logic is to replace it with documentation, as it has been a source of bugs.
Signed-off-by: Andrew Sauber <[email protected]>
|
@brlbil ping! |
|
it's green, there i click. |
Two commits for review
Add cilium clustermesh {enable,disable} using Helm
Implements a version of
clustermesh enableandclustermesh disableusing Helm actions run directly against the clusterEnable Helm mode matrix for multicluster tests
Enables a workflow matrix for "classic" and "helm" mode for multicluster tests using GKE. The recently added parameter for cilium_cli_mode in the
cilium-cli-test-job-chartis used.