easegress-io / easegress

A Cloud Native traffic orchestration system
https://megaease.com/easegress/
Apache License 2.0
5.78k stars 501 forks source link

Easegress does not support server level (request URIs independent) filter mechanism #726

Closed jthann closed 10 months ago

jthann commented 2 years ago

Currently,In Easegress we have global filter to handle business logic that all pipelines need. But using GlobalFilter means the request URI must match the path rule we configure like following: if client access URI /a/xxx or /b/xxx ,the GlobalFilter logic will execute, but if clients access other URIs that is not matched by rules like /c/xxx. The GlobalFilter logic will not execute and just return http 404. Because in Easegress GlobalFilter execute after URI matching successfully.

kind: GlobalFilter
name: gf
beforePipeline:
  flow:
    - filter: f1
  filters:
    - name: f1
      kind: Filter1
---
kind: HTTPServer
name: https
port: 80
globalFilter: gf
rules:
  - paths:
      - pathPrefix: /a
        backend: pipelineA
      - pathPrefix: /b
        backend: pipelineB
---
#omit pipeline config for clarity

This is a problem in practical use,in many cases,we need to handle logic from request URIs which are not matched and don't expect just return http 404 status to these requests. For example in our SSOAuth global filter,we need to handle login logout sendToken..., these URIs will never be proxied to backend pipeline. And also there are other unknown URIs we need to return custom result.

So I think Easegress should support this mechanism,let's just call it ServerFilter interface(Just for discussion),this ServerFilter should execute before URIs rule matching and user can implement the interface to develop custom logic.

For clarify this feature rationality. In addition to practical usage,let's see other gateways in industry how to handle this. First in Java ecosystem,if using SpringMVC or SpringBoot or Spring Cloud Netflix Zuul(Based on servlet),we have javax.servlet.Filter which is Servlet specification interface,enhanced by Spring Framework providing org.springframework.web.filter.OncePerRequestFilter ,users can implement to handle. if using Spring Cloud Gateway (Spring WebFlux based on), we have org.springframework.web.server.WebFilter. In APISIX it provides global_rules: plugins: config. All these filters or plugins are request URIs independent.

suchen-sci commented 2 years ago

Hi @jthann thanks for your issue, here is a naive way to solve this problem. We are open to the discussion of server level filter, but it may a little bit confused with the idea of GlobalFilter...

Any idea here?

kind: GlobalFilter
name: gf
beforePipeline:
  flow:
    - filter: f1
  filters:
    - name: f1
      kind: Filter1
---
kind: HTTPServer
name: https
port: 80
globalFilter: gf
rules:
  - paths:
      - pathPrefix: /a
        backend: pipelineA
      - pathPrefix: /b
        backend: pipelineB
      - backend: pipelineNone
---
#omit pipeline config for clarity

kind: Pipeline
name: pipelineNone
flow: 
- filter: doNothing
  namespace: spacefornothing
filters:
- filter: doNothing
  kind: FilterDoNothing
suchen-sci commented 2 years ago

Hi @jthann we do some discuss about this issue, we plan to add a special built-in pipeline called NopPipeline. And this pipeline do nothing. So previous naive approach become following:

kind: GlobalFilter
name: gf
beforePipeline:
  flow:
    - filter: f1
  filters:
    - name: f1
      kind: Filter1
---
kind: HTTPServer
name: https
port: 80
globalFilter: gf
rules:
  - paths:
      - pathPrefix: /a
        backend: pipelineA
      - pathPrefix: /b
        backend: pipelineB
      - backend: NopPipeline
---
#omit pipeline config for clarity

Add server level filter maybe not necessary since we already have GlobalFilter. Tell us if this is ok for you. Thanks again for your issue.

jthann commented 2 years ago

@suchen-sci Yes, I do agree that a ServerFilter is confused with current GlobalFilter. I've created a concept ServerFilter just to illustrate the function,no really mean add a ServerFilter object implementation in Easegress. Using NopPipeline perhaps can solve this problem,but I feel this is strange and like hacked something,especially increasing the understanding burden for most common Easegress users.

I think we should use a common and universal way to solve this problem and my opinion is to refactor GlobalFilter logic to support this feature and users should not be aware of it. Let me describe my solution:

In Easegress we have there type Filters according to handling sequence:First is Before Pipeline filters in GlobalFilter,Second is Path Backend Pipeline filters, Third is After Pipeline filters in GlobalFilter. Right now,the there type Filters just put together like this pkg/object/httpserver/mux.go:524

    globalFilter := mi.getGlobalFilter()
    if globalFilter == nil {
        handler.Handle(ctx)
    } else {
        globalFilter.Handle(ctx, handler)
    }

The refactoring logic is to split there type Filters for there independent phase: GlobalFilter.HandleBefore Pipeline.Handle GlobalFilter.HandleAfter GlobalFilter.HandleBefore execute before URL route matching Pipeline.Handle and GlobalFilter.HandleAfter execute after URL route matching

In GlobalFilter within pkg/object/globalfilter/globalfilter.go adding following two methods:

func (gf *GlobalFilter) HandleBefore(ctx *context.Context) (result string, ended bool) {
    var before *pipeline.Pipeline
    if v := gf.beforePipeline.Load(); v != nil {
        if before, _ = v.(*pipeline.Pipeline); before != nil {
            return before.HandleWithEnd(ctx)
        }
    }
    return "", false
}

func (gf *GlobalFilter) HandleAfter(ctx *context.Context) (result string, ended bool) {
    if v := gf.afterPipeline.Load(); v != nil {
        if after, _ := v.(*pipeline.Pipeline); after != nil {
            return after.HandleWithEnd(ctx)
        }
    }
    return "", true
}

In Pipeline within pkg/object/pipeline/pipeline.go adding following methods and remove HandleWithBeforeAfter method which is not clear.

// HandleWithEnd is the handler return with filter end flag.
func (p *Pipeline) HandleWithEnd(ctx *context.Context) (string, bool) {
    stats := make([]FilterStat, 0, len(p.flow))
    result, stats, sawEnd := p.doHandle(ctx, p.flow, stats)
    ctx.LazyAddTag(func() string {
        return serializeStats(stats)
    })
    return result, sawEnd
}

Next in pkg/object/httpserver/mux.go:414 within func (mi *muxInstance) serveHTTP(stdw http.ResponseWriter, stdr *http.Request) method refactor as following

func (mi *muxInstance) serveHTTP(stdw http.ResponseWriter, stdr *http.Request) {

        //omit code line that is not changed 
    if mi.spec.XForwardedFor {
        appendXForwardedFor(req)
    }

    route := mi.search(req)
    maxBodySize := mi.spec.ClientMaxBodySize
    if route.code == 0 {
        maxBodySize = route.path.clientMaxBodySize
    }
    err := req.FetchPayload(maxBodySize)
    if err == httpprot.ErrRequestEntityTooLarge {
        logger.Debugf("%s: %s", mi.superSpec.Name(), err.Error())
        buildFailureResponse(ctx, http.StatusRequestEntityTooLarge)
        return
    }
    if err != nil {
        logger.Debugf("%s: failed to read request body: %v", mi.superSpec.Name(), err)
        buildFailureResponse(ctx, http.StatusBadRequest)
        return
    }
    var gf *globalfilter.GlobalFilter
    if gf = mi.getGlobalFilter(); gf != nil {
        if _, end := gf.HandleBefore(ctx); end {
            return
        }
    }
    if route.code != 0 {
        logger.Debugf("%s: status code of result route: %d", mi.superSpec.Name(), route.code)
        buildFailureResponse(ctx, route.code)
        return
    }

    handler, ok := mi.muxMapper.GetHandler(route.path.backend)
    if !ok {
        logger.Debugf("%s: backend %q not found", mi.superSpec.Name(), route.path.backend)
        buildFailureResponse(ctx, http.StatusServiceUnavailable)
        return
    }

    route.path.rewrite(req)
    if p, ok := handler.(*pipeline.Pipeline); ok {
        if _, end := p.HandleWithEnd(ctx); end {
            return
        }
    }
    if gf != nil {
        gf.HandleAfter(ctx)
    }
}

After above refactoring, we can support this feature and keep compatibility at the same time. Above all for Easegress users,it does not add their understanding burden.

I've implemented the refactoring function in my maintained branch based on Easegress v2.0.0 and passed test cases in our business scene,if it is acceptable,I'll make a PR.

suchen-sci commented 2 years ago

Hi @jthann thanks a lot for your reply.

Here is my understanding about this issue. For HTTPServer, if a request match a rule, then exec corresponding pipeline. If not match, return 404. For GlobalFilter, it exec before and after the pipeline.

And by using your solution, For HTTPServer, if a request not match, it exec GlobalFilter before pipeline and then return. For GlobalFilter, beforePipeline is no longer before pipeline, it becomes before url match. And afterPipeline is still after pipeline.

Maybe one way to solve this problem is to add new part in GlobalFilter called beforeMatch? I am not sure about this. Any idea here? @localvar

localvar commented 2 years ago

I prefer using a NOP pipeline, there are two cases here:

And if we check the access log of an HTTP server, we can see there are many garbage requests, which should be discarded directly, this is why I think we should support the 2nd case, and a NOP pipeline can support both cases.

xxx7xxxx commented 10 months ago

We supported it in https://github.com/easegress-io/easegress/blob/main/docs/02.Tutorials/2.3.Pipeline-Explained.md#globalfilter