fix: properly propagate flags used for kubeproxyreplacement #2048
Conversation
bittermandel
changed the title
bittermandel
changed the title
|
@squeed with the changes from the last commit, the following flow is enabled:
This flow makes sense to me and is compatible with previous versions. |
There was a problem hiding this comment.
@bittermandel, thanks for the PR.
You have some gofmt and revive errors in your latest commit. See the "Go / build (pull_request)" workflow logs. Would you be able to resolve those?
Could you also squash your latest changes into the earlier commits?
In addition, could you reformat your commit messages with a shorter summary line and some additional description of the changes? (See this PR for a good example of our commit message standards https://github.com/cilium/cilium-cli/pull/2035/commits)
|
Commit de0d407 does not match "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Previously when installing Cilium using cilium-cli on a cluster without kube-proxy installed, the k8sServiceHost and k8sServicePort flags were ignored and instead the current kubernetes context endpoints were used as a default. This change prioritizes the helm flags over the current kubernetes context, and returns an error if a default cannot be determined. Signed-off-by: Jonathan Grahl <[email protected]>
|
@asauber I made sure to squash the commits, put a more descriptive commit message and a description! |
| @@ -266,6 +266,11 @@ cilium install | |||
| cilium install --context kind-cluster1 --set cluster.id=1 --set cluster.name=cluster1 | |||
| `, | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| cmd.Flags().Visit(func(f *pflag.Flag) { | |||
| if f.Name == "set" && strings.Contains(f.Value.String(), "kubeProxyReplacement") { | |||
There was a problem hiding this comment.
This isn't sufficient as a test for kubeProxyReplacement, as the value could be kubeProxyReplacement=false. In this case we do not want to set params.UserSetKubeProxyReplacement = true.
There was a problem hiding this comment.
The kind workflow is running into an issue with the latest logic:
🔮 Auto-detected kube-proxy has been installed
Error: Unable to install Cilium: could not autodetect API server endpoint, cannot replace kubeproxy
❌ Could not autodetect API server endpoint. Either set a valid kubernetes context or set k8sServerHost & k8sServerPort.
Could you look into why this is the case?
|
@bittermandel Looks like this is failing CI, PTAL. |
|
Looks like the author has abandoned the PR, closing for now. |
After the switch to Helm install & upgrade mode rather than classic mode, the flags for kubeProxyReplacement, k8sServiceHost & k8sServicePort was not propagated properly.
These changes makes sure that there's no auto detection done if the
kubeProxyReplacementflag has been set, and if it's not set, make sure to prioritze the k8sServiceHost / k8sServicePort flags over getting it from the current context.