Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Envoy version check if L7 proxy is disabled #20440

Merged
merged 1 commit into from Jul 8, 2022

Conversation

bmcustodio
Copy link
Contributor

If --enable-l7-proxy is set to false, there is no point in performing the Envoy version check, and so it should be automatically disabled. One of the motivations for this change is that in some scenarios it's necessary to disable the L7 proxy to allow Cilium to start (eg. #17467), but specifying --enable-l7-proxy=false is not enough due to the version check still being performed in this situation. This commit attempts to improve the UX by disabling the version check if Envoy itself is disabled.

Envoy version checking is now disabled whenever L7 proxy is disabled too

@bmcustodio bmcustodio requested a review from a team July 8, 2022 14:10
@bmcustodio bmcustodio requested a review from a team as a code owner July 8, 2022 14:10
@bmcustodio bmcustodio requested a review from pippolo84 July 8, 2022 14:10
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 8, 2022
@bmcustodio bmcustodio requested a review from twpayne July 8, 2022 14:10
@pchaigno pchaigno added area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/misc This PR makes changes that have no direct user impact. labels Jul 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 8, 2022
pkg/option/config.go Outdated Show resolved Hide resolved
@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.11 and removed release-note/misc This PR makes changes that have no direct user impact. labels Jul 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.7 Jul 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.0 Jul 8, 2022
@bmcustodio bmcustodio force-pushed the pr/bruno/no-envoy-version-check-if-no-l7-proxy branch from 8345978 to 64a32ec Compare July 8, 2022 14:19
If '--enable-l7-proxy' is set to 'false', there is no point in
performing the Envoy version check, and so it should be automatically
disabled. One of the motivations for this change is that in some
scenarios it's necessary to disable the L7 proxy to allow Cilium to
start (eg. #17467), but
specifying '--enable-l7-proxy=false' is not enough due to the version
check still being performed in this situation. This commit attempts to
improve the UX by disabling the version check if Envoy itself is
disabled.

Signed-off-by: Bruno M. Custódio <brunomcustodio@gmail.com>
@bmcustodio bmcustodio force-pushed the pr/bruno/no-envoy-version-check-if-no-l7-proxy branch from 64a32ec to c6523db Compare July 8, 2022 14:23
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pchaigno
Copy link
Member

pchaigno commented Jul 8, 2022

I don't think we need to run the end-to-end tests for this given it's only going to skip a check when l7Proxy is set. Smoke tests are passing and reviews are in, so marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 8, 2022
@aanm aanm merged commit a46e572 into master Jul 8, 2022
@aanm aanm deleted the pr/bruno/no-envoy-version-check-if-no-l7-proxy branch July 8, 2022 22:11
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.12 in 1.12.0 Jul 14, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.12 in 1.12.0 Jul 14, 2022
@aanm aanm added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Jul 14, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.0 Jul 14, 2022
@joestringer joestringer added this to Needs backport from master in 1.11.8 Jul 15, 2022
@joestringer joestringer removed this from Needs backport from master in 1.11.7 Jul 15, 2022
@tklauser tklauser added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Aug 11, 2022
@tklauser tklauser moved this from Needs backport from master to Backport done to v1.11 in 1.11.8 Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.11.8
Backport done to v1.11
1.12.0
Backport done to v1.12
Development

Successfully merging this pull request may close these issues.

None yet

6 participants