Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract internal router creation from server #3204

Merged
merged 10 commits into from Apr 23, 2018

Conversation

juliens
Copy link
Member

@juliens juliens commented Apr 18, 2018

What does this PR do?

Build internal router (api, ping, etc...) out of server.go.

Motivation

Separation of concern.

More

  • Added/updated tests

@juliens juliens added kind/enhancement a new or improved feature. status/2-needs-review labels Apr 18, 2018
@juliens juliens force-pushed the refacto-internal-router branch 2 times, most recently from 16e6bd2 to 5840b0a Compare April 18, 2018 15:16
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

👏 👏

Very interesting PR.

Just two suggestions :)

var serverMiddlewares []negroni.Handler

if globalConfiguration.EntryPoints[entryPointName].WhiteList != nil {
ipWhitelistMiddleware, _ := middlewares.NewIPWhiteLister(
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about manaaging the error returned by the function?

}

if globalConfiguration.EntryPoints[entryPointName].Auth != nil {
authMiddleware, _ := mauth.NewAuthenticator(globalConfiguration.EntryPoints[entryPointName].Auth, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about manaaging the error retruned by the function?

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Nice work @juliens. Only few comments 👏

}

// AddRoutes Add routes to the router
func (r *WithPrefix) AddRoutes(systemRouter *mux.Router) {
Copy link
Member

Choose a reason for hiding this comment

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

Why r ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this a router!

}

// AddRouter add a router in the aggregator
func (r *InternalRouterAggregator) AddRouter(router types.InternalRouter) {
Copy link
Member

Choose a reason for hiding this comment

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

Why r ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this a router!

}

// AddRoutes Add routes to the router
func (r *InternalRouterAggregator) AddRoutes(systemRouter *mux.Router) {
Copy link
Member

Choose a reason for hiding this comment

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

Why r ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this a router!

ping/ping.go Outdated

g.terminating = true
// WithContext causes the ping endpoint to serve non 200 responses.
func (g *Handler) WithContext(ctx context.Context) {
Copy link
Member

Choose a reason for hiding this comment

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

Why g ?

ping/ping.go Outdated
go func() {
<-ctx.Done()
g.terminating = true
}()
}

// AddRoutes add ping routes on a router
func (g *Handler) AddRoutes(router *mux.Router) {
Copy link
Member

Choose a reason for hiding this comment

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

Why g ?

server/server.go Outdated
@@ -665,8 +671,8 @@ func (s *Server) createTLSConfig(entryPointName string, tlsOption *traefiktls.TL
if entryPointName == s.globalConfiguration.ACME.EntryPoint {
checkOnDemandDomain := func(domain string) bool {
routeMatch := &mux.RouteMatch{}
router := router.GetHandler()
match := router.Match(&http.Request{URL: &url.URL{}, Host: domain}, routeMatch)
rt := router.GetHandler()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could simplify these two lines in one line of code

server/server.go Outdated
if s.globalConfiguration.EntryPoints[entryPointName].TLS.CipherSuites != nil {
// if our list of CipherSuites is defined in the entrypoint config, we can re-initilize the suites list as empty

//Set the list of CipherSuites if set in the config TOML
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a space after //

server/server.go Outdated

//Set the list of CipherSuites if set in the config TOML
if s.entryPoints[entryPointName].Configuration.TLS.CipherSuites != nil {
//if our list of CipherSuites is defined in the entrypoint config, we can re-initilize the suites list as empty
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a space after //

server/server.go Outdated
for entryPointName := range globalConfiguration.EntryPoints {
router := s.buildDefaultHTTPRouter()
for entryPointName := range s.entryPoints {
rt := s.buildDefaultHTTPRouter()
Copy link
Member

Choose a reason for hiding this comment

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

I think that you could directly use s.buildDefaultHTTPRouter() in the middlewares.NewHandlerSwitcher function instead of creating a variable.

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 👏 🐔

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM 👏 🐔 🐿️

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM 🐟

@traefiker traefiker merged commit 9daae9c into traefik:master Apr 23, 2018
@juliens juliens deleted the refacto-internal-router branch July 9, 2018 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants