You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/10/14 09:40:28 UTC
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2634: feat: Proxy the related requests to apisix
bzp2010 commented on code in PR #2634:
URL: https://github.com/apache/apisix-dashboard/pull/2634#discussion_r995491943
##########
api/internal/handler/authentication/authentication.go:
##########
@@ -51,9 +51,9 @@ type UserSession struct {
// swagger:model LoginInput
type LoginInput struct {
// username
- Username string `form:"username" binding:"required"`
+ Username string `json:"username" binding:"required"`
Review Comment:
Why did you change the `form` to `json`?
##########
api/config/config.yaml:
##########
@@ -32,7 +32,8 @@ data_source: # The data source type of API
security:
allow_list: # If we don't set any IP list, then localhost IP access is allowed by default.
- - 127.0.0.1 # The rules are checked in sequence until the first match is found.
+ - 172.16.238.1 # The rules are checked in sequence until the first match is found.
Review Comment:
The default configuration should not be modified.
##########
api/internal/handler/handler.go:
##########
@@ -60,10 +60,8 @@ func Wrap(f Func, inputType reflect.Type) gin.HandlerFunc {
}
resp := f(c, input)
- if resp.StatusCode == 0 {
- resp.StatusCode = 200
- }
- c.JSON(resp.StatusCode, buildResponse(resp.Success, resp.Message, resp.Data))
+
+ c.JSON(resp.StatusCode, buildResponse(resp.Success, resp.ErrMsg, resp.Data))
Review Comment:
Default response status codes should be provided.
##########
api/internal/handler/authentication/authentication.go:
##########
@@ -101,14 +101,14 @@ func (h *Handler) userLogin(_ *gin.Context, input interface{}) handler.Response
if user == nil {
return handler.Response{
StatusCode: http.StatusUnauthorized,
- Message: consts.ErrUsernamePassword.Error(),
+ ErrMsg: consts.ErrUsernamePassword.Error(),
}
}
if username != user.Username || password != user.Password {
return handler.Response{
StatusCode: http.StatusUnauthorized,
- Message: consts.ErrUsernamePassword.Error(),
+ ErrMsg: consts.ErrUsernamePassword.Error(),
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -91,12 +91,19 @@ func (h *Handler) setupClient(cfg config.Config) {
func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ StatusCode: resp.StatusCode(),
Review Comment:
There is no need to pass through the Admin API status code.
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
+ ErrMsg: err.Error(),
+ }
}
return handler.Response{
- Success: true,
- Data: data.Value(),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: data.Value(),
}
}
// proxy forwards user requests for these interfaces to the Admin API as-is.
-func (h *Handler) proxy(c *gin.Context) (*resty.Response, *handler.Response) {
+func (h *Handler) proxy(c *gin.Context) (*resty.Response, error) {
Review Comment:
No need to modify this for now
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
+ ErrMsg: err.Error(),
+ }
}
return handler.Response{
- Success: true,
- Data: data.Value(),
+ Success: true,
+ StatusCode: resp.StatusCode(),
Review Comment:
ditto
##########
api/test/docker/setup.sh:
##########
@@ -90,6 +90,7 @@ up() {
#modifying config
sed -i 's/127.0.0.1:2379/172.16.238.10:2379/' "$YAML_FILE"
+ sed -i 's/127.0.0.1:9180/172.16.238.30:9180/' "$YAML_FILE"
Review Comment:
As far as I know, we don't use this file anymore.
##########
api/test/e2e/base/base.go:
##########
@@ -304,18 +301,24 @@ func GetResourceList(resource string) string {
func CleanResource(resource string) {
resources := GetResourceList(resource)
- list := gjson.Get(resources, "data.rows").Value().([]interface{})
+ //body, _ := json.Marshal(resources)
+ //res := fmt.Sprintf("%s%s", "resources: ", resources)
+ //panic(res)
Review Comment:
Are they still useful?
##########
api/internal/handler/authentication/authentication.go:
##########
@@ -101,14 +101,14 @@ func (h *Handler) userLogin(_ *gin.Context, input interface{}) handler.Response
if user == nil {
return handler.Response{
StatusCode: http.StatusUnauthorized,
- Message: consts.ErrUsernamePassword.Error(),
+ ErrMsg: consts.ErrUsernamePassword.Error(),
Review Comment:
The response body style should not be modified, it is predetermined.
##########
api/internal/handler/handler.go:
##########
@@ -38,8 +38,8 @@ type Func = func(c *gin.Context, input interface{}) Response
type Response struct {
StatusCode int
Success bool
- Message string
Data interface{}
+ ErrMsg string
Review Comment:
The response body style should not be modified, it is predetermined.
##########
api/internal/handler/resources/resources.go:
##########
@@ -91,12 +91,19 @@ func (h *Handler) setupClient(cfg config.Config) {
func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
Review Comment:
We'd better not change it for now, maybe it's better to address the code style issue in a later PR.
##########
api/internal/handler/resources/resources.go:
##########
@@ -77,7 +77,7 @@ func (h *Handler) ApplyRoute(r *gin.Engine, cfg config.Config) {
}
// add misc routes
- r.GET("/apisix/admin/plugins/list", handler.Wrap(h.ProxyMisc, nil))
+ r.GET("/apisix/admin/plugins/*param", handler.Wrap(h.ProxyMisc, nil))
Review Comment:
`list` is enough. It is used to get the full table of plugins.
We have this usage, and you can verify it this way
- /apisix/admin/plugins/list
- /apisix/admin/schema/route
- /apisix/admin/schema/plugins/real-ip
##########
api/internal/handler/resources/resources.go:
##########
@@ -91,12 +91,19 @@ func (h *Handler) setupClient(cfg config.Config) {
func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ StatusCode: resp.StatusCode(),
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
Review Comment:
ditto
##########
api/internal/handler/authentication/authentication.go:
##########
@@ -51,9 +51,9 @@ type UserSession struct {
// swagger:model LoginInput
type LoginInput struct {
// username
- Username string `form:"username" binding:"required"`
+ Username string `json:"username" binding:"required"`
// password
- Password string `form:"password" binding:"required"`
+ Password string `json:"password" binding:"required"`
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
Review Comment:
There is no need to pass through the Admin API status code.
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
+ ErrMsg: err.Error(),
+ }
Review Comment:
ditto
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
+ ErrMsg: err.Error(),
+ }
}
return handler.Response{
- Success: true,
- Data: data.Value(),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: data.Value(),
}
Review Comment:
ditto
##########
api/test/docker/apisix_config.yaml:
##########
@@ -57,61 +54,14 @@ apisix:
- 10095
nginx_config:
- error_log_level: "debug"
-
-plugins: # plugin list (sorted in alphabetical order)
- - api-breaker
- - authz-keycloak
- - basic-auth
- - batch-requests
- - consumer-restriction
- - cors
- - echo
- - fault-injection
- - grpc-transcode
- - hmac-auth
- - http-logger
- - ip-restriction
- - jwt-auth
- - kafka-logger
- - key-auth
- - limit-conn
- - limit-count
- - limit-req
- - node-status
- - openid-connect
- - prometheus
- - proxy-cache
- - proxy-mirror
- - proxy-rewrite
- - redirect
- - referer-restriction
- - request-id
- - request-validation
- - response-rewrite
- - serverless-post-function
- - serverless-pre-function
- - skywalking
- - sls-logger
- - syslog
- - tcp-logger
- - udp-logger
- - uri-blocker
- - wolf-rbac
- - zipkin
- - server-info
+ error_log_level: debug
plugin_attr:
skywalking:
- service_name: APISIX
- service_instance_name: "APISIX Instance Name"
endpoint_addr: http://172.16.238.50:12800
server-info:
- report_interval: 60
report_ttl: 3600
prometheus:
- export_uri: /apisix/prometheus/metrics
enable_export_server: true
export_addr:
- ip: "0.0.0.0"
- port: 9091
+ ip: 0.0.0.0
Review Comment:
Instead of modifying `apisix_config.yaml` for now, we are now using `apisix_config_v3.yaml`.
##########
api/internal/handler/resources/resources.go:
##########
@@ -106,60 +113,57 @@ func (h *Handler) ProxyResource(c *gin.Context, _ interface{}) handler.Response
}
return handler.Response{
- Success: true,
- Data: h.processResponse(version, data),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: h.processResponse(version, data),
}
}
// ProxyMisc sends the Admin API response as-is, adding only the wrapping required by the Dashboard
func (h *Handler) ProxyMisc(c *gin.Context, _ interface{}) handler.Response {
resp, err := h.proxy(c)
if err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ ErrMsg: err.Error(),
+ }
}
data := gjson.ParseBytes(resp.Body())
if err := h.extractError(data); err != nil {
- return *err
+ return handler.Response{
+ Success: false,
+ StatusCode: resp.StatusCode(),
+ ErrMsg: err.Error(),
+ }
}
return handler.Response{
- Success: true,
- Data: data.Value(),
+ Success: true,
+ StatusCode: resp.StatusCode(),
+ Data: data.Value(),
}
}
// proxy forwards user requests for these interfaces to the Admin API as-is.
-func (h *Handler) proxy(c *gin.Context) (*resty.Response, *handler.Response) {
+func (h *Handler) proxy(c *gin.Context) (*resty.Response, error) {
req := h.client.NewRequest()
req.SetHeaderMultiValues(c.Request.Header)
req.SetQueryParamsFromValues(c.Request.URL.Query())
req.SetBody(c.Request.Body)
resourceURI := strings.Replace(c.Request.URL.Path, "/apisix/admin", "", 1)
resp, err := req.Execute(c.Request.Method, resourceURI)
- if err != nil {
- return nil, &handler.Response{
- StatusCode: http.StatusInternalServerError,
- Message: "Admin API request failed",
- }
- }
-
- return resp, nil
+ return resp, err
}
-func (h *Handler) extractError(data gjson.Result) *handler.Response {
+func (h *Handler) extractError(data gjson.Result) error {
Review Comment:
No need to modify this for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org