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

fix: Delegated ipam not configure ipv6 if ipv6 disabled in agent #31104

Merged
merged 1 commit into from Mar 27, 2024

Conversation

tamilmani1989
Copy link
Contributor

@tamilmani1989 tamilmani1989 commented Mar 1, 2024

Delegated ipam returns ipv6 address to cilium cni even if ipv6 disabled in cilium agent config. In this scenario, ipv6 node addressing is not set and its causing cilium cni to crash if delegated ipam returns ipv6.

Requires backport to 1.14 and 1.15 versions

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #31103

<!-- Fix crash of cilium-cni with delegated IPAM returning ipv6 when IPv6 is disabled in Agent -->

@tamilmani1989 tamilmani1989 requested a review from a team as a code owner March 1, 2024 23:31
@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 Mar 1, 2024
@squeed squeed added needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Mar 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.8 Mar 4, 2024
@squeed
Copy link
Contributor

squeed commented Mar 4, 2024

Good catch.

My suggestion would be to fix this on this line instead -- something like

if ipv6IsEnabled(ipam) && conf.Addressing.IPV6 != nil {

And, for extra safety, do the same for ipv4 on line 578.

Make sense?

@tamilmani1989
Copy link
Contributor Author

tamilmani1989 commented Mar 4, 2024

Good catch.

My suggestion would be to fix this on this line instead -- something like

if ipv6IsEnabled(ipam) && conf.Addressing.IPV6 != nil {

And, for extra safety, do the same for ipv4 on line 578.

Make sense?

Yep I thought about this. The reason I went with this approach is ipv6IsEnabled returns true even if conf.Addressing.IPV6 is nil . Thats why decided to update in allocateIPsWithDelegatedPlugin to be safer and not to regress. I'm fine adding in the place you suggested if there won't be any issues or even better to make ipv6IsEnabled to return false if ipam.HostAddressing.IPV6 is nil?

	ipam := &models.IPAMResponse{
		HostAddressing: conf.Addressing,
		Address:        &models.AddressPair{},
	}

func ipv6IsEnabled(ipam *models.IPAMResponse) bool {
	if ipam == nil || ipam.Address.IPV6 == "" {
		return false
	}

	**if ipam.HostAddressing != nil && ipam.HostAddressing.IPV6 != nil {
		return ipam.HostAddressing.IPV6.Enabled
	}**

	return true
}

@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 4, 2024
@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 Mar 4, 2024
@joestringer
Copy link
Member

Hi, I'd suggest a release note in the description like: "Fix crash of cilium-cni with delegated IPAM when IPv6 is disabled". This way users have a decent chance to understand the change and whether this fix may address a problem they are facing.

@joestringer
Copy link
Member

/test

1 similar comment
@tamilmani1989
Copy link
Contributor Author

/test

@squeed
Copy link
Contributor

squeed commented Mar 11, 2024

I'm fine adding in the place you suggested if there won't be any issues or even better to make ipv6IsEnabled to return false if ipam.HostAddressing.IPV6 is nil?

@tamilmani1989 let's do both, belt-and-suspenders. allocateIPsWithDelegatedPlugin is really about parsing the ipam result, so it shouldn't care, per se, about configuration. Separately, we should consult configuration before applying the IPAM result to the interface.

@tamilmani1989
Copy link
Contributor Author

@squeed Thanks for reviewing. Addressed your comment.

@jrajahalme jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.14.9 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.14.8 Mar 13, 2024
@squeed
Copy link
Contributor

squeed commented Mar 22, 2024

/test

Delegated ipam returns ipv6 address to cilium cni even if ipv6 disabled
in cilium agent config. In this scenario, ipv6 node addressing is not
set and its causing cilium cni to crash if delegated ipam returns ipv6
but disabled in cilium agent.

Signed-off-by: Tamilmani <tamanoha@microsoft.com>
@tamilmani1989
Copy link
Contributor Author

/test

@jrajahalme jrajahalme added this to Needs backport from main in 1.14.10 Mar 26, 2024
@jrajahalme jrajahalme removed this from Needs backport from main in 1.14.9 Mar 26, 2024
@tamilmani1989
Copy link
Contributor Author

tamilmani1989 commented Mar 26, 2024

/test

@tamilmani1989
Copy link
Contributor Author

/ci-eks

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 27, 2024
@tklauser tklauser added this pull request to the merge queue Mar 27, 2024
Merged via the queue into cilium:main with commit 034aee7 Mar 27, 2024
62 checks passed
@joamaki joamaki mentioned this pull request Apr 2, 2024
10 tasks
@joamaki joamaki added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Apr 2, 2024
@joamaki joamaki mentioned this pull request Apr 2, 2024
13 tasks
@joamaki joamaki added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Apr 2, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Apr 4, 2024
@asauber asauber moved this from Needs backport from main to Backport done to v1.14 in 1.14.10 Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. 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.14.10
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

Delegated ipam returns ipv6 causing cilium cni to panic in ipv6 disabled mode
5 participants