Dynamic registration prototype by caesarxuchao · Pull Request #46294 · kubernetes/kubernetes

caesarxuchao · 2017-05-23T17:33:13Z

Implementing the api proposed in kubernetes/community#611.
Wiring the code to serve the api via apiserver.

Adding admissionregistration API group which enables dynamic registration of initializers and external admission webhooks. It is an alpha feature.

caesarxuchao · 2017-05-23T20:38:38Z

staging/src/k8s.io/apiserver/pkg/registry/apiserver/scheme/register.go

The registry needs Registry, Scheme, Codecs etc. Because we are in k8s.io/apiserver, we cannot import k8s.io/kubernetes/pkg/api, so i have to create this package.

caesarxuchao · 2017-05-23T20:39:54Z

staging/src/k8s.io/apiserver/pkg/registry/apiserver/scheme/register.go

We need to register the unversioned types to the Scheme, otherwise the API discovery endpoints won't work.

caesarxuchao · 2017-05-23T20:41:18Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

I think we can deprecate this field.

Without commenting it out, the API installation of apisever/v1alpha1 will fail with "no type ExportOptions is registered for v1". This error doesn't occur when installing other k8s APIs because their registry use the k8s.io/kubernetes/pkg/api.Scheme, which installs these metav1 types to all versions, including "v1".

Hrm - we will need this in the future - it's intended to control what version of generic APIs are defaulted by the server, like the forthcoming Table.

Meta types should be registered in your group for now - is there a reason you don't want to?

caesarxuchao · 2017-05-23T21:51:51Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

Shall we call it Verbs or Operations? rbac calls is Verbs, and the authorizer interface call it Verbs, but the admission interface calls it Operations

@whitlockjc and I lean towards Operations.

lavalamp · 2017-05-23T22:11:41Z

staging/src/k8s.io/apiserver/pkg/registry/apiserver/externaladmissionhookconfiguration/doc.go

Wow I really object to having a specific API type living in the generic apiserver's registry package. @deads2k did you really want this? ...

OK, Chao is going to move these registry classes into the main repo.

Wow I really object to having a specific API type living in the generic apiserver's registry package. @deads2k did you really want this? ..

I really want to have separate buckets for "generic API server types" and "kube-apiserver types". The types are two distinct buckets. It's easy to have separate buckets and later move them around. It's really hard to put it all into one bucket and then later try to separate some stuff that was mixed in the bucket. Because of that, putting them here is much easier to change than putting them in the main repo.

The first thing you're going to do is claim that the apiserver needs them and so you'll group them in a k8s.io/apis bucket.

What about an alternative where we actually model it as a separate API server that is added to the filter chain like we've done with kube-apiextensions-server? The integration ends up clean and these stay out of kube-apiserver. Until we get the third bucket, the types can live here to avoid a cyclical dependency, but my concerns about "making mud" in the shared API would be ameliorated and you'd have a clear delineation for a proper move once the cycle was resolved.

These are not generic apiserver types, though. The registration api is only going to be hosted by the main apiserver.

But are not the same category of thing as the bucket you are going to place them into for the sake of expedience and I sincerely doubt that any separation will ever be made, so this would live forever alongside jobs, hpas, pod disruption budgets, and a host of other APIs it doesn't belong with.

I think its a real shame to do this, you can't easily unmake soup, but ultimately I'm not willing to sacrifice the feature to keep separate things separate.

These are not generic apiserver types, though. The registration api is only going to be hosted by the main apiserver.

[edit] I think this is a strong argument to not put the api in k8s.io/apiserver

this would live forever alongside jobs, hpas, pod disruption budgets,

I agree. Admission configuration doesn't blend with jobs, hpa, etc. But currently we don't have another place to put API that is served by the apiserver.

I agree. Admission configuration doesn't blend with jobs, hpa, etc. But currently we don't have another place to put API that is served by the apiserver.

I'd prefer to see that place built for something net-new than continue a pattern of placing largely unrelated APIs into the same bucket. It's net new, so no one is going to complain that their cheese moved. If we can't make "the right" place, keeping it logically separate in a less than ideal location (here), makes it possible to move this later with minimal complaint. If we go in the other direction (put it into k8s.io/kubernetes/pkg/apis), then we are extremely unlikely to correct it later.

new place would be best, but we don't have time to build a new place IMO. As long as it's in its own group...

IMO this is a level 2 code separation concern, and we still haven't finished the level 1 code separation.

caesarxuchao · 2017-05-24T01:32:41Z

defaulter-gen never worked for any staging repo... if we decided to keep the api in k8s.io/apiserver, we'll need to update k8s.io/gengo, and then vendor it back.

caesarxuchao · 2017-05-24T02:15:43Z

Can I get some reviews for the "api" commit?

Other commits will change if we decide to move the api to k8s.io/kubernetes.

smarterclayton · 2017-05-24T18:16:00Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

Put this in doc.go

smarterclayton · 2017-05-24T18:16:19Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

smarterclayton · 2017-05-24T18:23:15Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

Comment that operations must match admission.Attributes?

lavalamp

comments on validation

lavalamp · 2017-05-24T17:44:43Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

update what about it?

@deads2k the code history is lost during the move to staging. It looks like we used to have a completely different criteria on QualifiedName. Shall I create my own function?

lavalamp · 2017-05-24T17:47:20Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go

please implement this todo

lavalamp · 2017-05-24T17:48:03Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go

why not? do we really need to distinguish between empty and nil?

lavalamp · 2017-05-24T17:49:08Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go

(maybe 'empty or nil'?)

yeah, should be empty or nil. Done.

caesarxuchao · 2017-05-24T21:46:16Z

Addressed @lavalamp and @smarterclayton's comments in the last two commits. PTAL.

The remaining disagreement is on where to put the API group: #46294 (comment).

caesarxuchao · 2017-05-26T02:59:38Z

Tests should be fixed. Adding back the lgtm label.

…wagger-spec.sh

caesarxuchao · 2017-05-26T07:15:40Z

Rebased.

k8s-ci-robot · 2017-05-26T08:06:10Z

@caesarxuchao: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`89e506c`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`
pull-kubernetes-cross	`89e506c`	link	`@k8s-bot pull-kubernetes-cross test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

caesarxuchao · 2017-05-26T16:35:30Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

lavalamp · 2017-05-26T19:40:21Z

I0526 00:56:10.391] # k8s.io/kubernetes/test/e2e_node
I0526 00:56:10.391] test/e2e_node/summary_test.go:138: constant 100000000000 overflows int

smarterclayton · 2017-05-26T19:53:51Z

Hahahaha

…

On Fri, May 26, 2017 at 3:40 PM, Daniel Smith ***@***.***> wrote: I0526 00:56:10.391] # k8s.io/kubernetes/test/e2e_node I0526 00:56:10.391] test/e2e_node/summary_test.go:138: constant 100000000000 overflows int — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxLdV-T45w5ODgqITbamrN1FZZxaks5r9yqvgaJpZM4NkCBS> .

k8s-github-robot · 2017-05-26T19:58:06Z

Automatic merge from submit-queue (batch tested with PRs 46383, 45645, 45923, 44884, 46294)

smarterclayton · 2017-05-31T02:04:45Z

Generated client has namespace generation, but should be non-namespaced.

caesarxuchao · 2017-05-31T02:18:35Z

Eh, right. Will send a fix tonight.

smarterclayton · 2017-05-31T02:21:52Z

Doesn't block me, so not critical tonight.

caesarxuchao · 2017-05-31T02:30:46Z

Sent #46668.

caesarxuchao self-assigned this May 23, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 23, 2017

caesarxuchao force-pushed the dynamic-registration-prototype branch from c7343ad to d2f74ae Compare May 23, 2017 20:18

caesarxuchao mentioned this pull request May 23, 2017

Configuration manager for dynamic admission control registration #46302

Merged

caesarxuchao force-pushed the dynamic-registration-prototype branch 2 times, most recently from 4ce5527 to 5274cdb Compare May 23, 2017 20:36

caesarxuchao commented May 23, 2017

View reviewed changes

caesarxuchao force-pushed the dynamic-registration-prototype branch from 5274cdb to 5a4e9b9 Compare May 23, 2017 20:54

caesarxuchao commented May 23, 2017

View reviewed changes

lavalamp reviewed May 23, 2017

View reviewed changes

caesarxuchao force-pushed the dynamic-registration-prototype branch from 8067154 to 48c205e Compare May 24, 2017 02:10

caesarxuchao changed the title ~~[WIP] Dynamic registration prototype~~ Dynamic registration prototype May 24, 2017

caesarxuchao assigned lavalamp, smarterclayton and deads2k and unassigned caesarxuchao May 24, 2017

smarterclayton reviewed May 24, 2017

View reviewed changes

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go Outdated

Copy link
Copy Markdown

Contributor

smarterclayton May 24, 2017

Choose a reason for hiding this comment

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

Put this in doc.go

smarterclayton reviewed May 24, 2017

View reviewed changes

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go Outdated

Copy link
Copy Markdown

Contributor

smarterclayton May 24, 2017

Choose a reason for hiding this comment

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

typo

smarterclayton reviewed May 24, 2017

View reviewed changes

lavalamp reviewed May 24, 2017

View reviewed changes

caesarxuchao mentioned this pull request May 25, 2017

fix default-gen's output path for packages in vendor directory kubernetes/gengo#59

Closed

caesarxuchao force-pushed the dynamic-registration-prototype branch 2 times, most recently from 2a7bbd8 to 2bb97b3 Compare May 25, 2017 04:37

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 26, 2017

caesarxuchao removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 26, 2017

caesarxuchao force-pushed the dynamic-registration-prototype branch from eff4776 to dd538b0 Compare May 26, 2017 02:37

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

caesarxuchao force-pushed the dynamic-registration-prototype branch from dd538b0 to 21bb3ad Compare May 26, 2017 06:54

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 26, 2017

Chao Xu added 6 commits May 25, 2017 23:55

api

6b5bd19

go-to-protobuf take care of admissionregistration

1500017

enable all available api versions when starting apiserver in update-s…

bc75978

…wagger-spec.sh

serve the api in kube-apiserver

262799f

generated clients

bc9b305

generated

89e506c

caesarxuchao force-pushed the dynamic-registration-prototype branch from 21bb3ad to 89e506c Compare May 26, 2017 07:14

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

k8s-github-robot merged commit 7bc6da0 into kubernetes:master May 26, 2017

sttts mentioned this pull request May 31, 2017

hack/update-swagger-spec.sh: use posix regex syntax #46698

Merged

Conversation

caesarxuchao commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 24, 2017

Uh oh!

caesarxuchao commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 24, 2017

Uh oh!

caesarxuchao commented May 26, 2017

Uh oh!

caesarxuchao commented May 26, 2017

Uh oh!

k8s-ci-robot commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caesarxuchao commented May 23, 2017 •

edited

Loading

caesarxuchao May 23, 2017 •

edited

Loading

caesarxuchao May 24, 2017 •

edited

Loading

k8s-ci-robot commented May 26, 2017 •

edited

Loading