Cleanup genericapiserver handler chain #33164

sttts · 2016-09-21T09:42:44Z

move generic (api independent) handler filters to pkg/genericapiserver/filters
entangle genericapiserver.New()
unify signature of all handler filters (also those in pkg/apiserver)

This change is

deads2k · 2016-09-21T12:08:13Z

pkg/api/requestcontext.go

-		}), nil
-	} else {
-		return handler, errors.New("Unknown RequestContextMapper implementation.")
+func WithRequestContext(handler http.Handler, mapper RequestContextMapper) http.Handler {


Any idea why it's in this package? Doesn't have to be this pull, but unless there's a strong reason, I'd like it moved eventually.

Had the same question. Also the Context interface looks api independent. Doesn't have to be so deep in the package hierarchy. A good topic for a follow-up PR.

Had the same question. Also the Context interface looks api independent. Doesn't have to be so deep in the package hierarchy. A good topic for a follow-up PR.

agreed.

deads2k · 2016-09-21T12:12:40Z

pkg/apiserver/handlers.go

@@ -462,7 +144,7 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib
 }

 // WithAuthorizationCheck passes all authorized requests on to handler, and returns a forbidden error otherwise.
-func WithAuthorizationCheck(handler http.Handler, getAttribs RequestAttributeGetter, a authorizer.Authorizer) http.Handler {
+func WithAuthorization(handler http.Handler, getAttribs RequestAttributeGetter, a authorizer.Authorizer) http.Handler {


If a == nil, let's be friendly, skip adding this, and glog a warning.

Most filters just return the passed handler when the parameters are nil. Will do the same here.

deads2k · 2016-09-21T12:14:20Z

pkg/genericapiserver/config.go

@@ -337,18 +346,64 @@ func (c Config) New() (*GenericAPIServer, error) {
 		})
 	}

+	if len(c.AuditLogPath) != 0 {


Let's add to our running issue. I think all the audit stuff should be gathered up into a pointer to a struct.

Not in this pull.

deads2k · 2016-09-21T12:15:43Z

pkg/genericapiserver/config.go

+}
+
+func (s *GenericAPIServer) buildHandlerChains(c *Config, handler http.Handler) (secure http.Handler, insecure http.Handler) {
+	longRunningRE := regexp.MustCompile(c.LongRunningRequestRE)


Can't we push these into WithTimeoutForNonLongRunningRequests?

yes, makes sense

deads2k · 2016-09-21T12:16:01Z

pkg/genericapiserver/config.go

+	longRunningFunc := genericfilters.BasicLongRunningRequestCheck(longRunningRE, map[string]string{"watch": "true"})
+
+	// common filters
+	handler = genericfilters.WithCORS(handler, allowedOriginRegexps(c.CorsAllowedOriginList), nil, nil, "true")


common for secure and insecure.

common for secure and insecure.

Yeah, I was suggesting a rename. Doesn't really matter to me.

deads2k · 2016-09-21T12:16:51Z

pkg/genericapiserver/config.go

+	attributeGetter := apiserver.NewRequestAttributeGetter(c.RequestContextMapper, s.NewRequestInfoResolver())
+	secure = handler
+	secure = apiserver.WithAuthorization(secure, attributeGetter, c.Authorizer)
+	secure = audit.WithAudit(secure, attributeGetter, s.auditWriter) // before impersonation to read original user


Isn't this after impersonation right now?

Good spot. In origin it's correct.

deads2k · 2016-09-21T12:17:50Z

pkg/genericapiserver/config.go

+	return s, nil
+}
+
+func (s *GenericAPIServer) buildHandlerChains(c *Config, handler http.Handler) (secure http.Handler, insecure http.Handler) {


So much cleaner. TODO to type the function and take it as an optional struct value in genericapiserver.

deads2k · 2016-09-21T12:18:51Z

pkg/genericapiserver/config.go

+	return
+}
+
+func allowedOriginRegexps(allowedOrigins []string) []*regexp.Regexp {


Seems like this should live in your cors file

deads2k · 2016-09-21T12:19:33Z

pkg/genericapiserver/filters/cors.go

+	"strings"
+)
+
+// TODO: use restful.CrossOriginResourceSharing


I don't think we're likely to do this.

Somebody somewhen was planning to do that :)

deads2k · 2016-09-21T12:19:43Z

pkg/genericapiserver/filters/cors.go

+// WithCORS is a simple CORS implementation that wraps an http Handler.
+// Pass nil for allowedMethods and allowedHeaders to use the defaults. If allowedOriginPatterns
+// is empty or nil, no CORS support is installed.
+func WithCORS(handler http.Handler, allowedOriginPatterns []*regexp.Regexp, allowedMethods []string, allowedHeaders []string, allowCredentials string) http.Handler {


straight move?

deads2k · 2016-09-21T12:23:20Z

Assuming most of the code is straight moves, I've got a few minor comments, but this looks a lot better.

Any specific reason its still WIP? I'd be fine tying off this chunk without adding more to it.

deads2k · 2016-09-21T12:45:16Z

lgtm. squash and tag when you're happy with it.

soltysh · 2016-09-21T12:45:17Z

pkg/genericapiserver/config.go

 	secure = apiserver.WithImpersonation(secure, c.RequestContextMapper, c.Authorizer)
+	secure = audit.WithAudit(secure, attributeGetter, s.auditWriter) // before impersonation to read original user


sttts · 2016-09-22T11:29:08Z

Squashed. Waiting for #33095 to merge.

@sttts

Automatic merge from submit-queue Remove closing audit log file and add error check when writing to audit This picks the order fix from #33164. Additionally I've removed entirely closing the log file, since it didn't make sense where it was. I've also added error checks when actually writing to audit logs. @sttts ptal **1.4 justification:** Risk: the code only runs if auditing is enabled with an apiserver flag. So the risk is low. Rollback: nothing should depend on this Cost: the auditing feature is broken because the impersonation filter is applied before and you might not see the proper user when using `--as` flag. Additionally no errors are logged if writing to audit fails.

sttts · 2016-09-23T06:36:59Z

Rebased.

k8s-ci-robot · 2016-09-23T07:05:58Z

Jenkins verification failed for commit 11a608a6599ec1c7c705906b89f53798e8a71774. Full PR test history.

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.

k8s-github-robot · 2016-09-23T11:50:40Z

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

k8s-github-robot · 2016-09-23T12:26:51Z

Automatic merge from submit-queue

sttts added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/apiserver labels Sep 21, 2016

sttts assigned deads2k Sep 21, 2016

googlebot added the cla: yes label Sep 21, 2016

sttts mentioned this pull request Sep 21, 2016

Move TimeoutHandler+MaxInFlightLimit to Config.New() #33095

Closed

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Sep 21, 2016

sttts force-pushed the sttts-handler-chain-cleanup branch from f19ef45 to ccdfc49 Compare September 21, 2016 09:52

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 21, 2016

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

sttts changed the title ~~Cleanup genericapiserver handler chain~~ WIP: Cleanup genericapiserver handler chain Sep 21, 2016

deads2k reviewed Sep 21, 2016

View reviewed changes

soltysh reviewed Sep 21, 2016

View reviewed changes

sttts changed the title ~~WIP: Cleanup genericapiserver handler chain~~ Cleanup genericapiserver handler chain Sep 21, 2016

soltysh mentioned this pull request Sep 21, 2016

Remove closing audit log file and add error check when writing to audit #33170

Merged

sttts force-pushed the sttts-handler-chain-cleanup branch from 3a31ed6 to daef268 Compare September 21, 2016 13:56

sttts mentioned this pull request Sep 21, 2016

Epic: make genericapiserver generic #32792

Closed

20 tasks

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2016

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

sttts force-pushed the sttts-handler-chain-cleanup branch from 0b6ad8d to 11a608a Compare September 23, 2016 06:36

sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 23, 2016

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

sttts force-pushed the sttts-handler-chain-cleanup branch from 11a608a to 4a93c94 Compare September 23, 2016 07:14

sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 23, 2016

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

sttts force-pushed the sttts-handler-chain-cleanup branch from 4a93c94 to 1350325 Compare September 23, 2016 08:41

sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 23, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2016

sttts added 3 commits September 23, 2016 12:00

Move TimeoutHandler+MaxInFlightLimit to Config.New()

8b33a9e

Simplify genericapiserver.Run()

3799ffa

Cleanup handler chain

87356c0

sttts force-pushed the sttts-handler-chain-cleanup branch from 1350325 to 87356c0 Compare September 23, 2016 10:08

sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 23, 2016

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

k8s-github-robot merged commit 0829f39 into kubernetes:master Sep 23, 2016

sttts deleted the sttts-handler-chain-cleanup branch September 23, 2016 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup genericapiserver handler chain #33164

Cleanup genericapiserver handler chain #33164

sttts commented Sep 21, 2016 •

edited by k8s-oncall

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k Sep 21, 2016

sttts Sep 21, 2016

deads2k commented Sep 21, 2016

deads2k commented Sep 21, 2016

soltysh Sep 21, 2016

sttts commented Sep 22, 2016

sttts commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

		secure = apiserver.WithImpersonation(secure, c.RequestContextMapper, c.Authorizer)
		secure = audit.WithAudit(secure, attributeGetter, s.auditWriter) // before impersonation to read original user

Cleanup genericapiserver handler chain #33164

Cleanup genericapiserver handler chain #33164

Conversation

sttts commented Sep 21, 2016 • edited by k8s-oncall

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 21, 2016

deads2k commented Sep 21, 2016

Choose a reason for hiding this comment

sttts commented Sep 22, 2016

sttts commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

sttts commented Sep 21, 2016 •

edited by k8s-oncall