Skip to content

azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD#15182

Merged
aanm merged 2 commits intocilium:masterfrom
AnishShah:azure-ipam
Mar 9, 2021
Merged

azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD#15182
aanm merged 2 commits intocilium:masterfrom
AnishShah:azure-ipam

Conversation

@AnishShah
Copy link
Copy Markdown
Contributor

@AnishShah AnishShah commented Mar 3, 2021 •
edited
Loading

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.
  • 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.
  • Thanks for contributing!

The incorrect JSON tag "-" was added to GatewayIP field in commit 8de6dc5
that was causing GatewayIP field to not get populated in CiliumNode CRD.
Since GatewayIP field was not populated, Cilium CNI was creating incorrect
ip rules and routes that was affecting pod-to-pod connectivity.

Fix a bug that was causing Azure IPAM with multiple pod subnets to not work.

@AnishShah AnishShah requested review from a team as code owners March 3, 2021 05:45
@AnishShah AnishShah requested a review from a team March 3, 2021 05:45
@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 3, 2021
@AnishShah AnishShah requested review from nathanjsweet and ti-mo March 3, 2021 05:45
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Mar 3, 2021
@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 3, 2021
@aanm aanm requested a review from christarazi March 3, 2021 10:09
aanm
aanm requested changes Mar 3, 2021
View reviewed changes
@AnishShah AnishShah requested a review from a team March 3, 2021 22:22
@AnishShah AnishShah changed the title azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD DO NOT MERGE azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD Mar 4, 2021
@AnishShah
Copy link
Copy Markdown
Contributor Author

AnishShah commented Mar 4, 2021

Adding DO NOT MERGE tag because I haven't got chance to test the second commit on my Azure cluster.

@christarazi
Copy link
Copy Markdown
Member

christarazi commented Mar 4, 2021

Adding DO NOT MERGE tag because I haven't got chance to test the second commit on my Azure cluster.

You can convert the PR into draft mode to indicate that it's WIP.

@AnishShah AnishShah marked this pull request as draft March 4, 2021 19:32
@AnishShah AnishShah changed the title DO NOT MERGE azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD Mar 4, 2021
ti-mo
ti-mo reviewed Mar 5, 2021
View reviewed changes
AnishShah added 2 commits March 5, 2021 10:25
@AnishShah
azure: Add the correct JSON tag to GatewayIP field in CiliumNode CRD
cf4aacd 
The incorrect JSON tag "-" was added to GatewayIP field in commit 8de6dc5
that was causing GatewayIP field to not get populated in CiliumNode CRD.

Signed-off-by: Anish Shah <anishshah@google.com>
@AnishShah
azure: Add Gateway field to AzureInterface and deprecate GatewayIP field
4be9699 
The JSON tag for GatewayIP field does not follow the same convention as
other field. Inorder to have the same consistency across all JSON tags,
we introduce Gateway field and deprecate GatewayIP field.

Signed-off-by: Anish Shah <anishshah@google.com>
@AnishShah AnishShah marked this pull request as ready for review March 5, 2021 20:32
@AnishShah
Copy link
Copy Markdown
Contributor Author

AnishShah commented Mar 5, 2021

Tested it on my cluster that this works

ti-mo
ti-mo approved these changes Mar 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

@aanm
Copy link
Copy Markdown
Member

aanm commented Mar 8, 2021 •
edited by christarazi
Loading

test-me-please

Edit: netnext hit #15246

@AnishShah
Copy link
Copy Markdown
Contributor Author

AnishShah commented Mar 8, 2021

Looks like there's a flaky test.

@christarazi
Copy link
Copy Markdown
Member

christarazi commented Mar 8, 2021

test-1.13-netnext

@aanm
Copy link
Copy Markdown
Member

aanm commented Mar 9, 2021

hit #15246

@aanm aanm merged commit 553e965 into cilium:master Mar 9, 2021
@AnishShah AnishShah deleted the azure-ipam branch March 9, 2021 16:59
@pchaigno pchaigno mentioned this pull request Mar 10, 2021
Merged
@christarazi christarazi mentioned this pull request Mar 10, 2021
Merged
@kaworu kaworu mentioned this pull request Mar 12, 2021
Merged
@joestringer joestringer added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Apr 20, 2021
@joestringer joestringer mentioned this pull request Apr 20, 2021
Merged
HadrienPatte added a commit that referenced this pull request Apr 2, 2026
@HadrienPatte
ipam: Remove obsolete fields from cloud provider IPAM types
c3c61e4 
Remove deprecated fields that have been unused for 5+ years:
* Azure `AzureInterface.GatewayIP` (obsolete since v1.10, see #15182)
* AWS `ENISpec.InstanceID` (obsolete since v1.8, see #10569)
* AWS `ENISpec.{MinAllocate,PreAllocate,MaxAboveWatermark}` (obsolete since v1.8, see #10089)

Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
@HadrienPatte HadrienPatte mentioned this pull request Apr 2, 2026
Open
HadrienPatte added a commit that referenced this pull request Apr 2, 2026
@HadrienPatte
ipam: Remove obsolete fields from cloud provider IPAM types
e45b778 
Remove deprecated fields that have been unused for 5+ years:
* Azure `AzureInterface.GatewayIP` (obsolete since v1.10, see #15182)
* AWS `ENISpec.InstanceID` (obsolete since v1.8, see #10569)
* AWS `ENISpec.{MinAllocate,PreAllocate,MaxAboveWatermark}` (obsolete since v1.8, see #10089)

Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ti-mo ti-mo ti-mo approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@nathanjsweet nathanjsweet Awaiting requested review from nathanjsweet

Assignees

@nathanjsweet nathanjsweet

@ti-mo ti-mo

@christarazi christarazi

Labels

release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@AnishShah @christarazi @aanm @ti-mo @kaworu @nathanjsweet @joestringer @nebril @pchaigno