bgpv1: Consolidate CRD API to follow K8s API Conventions #26040
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
rastislavs
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
rastislavs
commented
Jun 8, 2023
rastislavs
commented
Jun 8, 2023
rastislavs
commented
Jun 8, 2023
|
cc @danehans |
harsimran-pabla
requested changes
Jun 8, 2023
rastislavs
force-pushed
the
bgp-api-refactor
branch
2 times, most recently
from
9789bd6
to
a811886
Compare
rastislavs
force-pushed
the
bgp-api-refactor
branch
2 times, most recently
from
4a064eb
to
aa3458d
Compare
rastislavs
commented
Jun 12, 2023
rastislavs
force-pushed
the
bgp-api-refactor
branch
2 times, most recently
from
e7faf37
to
3fa6902
Compare
|
Thanks for all the comments so far @YutaroHayakawa & @harsimran-pabla! As I addressed them, I squashed relevant commits and moved this PR out of draft - ready for the final review! |
danehans
reviewed
Jun 12, 2023
rastislavs
force-pushed
the
bgp-api-refactor
branch
2 times, most recently
from
72f1cdd
to
bc81751
Compare
rastislavs
force-pushed
the
bgp-api-refactor
branch
from
68c2603
to
7d9c4e0
Compare
|
/test (rebased to fetch latest CI changes/fixes) |
This was referenced Jun 14, 2023
YutaroHayakawa
added
the
ready-to-merge
|
API change is a bit of description changes. I think it's ready to merge. |
maintainer-s-little-helper
bot
removed
the
ready-to-merge
rastislavs
added
kind/enhancement
danehans
approved these changes
Jun 14, 2023
maintainer-s-little-helper
bot
removed
the
ready-to-merge
rastislavs
added
ready-to-merge
Modifies the type of recently added CRD fields of type `meta/v1.Duration` to integer type and their name to follow the K8s API Conventions rule: "Duration fields must be represented as integer fields with units being part of the field name (e.g. leaseDurationSeconds). We don't use Duration in the API since that would require clients to implement go-compatible parsing." Also enables defaulting for these fields on apiserver by using the kubebuilder:default tags (for static defaults). This includes the KeepaliveTimeSeconds field, which was defaulted at runtime to 1/3 of HoldTimeSeconds. As per K8s API Conventions, static defaults are preferred. It also makes the value more transparent to the user. Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
This allows the default value to be versioned with the API. It also provides consistency with the kubebuilder tags that define the default values - both are kept in the same source file. Also moves timers validation logic to API package to co-locate the validation logic for API constraints that cannot be expressed with kubebuilder markers with the API itself. Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
Modifies the types of CRD fileds to follow the K8s API Conventions rules: - "Use pointer types for all optional fields that do not have built-in nil value" - "Do not use unsigned integers, due to inconsistent support across languages and libraries" - "All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int" Also changes the types of the existing ASN CRD fileds to int64 to satisfy the above rules. Defaults ExportPodCIDR field of CiliumBGPVirtualRouter to false explicitly for consistency with other optional fields. As the default behavior remains the same, this is a backward-compatible change. Pointer types can be directly dereferenced in the reconcilers code. To allow that we provide defaulting methods for our API types, that can be used from the controllers / unit tests. Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
rastislavs
force-pushed
the
bgp-api-refactor
branch
from
7d9c4e0
to
d857bb1
Compare
Add unit tests to newly added SetDefaults() and Validate() methods of the BGP CRD types. Do not perform detailed defaulting / validation checks in unit tests of other packages, to have that testing logic co-located within a single package. Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
rastislavs
force-pushed
the
bgp-api-refactor
branch
from
d857bb1
to
ff34296
Compare
|
/test |
Rebased due to a conflicting change in the main branch. All green after the rebase - marking as ready to merge again. |
rastislavs
added
the
ready-to-merge
aanm
requested changes
Jun 15, 2023
maintainer-s-little-helper
bot
removed
the
ready-to-merge
I believe this PR does not introduce any breaking change to the types that were already released in Cilium 1.13. Most of the changes in this PR are on the fields that were added as recent "to be released" features in the past weeks (listed in the PR description). There are only 3 backward compatible changes on the previously released CRD fields - copying from this PR's description:
EDIT: so there is no need for any upgrade path, this change is transparent for existing users. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to consolidate the BGP control plane k8s API to follow the K8s API Conventions.
Although it modifies some filed names, it does it only for fields that have been added recently (not yet released in any Cilium version) as part of:
Already released API fields are modified only in a backward compatible way (e.g. changed internal type to a pointer, but no type change in the generated
CustomResourceDefinition). The only changes of already released API in the generated ciliumbgppeeringpolicies.yaml are:exportPodCIDR: added default value (false, so not changing the default behaviour)localASN+peerASN: addedformatannotation, but without the change of the underlyingtypeThe changes are described in more detail in individual commits.
Although the diff looks pretty big, most of the changes are in the unit / component tests, where the API is actually used.
Fixes #25871