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

enabled l7Proxy & removed err.log message for WireGuard #1419

Conversation

Neilblaze
Copy link
Contributor

Since cilium/cilium#19401 (to be released in v1.14) there's no need to disable the L7 proxy when WireGuard is enabled on the > v1.13 Cilium (the relevant log message L7 proxy disabled due to Wireguard encryption).

As a bonus, the unnecessary log/WARN Wireguard does not support node encryption yet has also been removed.

Fixes #1405
cc: @brb
Signed-off-by: Pratyay Banerjee

@Neilblaze Neilblaze requested a review from a team as a code owner February 27, 2023 15:26
@Neilblaze Neilblaze temporarily deployed to ci February 27, 2023 15:27 — with GitHub Actions Inactive
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks! Some comment.

install/helm.go Outdated
if k.params.NodeEncryption {
k.Log("⚠️️ Wireguard does not support node encryption yet")
}
helmMapOpts["l7Proxy"] = "true"
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to set any l7Proxy here, as it should be enabled by default.

However, we need to figure out which version of Cilium we are trying to install. I.e., we need to keep the existing code for <1.14 versions.

Copy link
Contributor Author

@Neilblaze Neilblaze Feb 27, 2023

Choose a reason for hiding this comment

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

@brb Thanks for the review. That means I just need to implement a simple versioncheck ("<=1.13.0") to restrict, right? 👇🏼

switch k.params.Encryption {
case encryptionIPsec:
	helmMapOpts["encryption.enabled"] = "true"
	helmMapOpts["encryption.type"] = "ipsec"
	if k.params.NodeEncryption {
		helmMapOpts["encryption.nodeEncryption"] = "true"
	}
case encryptionWireguard:
	helmMapOpts["encryption.enabled"] = "true"
	helmMapOpts["encryption.type"] = "wireguard"
	// TODO(gandro): Future versions of Cilium will remove the following
	// two limitations, we will need to have set the config map values
	// based on the installed Cilium version
	if versioncheck.MustCompile("<=1.13.0")(k.chartVersion) {
		helmMapOpts["l7Proxy"] = "false"
		k.Log("ℹ️  L7 proxy disabled due to Wireguard encryption")

		if k.params.NodeEncryption {
			k.Log("⚠️️  Wireguard does not support node encryption yet")
		}
	}
}

Copy link
Member

Choose a reason for hiding this comment

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

Yep 👍

@Neilblaze Neilblaze temporarily deployed to ci February 27, 2023 16:25 — with GitHub Actions Inactive
@Neilblaze Neilblaze requested review from brb and removed request for nathanjsweet February 27, 2023 21:11
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks! Could you squash all commit messages into a single one? Also, please no emojis in commit messages 😅

@Neilblaze Neilblaze force-pushed the install-enable-l7Proxy-with-WireGuard-enabled branch from 8b1a430 to 117668a Compare February 28, 2023 14:55
@Neilblaze Neilblaze temporarily deployed to ci February 28, 2023 14:55 — with GitHub Actions Inactive
@Neilblaze
Copy link
Contributor Author

@brb done ✔️

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Close to merging. One more nit - could you remove the duplicates from your commit message?

enabled l7Proxy & removed err.log message for WireGuard

Signed-off-by: Pratyay Banerjee <putubanerjee23@gmail.com>

enabled l7Proxy & removed err.log message for WireGuard

Signed-off-by: Pratyay Banerjee <putubanerjee23@gmail.com>

enabled l7Proxy & removed err.log message for WireGuard

Signed-off-by: Pratyay Banerjee <putubanerjee23@gmail.com>

Signed-off-by: Pratyay Banerjee <putubanerjee23@gmail.com>
@Neilblaze Neilblaze force-pushed the install-enable-l7Proxy-with-WireGuard-enabled branch from 117668a to 6ea9025 Compare February 28, 2023 19:27
@Neilblaze Neilblaze temporarily deployed to ci February 28, 2023 19:27 — with GitHub Actions Inactive
@Neilblaze
Copy link
Contributor Author

One more nit - could you remove the duplicates from your commit message?

@brb Thanks for the catch. Everything should be fine now! 😄

@Neilblaze Neilblaze requested a review from brb February 28, 2023 20:05
@tklauser tklauser merged commit 64924bf into cilium:master Mar 1, 2023
@Neilblaze Neilblaze deleted the install-enable-l7Proxy-with-WireGuard-enabled branch March 1, 2023 07:24
@ShiroDN
Copy link
Contributor

ShiroDN commented Apr 21, 2023

There is a problem now, condition versioncheck.MustCompile("<=1.13.0") does not match patch versions of cilium ex. 1.13.x so now when I tried to install cilium with --encryption wireguard it keeps restarting with error:

level=fatal msg="Wireguard (--enable-wireguard) is not compatible with L7 proxy (--enable-l7-proxy)" subsys=daemon

Created an PR to fix this: #1529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install: Don't disable L7 proxy when installing with WireGuard enabled
4 participants