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.

Sign up for GitHub

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

Jump to bottom

bgpv1: remove references to advertisement from CiliumBGPPeeringPolicy #30337

Merged
merged 1 commit into from Jan 22, 2024
Merged

bgpv1: remove references to advertisement from CiliumBGPPeeringPolicy #30337

merged 1 commit into from Jan 22, 2024

Conversation

harsimran-pabla
Copy link
Contributor

@harsimran-pabla harsimran-pabla commented Jan 19, 2024 •
edited

Advertisement field got introduced into CiliumBGPFamily type when adding v2 APIs. This field is only required in new CiliumBGPPeerConfig structures. This change removes advertisement from CiliumBGPFamily and introduces new type CiliumBGPFamilyWithAdverts which will be used in v2 APIs.

There is no impact to current usage of the CiliumBGPPeeringPolicy CRD.

@harsimran-pabla harsimran-pabla added kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. area/bgp labels Jan 19, 2024
@harsimran-pabla harsimran-pabla requested a review from a team January 19, 2024 14:30
@harsimran-pabla harsimran-pabla requested a review from a team as a code owner January 19, 2024 14:30
@harsimran-pabla harsimran-pabla requested review from rastislavs and removed request for a team January 19, 2024 14:30
@harsimran-pabla harsimran-pabla added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Jan 19, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in v1.15.0-rc.1 Jan 19, 2024
@harsimran-pabla harsimran-pabla marked this pull request as draft January 19, 2024 14:37
@harsimran-pabla
bgpv1: remove references to advertisement from CiliumBGPPeeringPolicy
fd3939c 
Advertisement field got introduced into CiliumBGPFamily type when adding
v2 APIs. This field is only required in new CiliumBGPPeerConfig
structures. This change removes advertisement from CiliumBGPFamily and
introduces new type CiliumBGPFamilyWithAdverts which will be used in v2
APIs.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
rastislavs
rastislavs approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM

@harsimran-pabla harsimran-pabla marked this pull request as ready for review January 19, 2024 15:28
@harsimran-pabla harsimran-pabla requested a review from a team as a code owner January 19, 2024 15:28
@harsimran-pabla
Copy link
Contributor Author

/test

@harsimran-pabla harsimran-pabla added the release-blocker/1.15 This issue will prevent the release of the next version of Cilium. label Jan 19, 2024
@harsimran-pabla
Copy link
Contributor Author

harsimran-pabla commented Jan 19, 2024 •
edited

Ginkgo test failure "Policy map sync fixed errors" : #29727

@danehans
Copy link
Contributor

Advertisement field got introduced into CiliumBGPFamily type when adding v2 APIs. This field is only required in new CiliumBGPPeerConfig structures.

The field is a pointer and includes the omitempty json stuct tag so the field is not required. Can you clarify?

@harsimran-pabla
Copy link
Contributor Author

Hi @danehans

The field is a pointer and includes the omitempty json stuct tag so the field is not required. Can you clarify?

I mean functionality of Advertisements is only required in BGPv2 CiliumBGPPeerConfig resource. It is indeed an optional field in that context. However, it should not be present at all in CiliumBGPPeeringPolicy which have references to CiliumBGPFamily.

@danehans danehans self-requested a review January 19, 2024 17:49
danehans
danehans approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

Thanks for providing clarification for the changes.

@harsimran-pabla harsimran-pabla mentioned this pull request Jan 19, 2024
Merged
@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 Jan 19, 2024
@aditighag aditighag added this pull request to the merge queue Jan 22, 2024
Merged via the queue into cilium:main with commit 04044c3 Jan 22, 2024
62 checks passed
@joamaki joamaki mentioned this pull request Jan 30, 2024
Merged
28 tasks
@harsimran-pabla harsimran-pabla mentioned this pull request Jan 30, 2024
Merged
@aanm
Copy link
Member

aanm commented Jan 31, 2024

@harsimran-pabla I assume this PR was backported in #30531? If yes, I've removed the needs-backport/1.15 label

@aanm aanm added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 31, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport done to v1.15 in v1.15.0-rc.1 Jan 31, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport done to v1.15 in v1.15.0-rc.1 Jan 31, 2024
@harsimran-pabla
Copy link
Contributor Author

@harsimran-pabla I assume this PR was backported in #30531? If yes, I've removed the needs-backport/1.15 label

Yes @aanm , thanks for linking PR and removing label. Missed few steps while manually doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danehans danehans danehans approved these changes

@christarazi christarazi christarazi approved these changes

@rastislavs rastislavs rastislavs approved these changes

Assignees
No one assigned
Labels
area/bgp backport-done/1.15 The backport for Cilium 1.15.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-blocker/1.15 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
v1.15.0-rc.1
Backport done to v1.15
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@harsimran-pabla @danehans @aanm @christarazi @rastislavs @aditighag