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

KEP-1880 Multiple Service CIDRs #3365

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Conversation

aojea
Copy link
Member

@aojea aojea commented Jun 8, 2022

  • One-line PR description: Services IP Ranges API: Initial implementation
  • Other comments:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 8, 2022
@k8s-ci-robot k8s-ci-robot requested review from dcbw and thockin June 8, 2022 17:24
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. labels Jun 8, 2022
// IPRangeSpec describe how the IPRange's specification looks like.
type IPRangeSpec struct {
// An IPv4 block in CIDR notation "10.0.0.0/8"
IPv4 string `json:"ipv4"`
Copy link

@squeed squeed Jun 8, 2022

Choose a reason for hiding this comment

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

Why bother having multiple families? What benefit does that add? To me, it seems like a single family per object would be easier to reason about.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find the discussion in the multiple clusterCidr API KEP #2594, but I made exactly the same point your are doing now, but changed my opinion.
Having different objects for IP family makes harder the reconciliation and validation, and you can always leave one empty for single stack

Copy link
Member

Choose a reason for hiding this comment

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

I also could not find the exact discussion, but IIRC there were a couple main arguments:

  1. for pod CIDRs we have to specify a per-node mask, and it was wierd to have potentially different masks per family
  2. configuration races - have 1 family per resource means it is possible to observe the config "half complete" and fail to allocate IPs of the intended family.

(1) does not apply here, exactly, but it is kind of similar - one could argue that you usually want service CIDRs to be the same size across families.

(2) I'm not super worried about this, personally - if someone asks for v6 spcifically, and v6 was not configured yet, it will fail syncronously. If they said preferDualStack, then they clearly don't NEED it.

That said, I see some value in forming this the same as the CCC KEP.

Copy link
Member Author

Choose a reason for hiding this comment

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

based on this new feedback and the desire of resizing and scale up and down ranges I think we should go with single family #3365 (comment)

To be realistic, once we remove the size limitation on IPv6 I don't think that people will need multiple ranges ono IPv6, a /64 is huge, the actual problem is with IPv4 and scale up or scale down service

Copy link
Member

Choose a reason for hiding this comment

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

The biggest arguments in favor of supporting both are:

  1. Consistency with CCC
  2. A single rnage object for apiserver default ranges
  3. No race if the user ever changes the config by adding a new range

I don't want to block on this - can we add a beta criterion to revisit this and see if we are still happy?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm renaming everything to be consistent with the multi-cluster-cidr KEP

@wojtek-t wojtek-t self-assigned this Jun 9, 2022
@thockin thockin self-assigned this Jun 10, 2022
@aojea aojea marked this pull request as draft June 11, 2022 10:28
@aojea aojea marked this pull request as ready for review June 11, 2022 15:44
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 11, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 11, 2022
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

All the PRR sections need love, too :)

keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-services-ip-ranges/README.md Outdated Show resolved Hide resolved
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

For further changes, please use new commits. We have confused the crap out of Github's UI (again)

@thockin thockin added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 19, 2022
@aojea aojea changed the title [WIP] KEP-1880 Services IP Ranges API KEP-1880 Multiple Service CIDRs Jun 20, 2022
keps/sig-network/1880-multiple-service-cidrs/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-multiple-service-cidrs/README.md Outdated Show resolved Hide resolved
keps/sig-network/1880-multiple-service-cidrs/README.md Outdated Show resolved Hide resolved

During this time, there is a chance that an apiserver tries to allocate this IPAddress, with a possible situation where
2 Services has the same IPAddress. In order to avoid it, the Allocator will not delete an IP from its local cache
until it verifies that the consumer associated to that IP has been deleted too.
Copy link
Member

Choose a reason for hiding this comment

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

I forget if apiserver ones run repair loop at startup? If it does, that's enough to rebuild the cache and (at least for services) prevent duplicate allocation.

I think

Copy link
Member Author

Choose a reason for hiding this comment

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

they do, there is a bootstrap controller that run in a post-start hook, this controller run the repair loops.


```
<<[UNRESOLVED keps/sig-network/3070-reserved-service-ip-range]>>
Option 1: Maintain the same formula and behavior per ServiceCIDRConfig
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I think we should do option 1 (no API) and see if we really need option 3. I suspect the impl will be very similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

"less is more" thing, right?

keps/sig-network/1880-multiple-service-cidrs/README.md Outdated Show resolved Hide resolved
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

I'm pretty LGTM on this. DO you want to manually squash (with a good message) or just let the bot do it?

@thockin
Copy link
Member

thockin commented Jun 21, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
@aojea
Copy link
Member Author

aojea commented Jun 21, 2022

I'm pretty LGTM on this. DO you want to manually squash (with a good message) or just let the bot do it?

the former 😄

Kubernetes networking is based fundamentally on two kind of networks,
the Pod network aka Cluster Network and the Service Network. These
networks are configured statically, using flags, causing a lot of
limitations and pain on users and administrators.

The Pod network is consumed by the CNI plugins, Kubernetes already provides
a simple IPAM that will be extended by the KEP-2593 "Multiple Cluster
CIDRs", allowing users to use an API to configure it.

This KEP provides a similar API for the Service Network, allowing users
to dynamically add more networks or resize the existing ones. It also
remove some of the limitations of current implementation.
@johnbelamaric
Copy link
Member

Ok, I think all my questions are answered, so once it has SIG approval I can give PRR approval

@thockin
Copy link
Member

thockin commented Jun 22, 2022

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2022
@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, johnbelamaric, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9efd962 into kubernetes:master Jun 22, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 22, 2022
pacoxu pushed a commit to pacoxu/enhancements that referenced this pull request Oct 12, 2022
* KEP-1880 Multiple Service CIDRs

Kubernetes networking is based fundamentally on two kind of networks,
the Pod network aka Cluster Network and the Service Network. These
networks are configured statically, using flags, causing a lot of
limitations and pain on users and administrators.

The Pod network is consumed by the CNI plugins, Kubernetes already provides
a simple IPAM that will be extended by the KEP-2593 "Multiple Cluster
CIDRs", allowing users to use an API to configure it.

This KEP provides a similar API for the Service Network, allowing users
to dynamically add more networks or resize the existing ones. It also
remove some of the limitations of current implementation.

* clarify serviceCIDRConfig IPAddress relation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants