Admission options spits out admission control by p0lyn0mial · Pull Request #45355 · kubernetes/kubernetes

p0lyn0mial · 2017-05-04T14:17:15Z

What this PR does / why we need it:

This PR adds ApplyTo method to AdmissionOptions struct. The method creates and initialises admission control to the server configuration.

Release note:

NONE

k8s-ci-robot · 2017-05-04T14:17:29Z

Hi @p0lyn0mial. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

k8s-reviewable · 2017-05-04T14:17:35Z

This change is

p0lyn0mial · 2017-05-04T14:22:38Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+// ApplyTo adds the admission chain to the server configuration
+// note that pluginIntializer is optional, a generic plugin intializer will always be provided and appended
+// to the list of plugin initializers.
+func (a *AdmissionOptions) ApplyTo(pluginInitializer admission.PluginInitializer, authz authorizer.Authorizer, restConfig *rest.Config, serverCfg *server.Config) error {


we could also provide two methods one which would require external plugin initialiser (AppyToWithPluginInitailizer) and the other without a plugin initialiser (ApplyTo).

What if the plugin initializer came last as a ... argument?

it's a good idea

p0lyn0mial · 2017-05-04T14:23:46Z

cmd/kube-apiserver/app/options/options.go

 	// Overwrite the default for storage data format.
 	s.Etcd.DefaultStorageMediaType = "application/vnd.kubernetes.protobuf"
+	// Set the default for admission plugins names
+	s.Admission.PluginsNames = []string{"AlwaysAdmit"}


maybe it is better to append ?

This is fine. It's my preference.

p0lyn0mial · 2017-05-04T14:25:46Z

/assign @deads2k

p0lyn0mial · 2017-05-04T14:30:06Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+	}
+
+	// initi generic plugin initalizer
+	if a.genericPluginInitializer == nil {


note that generic plugin initializer is initialized only once.

note that generic plugin initializer is initialized only once.

Doc on the method that we lazily init the generic plugin initializer down here.

p0lyn0mial · 2017-05-04T14:30:44Z

I will add some tests once accepted.

deads2k · 2017-05-04T14:34:37Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+		return fmt.Errorf("failed to read plugin config: %v", err)
+	}
+
+	// initi generic plugin initalizer


deads2k · 2017-05-04T14:41:58Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+		if err != nil {
+			return err
+		}
+		sharedInformers := informers.NewSharedInformerFactory(clientset, restConfig.Timeout)


@sttts @p0lyn0mial This has made our (existing) information flow problem really obvious. To work well, we need the ShareInformers to be shared for an entire API server. They're based on the loopback config, which is set as part of the servingOptions.ApplyTo. What if we had that ApplyTo actually construct the sharedInformers there and used the from outside like we do with the LoopbackClientConfig?

oh, I didn't realise it's an issue. I think this is due to maintaining cache state in informers - right ?

That means we would have to extend Config structure ? (add SharedInformers) ?

oh, I didn't realise it's an issue. I think this is due to maintaining cache state in informers - right ?

That means we would have to extend Config structure ? (add SharedInformers) ?

Yeah, it would. In the kube-apiserver we actually use informers (slightly different ones, but I'd expect most to use externals) to wire up our authenticator and authorizer: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go#L347-L360 .

We perform similar work in the kube-aggregator: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go#L111 and its going to come up again in kube-apiextensions-server.

okay, I will add "external" SharedInformers to Config.

I don't like this. Creating the loopback client in SecureServingOptions.ApplyTo is already too early IMO because of the effective listen port (which is set only when the server actually starts to listen). With shared imformers we would manifest this even further.

I don't like this. Creating the loopback client in SecureServingOptions.ApplyTo is already too early IMO because of the effective listen port (which is set only when the server actually starts to listen). With shared imformers we would manifest this even further.

This pull reflects the information flow we have. We use the client to create the informer to create the authentication, authorization, and admission filters which are used to create the server which is used to listen. I don't think I see that flow as likely change and I'd like to avoid having multiple informers floating around.

@sttts
We would create SharedInformers somewhere here https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go#L294

Normally kube-apiserver creates them a few lines later
https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go#L334

We should be safe to do that in SecureServingOptions.ApplyTo what do you think ?

liggitt · 2017-05-04T15:10:36Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

-	Control           string
-	ControlConfigFile string
-	Plugins           *admission.Plugins
+	PluginsNames             []string


PluginNames?

liggitt · 2017-05-04T15:11:18Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

-		Plugins: plugins,
-		Control: "AlwaysAdmit",
+		Plugins:                  plugins,
+		PluginsNames:             []string{},


seems a little odd to specify both of these and not have names in sync with plugins

seems a little odd to specify both of these and not have names in sync with plugins

I don't understand your comment. One is a registry of all available admission plugins. The other is an ordered list of the chain you want. They are not equivalent.

ah, I misread, I thought the names got initialized into the plugins

sttts · 2017-05-08T12:33:50Z

@p0lyn0mial I have created a PR against your branch: p0lyn0mial#1. We have wiring for the internal shared informers already. I have removed your new GenericConfig field and put the new external informers next to the internal informer code. If we think this wiring is ugly, let's do a cleanup as a follow-up. But at least now it is uniform (-ly ugly).

deads2k · 2017-05-08T12:50:03Z

@sttts I described the flow of information like this:

This pull reflects the information flow we have. We use the client to create the informer to create the authentication, authorization, and admission filters which are used to create the server which is used to listen. I don't think I see that flow as likely change and I'd like to avoid having multiple informers floating around.

Can you describe an alternative that doesn't involve mutatable authentication, authorization, and admission? You pull doesn't resolve the problem and simply ensures that every API server built will have to wire these pieces together in order to get core, non-optional admission (namespace enforcement) working. That seems burdensome and a likely point of failure.

sttts · 2017-05-08T13:07:41Z

@deads2k I will look into this cycle issue in a follow-up. I don't want to block this PR because of that.

deads2k · 2017-05-08T14:47:52Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+
+// ApplyTo adds the admission chain to the server configuration
+// the method lazily initializes a generic plugin that is appended to the list of pluginInitializers
+func (a *AdmissionOptions) ApplyTo(authz authorizer.Authorizer, restConfig *rest.Config, serverCfg *server.Config, sharedInformers informers.SharedInformerFactory, pluginInitializers ...admission.PluginInitializer) error {


I think you can can get the authorizer, loopbackconfig, and informers from the server.Config, so they don't need separate args.

Indicate what is used from the genericconfig in the method documentation.

deads2k · 2017-05-08T14:48:31Z

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

+	}
+
+	// init generic plugin initalizer
+	if a.genericPluginInitializer == nil {


Since you're re-using the informers from the config, this block no longer needs to be conditional.

I think we can go even one step further. genericPluginInitializer doesn't have to be a member variable.

I think we can go even one step further. genericPluginInitializer doesn't have to be a member variable.

true

deads2k · 2017-05-08T14:52:40Z

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

 	// It comes after all filters and the API handling
 	FallThroughHandler *mux.PathRecorderMux
+	// SharedInformerFactory provides shared informers for resources
+	SharedInformerFactory informers.SharedInformerFactory


You need to add a poststarthook to call .Start on these about here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/config.go#L407 . Maybe a name like with something like generic-apiserver-start-informers

so basically everything that needs to be run in a separate goroutine goes through hooks ?

Most things. It makes them inspectable. In this case, it makes the lifecycle of the informer factory the responsibility of the library.

deads2k · 2017-05-08T15:09:16Z

@p0lyn0mial a few comments. Go ahead and squash into one or two commits.

@p0lyn0mial

Automatic merge from submit-queue plumb stopch to post start hook index since many of them are starting go funcs Many post-start hooks require a stop channel to properly terminate their go funcs. @p0lyn0mial I think you need this for #45355 ptal. @ncdc per request @sttts can you review too since Andy is out?

deads2k · 2017-05-12T15:02:57Z

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

I think we can tolerate this being nil to start. It's a little weird, but I'm more interested in getting it wired than chasing an initialization flow through federation unit tests.

alright, that simplifies a lot.

deads2k · 2017-05-12T15:03:29Z

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

Add the nil check here so we don't register the post start hook at all if its missing.

deads2k · 2017-05-12T15:03:34Z

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

deads2k · 2017-05-12T15:03:44Z

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

isHookRegistered

ApplyTo adds the admission chain to the server configuration the method lazily initializes a generic plugin that is appended to the list of pluginInitializers. apiserver.Config will hold an instance of SharedInformerFactory to ensure we only have once instance. The field will be initialized in apisever.SecureServingOptions

p0lyn0mial · 2017-05-14T10:04:23Z

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

 	defer etcdserver.Terminate(t)

 	assert.NotNil(config.SwaggerConfig)
-	// assert.NotNil(config.OpenAPIConfig)


I removed commented lines. Tried to uncomment but the test failed.

p0lyn0mial · 2017-05-14T10:06:54Z

squashed and ready to be merged.

deads2k · 2017-05-15T11:41:18Z

@k8s-bot ok to test

deads2k · 2017-05-15T15:34:00Z

Looks like hack/update-bazel.sh and hack/update-staging-godeps.sh (no idea why that one changed...)

deads2k · 2017-05-16T14:22:23Z

/lgtm

k8s-github-robot · 2017-05-16T14:22:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial

Details

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [deads2k]
~~federation/cmd/federation-apiserver/OWNERS~~ [deads2k]
~~pkg/master/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/pkg/server/OWNERS~~ [deads2k]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-16T16:11:53Z

Automatic merge from submit-queue (batch tested with PRs 45408, 45355, 45528)

k8s-github-robot · 2017-05-16T16:15:31Z

@p0lyn0mial PR needs rebase

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 4, 2017

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

k8s-github-robot assigned caesarxuchao and liggitt May 4, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2017

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

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 4, 2017

p0lyn0mial commented May 4, 2017

View reviewed changes

k8s-ci-robot assigned deads2k May 4, 2017

p0lyn0mial commented May 4, 2017

View reviewed changes

deads2k reviewed May 4, 2017

View reviewed changes

liggitt reviewed May 4, 2017

View reviewed changes

deads2k mentioned this pull request May 8, 2017

Unify internal and external informer factories p0lyn0mial/kubernetes#1

Open

deads2k reviewed May 8, 2017

View reviewed changes

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

deads2k reviewed May 8, 2017

View reviewed changes

deads2k mentioned this pull request May 8, 2017

plumb stopch to post start hook index since many of them are starting go funcs #45495

Merged

p0lyn0mial force-pushed the admission_options_spits_out_admission_control branch from 319f1d3 to 4818f16 Compare May 11, 2017 22:14

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

deads2k reviewed May 12, 2017

View reviewed changes

staging/src/k8s.io/apiserver/pkg/server/config.go Outdated

Copy link

Contributor

deads2k May 12, 2017

Choose a reason for hiding this comment

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

it merged.

deads2k reviewed May 12, 2017

View reviewed changes

staging/src/k8s.io/apiserver/pkg/server/hooks.go Outdated

Copy link

Contributor

deads2k May 12, 2017

Choose a reason for hiding this comment

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

isHookRegistered

p0lyn0mial force-pushed the admission_options_spits_out_admission_control branch from 4818f16 to 8cea69a Compare May 14, 2017 09:58

p0lyn0mial commented May 14, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 15, 2017

liggitt removed their assignment May 15, 2017

bazel update

7b0950b

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2017

k8s-github-robot merged commit ece4124 into kubernetes:master May 16, 2017

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

Conversation

p0lyn0mial commented May 4, 2017

Uh oh!

k8s-ci-robot commented May 4, 2017

Uh oh!

k8s-reviewable commented May 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented May 4, 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!

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!

liggitt May 4, 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!

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

Uh oh!

Uh oh!

deads2k commented May 8, 2017

Uh oh!

sttts commented May 8, 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!

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!

deads2k commented May 8, 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

liggitt May 4, 2017 •

edited

Loading

sttts commented May 8, 2017 •

edited

Loading

deads2k commented May 8, 2017 •

edited

Loading