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

Conflicts with existing wildcard #136

Closed
thinkingmik opened this issue Aug 5, 2019 · 14 comments
Closed

Conflicts with existing wildcard #136

thinkingmik opened this issue Aug 5, 2019 · 14 comments
Labels

Comments

@thinkingmik
Copy link

Version: devopsfaith/krakend:0.10.0-dev

Problem

I'm trying to define the gateway endpoints, this is my krakend.json:

{
  "version": 2,
  "name": "MyLovely API Gateway v1.0.0",
	"port": 8080,
	"timeout": "30s",
	"cache_ttl": "300s",
  "endpoints": [ 
    {
      "method": "POST",
      "endpoint": "/api/exhibitions/{exhibitionCode}",
      "backend": [
        {
          "host": [ "http://localhost:8200" ],
          "url_pattern": "/api/exhibitions/{exhibitionCode}"
        }
      ]
    },
    {
      "method": "POST",
      "endpoint": "/api/exhibitions/{exhibitionCode}/info",
      "backend": [
        {
          "host": [ "http://localhost:8200" ],
          "url_pattern": "/api/exhibitions/{exhibitionCode}/info"
        }
      ]
    },
    {
      "method": "POST",
      "endpoint": "/api/exhibitions/search",
      "backend": [
        {
          "host": [ "http://localhost:8200" ],
          "url_pattern": "/api/exhibitions/search"
        }
      ]
    }
  ]
}

Error

I'm getting this error when I'm running the docker container:

panic: 'search' in new path '/api/exhibitions/search' conflicts with existing wildcard ':exhibitionCode' in existing prefix '/api/exhibitions/:exhibitionCode'

Question

Why I'm getting this error? Is there a way to define a static endpoint like /api/exhibitions/search without have conflicts with wildcard's endpoints?

@kpacha
Copy link
Member

kpacha commented Aug 5, 2019

hi @thinkingmik ,

the panic you're getting is thrown by the gin router and it's a known limitation of the router package the gin project uses

You can check it with this simple script:

package main

import "github.com/gin-gonic/gin"

func main() {
	r := gin.New()
	r.GET("/ping", handler)
	r.GET("/ping/foo", handler)
	r.GET("/ping/:a", handler)
	r.GET("/ping/:a/bar", handler)
}

func handler(c *gin.Context) {
	c.JSON(200, gin.H{
		"message": "pong",
	})
}
panic: wildcard route ':a' conflicts with existing children in path '/ping/:a'

@kpacha kpacha closed this as completed Aug 5, 2019
@thinkingmik
Copy link
Author

thinkingmik commented Aug 6, 2019

Ok thank you, but if I want to use another router like gorilla/mux instead of the default gin-gonic/gin router?

I would modify the router_engine.go and rebuild krakend-ce for my own, using gorilla/mux. Is the correct way?

package krakend

import (
	cors "github.com/devopsfaith/krakend-cors/mux"
	httpsecure "github.com/devopsfaith/krakend-httpsecure/mux"
	"github.com/devopsfaith/krakend/config"
	"github.com/devopsfaith/krakend/logging"
	"github.com/gorilla/mux"
)

// NewEngine creates a new mux engine with some default values and a secure middleware
func NewEngine(cfg config.ServiceConfig, logger logging.Logger) *mux.Engine {
	engine := mux.DefaultEngine()

	//engine.RedirectTrailingSlash = true
	//engine.RedirectFixedPath = true
	//engine.HandleMethodNotAllowed = true

	if corsMw := cors.New(cfg.ExtraConfig); corsMw != nil {
		engine.Use(corsMw)
	}

	if err := httpsecure.Register(cfg.ExtraConfig, engine); err != nil {
		logger.Warning(err)
	}

	return engine
}

@kpacha
Copy link
Member

kpacha commented Aug 7, 2019

you should also modify the handler_factory.go and executor.go because they are injecting router dependent modules and components

@thinkingmik
Copy link
Author

Yes of course, thank you.

@tfluehmann
Copy link

hi @thinkingmik

I am very interested in the same solution as you asked for in this thread. Do you have published your solution for that topic?

Thanks in advance,
tfluehmann

@thinkingmik
Copy link
Author

Hi @tfluehmann

sorry but I haven't implemented this solution... I'm still using the standard krakend-ce with default gin-gonic/gin router.

@novalagung
Copy link

hi @thinkingmik

I'm getting the same error as yours. May I know how did you resolve the issue while keep using the standard krakend-ce? or did you just change the endpoint schema to avoid the conflict?

@thinkingmik
Copy link
Author

Hi @novalagung,

with krakend-ce, that use by default gin-gonic router framework, there is no way to resolve this issue. You must pay attention when you write the endpoints to avoid conflicts. Sorry.

@cafebabepl
Copy link

I have been fascinated by krakend for several days but today I got to the wall. I have no idea how to organize endpoint naming for the existing API with this limitation.

@thinkingmik
Copy link
Author

thinkingmik commented Apr 24, 2020

I know @wkozi :( but after days of work writing endpoints with this limitation you will get used to it.

However, if you have existing APIs you will change only the exposed endpoint by krakend, not the backend endpoint of your existing API.

For example:

"method": "GET",
"endpoint": "/exposed/endpoint/ping",
"backend": [
  {
    "host": ["http://localhost:3000"],
    "url_pattern": "/different/ping",
    "sd": "static"
  }
]

@novalagung
Copy link

@wkozi I ended up creating my own web service, utilizing the krakend middleware with gorilla mux router.

@cafebabepl
Copy link

Of course, I understand the limitations and know the workaround, but now I migrate from Netflix Zuul and decompose our API. I understand api gateway as a product = a set of business methods from different microservices. In my opinion, the fact that I have to change my current endpoints, e.g.

/api/contracts/{contractId}/documents/{documentId}
/api/contracts/{contractId}/documents/{documentId}/attachments
/api/contracts/{contractId}/documents/current

to

/api/contracts/{contractId}/documents/{documentId}
/api/contracts/{contractId}/documents/{documentId}/attachments
/api/contracts/{contractId}/documents#current

is a big limitation ;-(

@cafebabepl
Copy link

@novalagung it's very interesting. I understand that this requires modification of krakend sources. Can I see it somewhere?

@github-actions
Copy link

github-actions bot commented Apr 8, 2022

This issue was marked as resolved a long time ago and now has been automatically locked as there has not been any recent activity after it. You can still open a new issue and reference this link.

@github-actions github-actions bot added the locked label Apr 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants