[KEP] Adding ResourceID to the discovery API #656
Conversation
k8s-ci-robot
added
kind/kep
caesarxuchao
force-pushed
the
storage-hash
branch
from
5467bcf to
4693052
Compare
|
cc @liggitt |
caesarxuchao
changed the title
| ``` | ||
|
|
||
| If in a future Kubernetes release, replicasets are accessible via | ||
| `fancy-apps/v1` as well, requests sent to `fancy-apps/v1` will be left |
There was a problem hiding this comment.
This admission webhook case is new since we last spoke about this issue and somewhat compelling.
What would the server do with this knowledge? Check for other registrations and send fancy-app/v1 requests through conversion and then to the extistant webhooks? Do something else?
There was a problem hiding this comment.
Yeah, that's what I expect. The apiserver should send the converted requests to the webhooks. Defaults have already been applied to the objects before they reach the admission stage, so conversion should be possible.
[edit] Do you want me to further expand on this webhook design in this KEP?
There was a problem hiding this comment.
Note that since the webhook controller is part of the apiserver, so it doesn't need to go through the discovery doc to detect aliases, unless we encourage aggregated apiservers to provide aliasing endpoints for resources hosted by other apiservers.
| admins of the admission webhooks, so it is not always possible to coordinate | ||
| cluster upgrades with webhook configuration upgrades. | ||
|
|
||
| #### Inaccurate resource quota counting |
There was a problem hiding this comment.
I'm less worried about this one. It hasn't practically come up very often.
| multiple endpoints. | ||
|
|
||
| [garbage collector]:https://github.com/kubernetes/kubernetes/tree/master/pkg/controller/garbagecollector | ||
| [storage migrator]:https://github.com/kubernetes-sigs/kube-storage-version-migrator |
There was a problem hiding this comment.
I'm not very worried about this. A few extra migrate calls (which are infrequent to begin with) isn't that big a deal.
| migrator][] migrates the same objects multiple times if they are served via | ||
| multiple endpoints. | ||
|
|
||
| [garbage collector]:https://github.com/kubernetes/kubernetes/tree/master/pkg/controller/garbagecollector |
There was a problem hiding this comment.
GC today doesn't even collapse versions of the same groupresource, does it? If you're worried, you could get a lot of improvement just with that.
There was a problem hiding this comment.
I removed this example. GC's internal graph is keyed by UIDs, so it is collapsed already.
Network bandwidth is not a concern today either, because GC is using the shared informer, unless we want to cut GC into its own binary in the future.
There was a problem hiding this comment.
We can revisit the GC case if we decide to pursue kubernetes/kubernetes#72192 (comment).
|
I think the admission webhook case is the most compelling to me. What would do with this information in that case? Once we sort that out, I find the hash to be a reasonable implementation would would prevent abuse and avoids trying to build multi-way links. I remember @liggitt was interested in this too. It's an unversioned API, so it's hard to take a choice back. |
caesarxuchao
force-pushed
the
storage-hash
branch
3 times, most recently
from
c35da90 to
7aeca92
Compare
|
Thanks, @deads2k. I pushed another commit to improve the |
caesarxuchao
force-pushed
the
storage-hash
branch
from
7aeca92 to
a302735
Compare
|
You'll still need to pass API review when it comes, but this works for me. /lgtm holding for other reviewers (no other lgtm in here yet) |
k8s-ci-robot
added
do-not-merge/hold
| For aggregated resources, because their discovery doc is fully controlled | ||
| by the aggregated apiserver, the kube-apiserver has no means to validate their | ||
| `resourceID`. If the server is implemented with the generic apiserver library, | ||
| the `resourceID` will be `SHA256(<etcd key prefix>)`. |
There was a problem hiding this comment.
We chatted last time about adding some checks to avoid conflicts of core resources' resourceId from those aggregated resources.
There was a problem hiding this comment.
@lavalamp suggested that it's valid for aggregated apiservers to point to the core etcd. See kubernetes/community#2805 (comment).
| ## Graduation Criteria | ||
|
|
||
| Because the discovery API is a GA feature, to add the `resourceID` field, it | ||
| needs to be of GA quality. |
There was a problem hiding this comment.
similar to the StorageVersionHash, it would go through alpha/beta before GA, right? would you document it in the documentation?
There was a problem hiding this comment.
Good point. Added. PTAL. Thanks.
caesarxuchao
force-pushed
the
storage-hash
branch
from
a302735 to
c1b2f8b
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@deads2k, @yliaog had lgtm'ed, and @lavalamp only had "minor comments" with this KEP, so I'll remove the hold. /hold cancel @calebamiles can you help approve? Thanks. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@idvoretskyi @mattfarina could you help approve? Thank you. |
caesarxuchao
force-pushed
the
storage-hash
branch
from
c1b2f8b to
d6a9c14
Compare
k8s-ci-robot
added
approved
|
Caleb told me to remove the KEP number to reduce the approvals required. KEP number is deprecated. @yliaog could you re-apply the lgtm? Thanks. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, deads2k, yliaog 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 |
Proposing adding a
resourceIDfield to every resource's discovery doc. If two resource endpoints refer the same set of objects, theirresourceIDs are the same. This enables clients to detect if two resource endpoints are referring the same set of objects.Moved from kubernetes/community#2805.