genericapiserver: more dependency cutoffs #40216

sttts · 2017-01-20T13:53:00Z

cut-off pkg/api.Resource and friends - lgtm
authn plugins -> k8s.io/apiserver -
webhook authz plugin -> k8s.io/apiserver - lgtm
~~pkg/cert -> k8s.io/apimachinery (will rebase on @deads2k's PR also moving it)~~
split pkg/config into kubelet config merger and flags - lgtm
split feature gate between generic apiserver and kube - lgtm
move pkg/util/flag into k8s.io/apiserver - lgtm

k8s-reviewable · 2017-01-20T13:53:05Z

This change is

deads2k · 2017-01-20T13:57:00Z

genericapiserver: cut off pkg/api.Resource dependency lgtm

deads2k · 2017-01-20T13:57:45Z

pkg/kubeapiserver/authenticator/config.go

@@ -30,15 +30,15 @@ import (
 	"k8s.io/apiserver/pkg/authentication/request/x509"
 	"k8s.io/apiserver/pkg/authentication/token/tokenfile"
 	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/plugin/pkg/authenticator/password/keystone"


I wasn't expecting all these to move. We have an apiserver dependency or is it just kubeapiserver?

keystone is simple, no deps left

authorizers have more dependencies. Only moved webhook there.

deads2k · 2017-01-20T13:58:26Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go

 	_ "k8s.io/client-go/pkg/apis/authentication/install"
-	_ "k8s.io/kubernetes/pkg/apis/authentication/install"


good change

deads2k · 2017-01-20T13:59:27Z

genericapiserver: move authn plugins into k8s.io/apiserver - did you move all the authentication plugins for completeness in case someone else wanted to use them? I could buy that, but we don't have direct dependency on them, right?

sttts · 2017-01-20T14:01:01Z

@deads2k yes, they are optional, but independent enough to not harm, if you really build your kube-independent apiserver.

deads2k · 2017-01-20T14:01:03Z

General comment on the authentication packages. I think of plugins as optional and I don't think the webhook is optional and these are registering themselves for activation like normal plugins, so I'm not sure calling them plugins is appropriate. Certainly not blocking, but of note.

sttts · 2017-01-20T14:02:25Z

Right about "plugin". Didn't want to change that here. Maybe for a follow-up.

deads2k · 2017-01-20T15:07:09Z

pkg/util/cert: split and move generic code to k8s.io/apimachinery - Did this split client and server stuff? I don't think util/cert belongs in apimachinery. Its not a great fit for client-go either, but the client already has a lot of cruft. I'd like to try it in client-go first. A copy is even an option later if there's little overlap.

deads2k · 2017-01-20T15:07:33Z

genericapiserver: move authz webhook plugins into k8s.io/apiserver lgtm

deads2k · 2017-01-20T15:08:21Z

pkg/util: move flags from pkg/util/config to pkg/util/flags - fine

deads2k · 2017-01-20T15:10:13Z

pkg/api/validation/validation.go

@@ -2034,7 +2035,7 @@ func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *api.Pod
 		if !strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) {
 			continue
 		}
-		if !utilflag.DefaultFeatureGate.AppArmor() {
+		if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmor) {


This is really ugly and generally wrong. Not new in this pull but the link here should be snipped. This is an admission choice, not a general validation choice.

Will add a TODO. Didn't look at the meaning of the gates.

deads2k · 2017-01-20T15:12:37Z

pkg/flag: make feature gate extensible and split between generic and - lgtm. Probably want an issue for the bad validation.

k8s-ci-robot · 2017-01-24T17:31:46Z

Jenkins Bazel Build failed for commit f540fbfd8420a2dc0607ae24465c48eea1af4c1b. Full PR test history.

cc @sttts, your PR dashboard

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

sttts · 2017-01-24T17:38:22Z

@k8s-bot cri node e2e test this

sttts · 2017-01-24T17:38:38Z

@k8s-bot bazel test this

k8s-ci-robot · 2017-01-24T19:16:59Z

Jenkins GCI GKE smoke e2e failed for commit f540fbfd8420a2dc0607ae24465c48eea1af4c1b. Full PR test history.

cc @sttts, your PR dashboard

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

k8s-ci-robot · 2017-01-24T19:30:56Z

Jenkins verification failed for commit f540fbfd8420a2dc0607ae24465c48eea1af4c1b. Full PR test history.

cc @sttts, your PR dashboard

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

deads2k · 2017-01-24T19:33:33Z

Jenkins verification failed for commit f540fbf. Full PR test history.

@sttts keep your pull away from mine, just in case it contagious! :)

sttts · 2017-01-24T19:42:02Z

Looking forward to yet another rebase tomorrow morning. Trying to understand the flake, maybe this:

hack/make-rules/../../hack/verify-godeps.sh: line 68: KEEP_TMP: unbound variable

@eparis ^^

…kube

k8s-github-robot · 2017-01-24T23:53:53Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-01-25T00:26:00Z

Automatic merge from submit-queue (batch tested with PRs 39260, 40216, 40213, 40325, 40333)

sttts added area/apiserver kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 20, 2017

sttts added this to the 1.6 milestone Jan 20, 2017

sttts assigned deads2k Jan 20, 2017

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

deads2k reviewed Jan 20, 2017

View reviewed changes

sttts force-pushed the sttts-more-cutoffs branch 2 times, most recently from 949e31f to e588602 Compare January 23, 2017 09:03

sttts changed the title ~~[WIP] genericapiserver: more dependency cutoffs~~ genericapiserver: more dependency cutoffs Jan 23, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 23, 2017

sttts force-pushed the sttts-more-cutoffs branch 2 times, most recently from 7cb44ae to 7095be0 Compare January 23, 2017 09:25

sttts force-pushed the sttts-more-cutoffs branch from 561f6bc to f540fbf Compare January 24, 2017 17:14

sttts added 8 commits January 24, 2017 20:56

genericapiserver: cut off pkg/api.Resource dependency

f36a3d8

genericapiserver: move authn plugins into k8s.io/apiserver

4077e0b

genericapiserver: move authz webhook plugins into k8s.io/apiserver

4beba15

pkg/util: move flags from pkg/util/config to pkg/util/flags

56d60cf

pkg/flag: make feature gate extensible and split between generic and …

a6b2ebb

…kube

pkg/util/flag: move to k8s.io/apiserver

82826ec

Add TODO for AppArmor validation depending on feature gate

d00c9a3

Update generated files

2b8e938

sttts force-pushed the sttts-more-cutoffs branch from f540fbf to 2b8e938 Compare January 24, 2017 19:56

k8s-github-robot merged commit df42444 into kubernetes:master Jan 25, 2017

calebamiles modified the milestones: v1.6, 1.6 Feb 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

genericapiserver: more dependency cutoffs #40216

genericapiserver: more dependency cutoffs #40216

sttts commented Jan 20, 2017 •

edited

Loading

k8s-reviewable commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k Jan 20, 2017

sttts Jan 20, 2017

sttts Jan 20, 2017

deads2k Jan 20, 2017

deads2k commented Jan 20, 2017

sttts commented Jan 20, 2017

deads2k commented Jan 20, 2017

sttts commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k Jan 20, 2017

sttts Jan 20, 2017

deads2k commented Jan 20, 2017

k8s-ci-robot commented Jan 24, 2017

sttts commented Jan 24, 2017

sttts commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

deads2k commented Jan 24, 2017

sttts commented Jan 24, 2017

k8s-github-robot commented Jan 24, 2017

k8s-github-robot commented Jan 25, 2017

		_ "k8s.io/client-go/pkg/apis/authentication/install"
		_ "k8s.io/kubernetes/pkg/apis/authentication/install"

genericapiserver: more dependency cutoffs #40216

genericapiserver: more dependency cutoffs #40216

Conversation

sttts commented Jan 20, 2017 • edited Loading

k8s-reviewable commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k Jan 20, 2017

Choose a reason for hiding this comment

sttts Jan 20, 2017

Choose a reason for hiding this comment

sttts Jan 20, 2017

Choose a reason for hiding this comment

deads2k Jan 20, 2017

Choose a reason for hiding this comment

deads2k commented Jan 20, 2017

sttts commented Jan 20, 2017

deads2k commented Jan 20, 2017

sttts commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k commented Jan 20, 2017

deads2k Jan 20, 2017

Choose a reason for hiding this comment

sttts Jan 20, 2017

Choose a reason for hiding this comment

deads2k commented Jan 20, 2017

k8s-ci-robot commented Jan 24, 2017

sttts commented Jan 24, 2017

sttts commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

deads2k commented Jan 24, 2017

sttts commented Jan 24, 2017

k8s-github-robot commented Jan 24, 2017

k8s-github-robot commented Jan 25, 2017

sttts commented Jan 20, 2017 •

edited

Loading