-
Notifications
You must be signed in to change notification settings - Fork 366
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
Add ability to specify a list of ipBlocks in crd/v1alpha2 ClusterGroup #1993
Conversation
9f981e0
to
a7c60f7
Compare
Codecov Report
@@ Coverage Diff @@
## main #1993 +/- ##
==========================================
+ Coverage 55.97% 57.35% +1.37%
==========================================
Files 262 262
Lines 19468 19473 +5
==========================================
+ Hits 10898 11168 +270
+ Misses 7407 7126 -281
- Partials 1163 1179 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically with an API change such as this one, the server needs to ensure that the first member of the new ipBlocks
field matches ipBlock
. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility. The rationale for this is that a CG created by a new client that includes a single IPBlock (using ipBlocks
) can still be accessed by older clients. Similarly, CG objects created by an older client (with ipBlock
) can be accessed by a new client which only looks at the new ipBlocks
field. However, given that this is a CRD and not an aggregated API, I am not sure there is a good way of doing it automatically. I am not sure what the best way to handle this is (cc @tnqn):
- considering this is an alpha API, it may be acceptable to ignore this altogether
- or instead of
ipBlock
andipBlocks
, we can haveipBlock
andextraIPBlocks
, and mandate thatipBlock
is always set first - or we can have a mutating webhook, but the cost seems high
I'm trying to understand the notion of |
The clients are the Antrea Controller and the application / user creating policies and CGs
I would tend to agree, since it changes the semantics of the CG. But then again, reading a CG and seeing an empty I would say the use case I was looking at was a new client being able to handle previously created CG CRs by only considering the new Given that, and given that I believe I am starting to overthink this, I think the current solution is fine. A client should only need to look at its own CG CRs, and once upgraded should consider both the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. See if @tnqn wants to review.
rebased on |
/test-all rebased to resolve merge conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. However there are some compilation issues.
Add controller logic to handle ipBlocks Add E2E tests for ipBlocks in CG Add validation and documentation
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds a new
ipBlocks
field for the ClusterGroup resource. It allows users to group a list of CIDRs in a ClusterGroup, instead of a single CIDR in theipBlock
field. For a single ClusterGroup,ipBlock
andipBlocks
should not be set at the same time. Users can continue to create ClusterGroups withipBlock
. This field however will be deprecated in the next API upgrade for ClusterGroup. See #2008.