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/08/05 03:12:33 UTC
[GitHub] [apisix-ingress-controller] AlinsRan opened a new pull request, #1222: fix: nodes convert failed
AlinsRan opened a new pull request, #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222
<!-- Please answer these questions before submitting a pull request -->
### Type of change:
<!-- Please delete options that are not relevant. -->
- [ ] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix-ingress-controller#community) first**
--
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
[GitHub] [apisix-ingress-controller] bzp2010 commented on a diff in pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#discussion_r946328504
##########
pkg/types/apisix/v1/types.go:
##########
@@ -216,21 +217,55 @@ type UpstreamNodes []UpstreamNode
// and by default empty array will be encoded as '{}'.
// We have to maintain the compatibility.
func (n *UpstreamNodes) UnmarshalJSON(p []byte) error {
+ var data []UpstreamNode
if p[0] == '{' {
- if len(p) != 2 {
- return errors.New("unexpected non-empty object")
+ value := map[string]float64{}
+ if err := json.Unmarshal(p, &value); err != nil {
+ return err
+ }
+ for k, v := range value {
+ node, err := mapKV2Node(k, v)
+ if err != nil {
+ return err
+ }
+ data = append(data, *node)
}
- *n = UpstreamNodes{}
+ *n = data
return nil
}
- var data []UpstreamNode
if err := json.Unmarshal(p, &data); err != nil {
return err
}
*n = data
return nil
}
+func mapKV2Node(key string, val float64) (*UpstreamNode, error) {
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
I think it is appropriate to configure the default port to `80`, unless the user manually specifies a schema for Upstream.
---------------------
BTW, the dashboard now uses the `object` to store nodes data (when created graphically using a form) it can be created without filling in the port, at this point there will be APISIX to determine the port automatically based on the scheme; when the user manually enters JSON to create it, a schema check will be performed, and if the port does not exist it will not be created successfully.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 merged pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
tao12345666333 merged PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222
--
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
[GitHub] [apisix-ingress-controller] bzp2010 commented on a diff in pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#discussion_r946328504
##########
pkg/types/apisix/v1/types.go:
##########
@@ -216,21 +217,55 @@ type UpstreamNodes []UpstreamNode
// and by default empty array will be encoded as '{}'.
// We have to maintain the compatibility.
func (n *UpstreamNodes) UnmarshalJSON(p []byte) error {
+ var data []UpstreamNode
if p[0] == '{' {
- if len(p) != 2 {
- return errors.New("unexpected non-empty object")
+ value := map[string]float64{}
+ if err := json.Unmarshal(p, &value); err != nil {
+ return err
+ }
+ for k, v := range value {
+ node, err := mapKV2Node(k, v)
+ if err != nil {
+ return err
+ }
+ data = append(data, *node)
}
- *n = UpstreamNodes{}
+ *n = data
return nil
}
- var data []UpstreamNode
if err := json.Unmarshal(p, &data); err != nil {
return err
}
*n = data
return nil
}
+func mapKV2Node(key string, val float64) (*UpstreamNode, error) {
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
I think it is appropriate to configure the default port to `80`, unless the user manually specifies a scheme for Upstream. We can perform a similar speculative process as APISIX.
_But the port in Ingress should be determined by the k8s service, is this here to be compatible with existing data change scenarios in etcd? For example, ingress data is modified by dashboard, etc._
---------------------
BTW, the dashboard now uses the `object` to store nodes data (when created graphically using a form) it can be created without filling in the port, at this point there will be APISIX to determine the port automatically based on the scheme; when the user manually enters JSON to create it, a schema check will be performed, and if the port does not exist it will not be created successfully.
--
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
[GitHub] [apisix-ingress-controller] AlinsRan commented on pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
AlinsRan commented on PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#issuecomment-1206042698
> 反序列化兼容处理后，从UpstreamNode序列化为json，apisix本身是否兼容，如果不兼容的话，apisix-ingress-controller是否要做同样的序列化处理？
I think it's compatible.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#issuecomment-1217698886
I will cherry-pick it into v1.5
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on a diff in pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on code in PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#discussion_r946308536
##########
pkg/types/apisix/v1/types.go:
##########
@@ -216,21 +217,55 @@ type UpstreamNodes []UpstreamNode
// and by default empty array will be encoded as '{}'.
// We have to maintain the compatibility.
func (n *UpstreamNodes) UnmarshalJSON(p []byte) error {
+ var data []UpstreamNode
if p[0] == '{' {
- if len(p) != 2 {
- return errors.New("unexpected non-empty object")
+ value := map[string]float64{}
+ if err := json.Unmarshal(p, &value); err != nil {
+ return err
+ }
+ for k, v := range value {
+ node, err := mapKV2Node(k, v)
+ if err != nil {
+ return err
+ }
+ data = append(data, *node)
}
- *n = UpstreamNodes{}
+ *n = data
return nil
}
- var data []UpstreamNode
if err := json.Unmarshal(p, &data); err != nil {
return err
}
*n = data
return nil
}
+func mapKV2Node(key string, val float64) (*UpstreamNode, error) {
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
ref: https://github.com/apache/apisix/blob/c4d5f2fca5b9ef98b551c769e2b1565185ed9630/apisix/schema_def.lua#L316-L341
port is not optional.
When APISIX is processing, there is a method to determine the port according to the protocol https://github.com/apache/apisix/blob/c4d5f2fca5b9ef98b551c769e2b1565185ed9630/apisix/upstream.lua#L167-L175
So here I suggest setting it to 80, WDYT @bzp2010
--
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
[GitHub] [apisix-ingress-controller] bzp2010 commented on a diff in pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#discussion_r946328504
##########
pkg/types/apisix/v1/types.go:
##########
@@ -216,21 +217,55 @@ type UpstreamNodes []UpstreamNode
// and by default empty array will be encoded as '{}'.
// We have to maintain the compatibility.
func (n *UpstreamNodes) UnmarshalJSON(p []byte) error {
+ var data []UpstreamNode
if p[0] == '{' {
- if len(p) != 2 {
- return errors.New("unexpected non-empty object")
+ value := map[string]float64{}
+ if err := json.Unmarshal(p, &value); err != nil {
+ return err
+ }
+ for k, v := range value {
+ node, err := mapKV2Node(k, v)
+ if err != nil {
+ return err
+ }
+ data = append(data, *node)
}
- *n = UpstreamNodes{}
+ *n = data
return nil
}
- var data []UpstreamNode
if err := json.Unmarshal(p, &data); err != nil {
return err
}
*n = data
return nil
}
+func mapKV2Node(key string, val float64) (*UpstreamNode, error) {
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
I think it is appropriate to configure the default port to `80`, unless the user manually specifies a schema for Upstream.
---------------------
Tips:
The dashboard now uses the `object` to store nodes data (when created graphically using a form) it can be created without filling in the port, at this point there will be APISIX to determine the port automatically based on the scheme; when the user manually enters JSON to create it, a schema check will be performed, and if the port does not exist it will not be created successfully.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on a diff in pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on code in PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#discussion_r946334199
##########
pkg/types/apisix/v1/types.go:
##########
@@ -216,21 +217,55 @@ type UpstreamNodes []UpstreamNode
// and by default empty array will be encoded as '{}'.
// We have to maintain the compatibility.
func (n *UpstreamNodes) UnmarshalJSON(p []byte) error {
+ var data []UpstreamNode
if p[0] == '{' {
- if len(p) != 2 {
- return errors.New("unexpected non-empty object")
+ value := map[string]float64{}
+ if err := json.Unmarshal(p, &value); err != nil {
+ return err
+ }
+ for k, v := range value {
+ node, err := mapKV2Node(k, v)
+ if err != nil {
+ return err
+ }
+ data = append(data, *node)
}
- *n = UpstreamNodes{}
+ *n = data
return nil
}
- var data []UpstreamNode
if err := json.Unmarshal(p, &data); err != nil {
return err
}
*n = data
return nil
}
+func mapKV2Node(key string, val float64) (*UpstreamNode, error) {
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
> is this here to be compatible with existing data change scenarios in etcd? For example, ingress data is modified by dashboard, etc.
yes. ref: #712 #1218
--
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
[GitHub] [apisix-ingress-controller] codecov-commenter commented on pull request #1222: fix: nodes convert failed
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1222:
URL: https://github.com/apache/apisix-ingress-controller/pull/1222#issuecomment-1216538180
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1222](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b0179d) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/cccad72a1e0ef60525c69371b4b27c4598c587c1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cccad72) will **increase** coverage by `13.22%`.
> The diff coverage is `33.16%`.
> :exclamation: Current head 5b0179d differs from pull request most recent head 34bb7c8. Consider uploading reports for the commit 34bb7c8 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1222 +/- ##
===========================================
+ Coverage 29.55% 42.78% +13.22%
===========================================
Files 81 73 -8
Lines 10166 6477 -3689
===========================================
- Hits 3005 2771 -234
+ Misses 6834 3409 -3425
+ Partials 327 297 -30
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...viders/apisix/translation/apisix\_cluster\_config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9hcGlzaXgvdHJhbnNsYXRpb24vYXBpc2l4X2NsdXN0ZXJfY29uZmlnLmdv) | `50.00% <ø> (ø)` | |
| [...kg/providers/apisix/translation/apisix\_consumer.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9hcGlzaXgvdHJhbnNsYXRpb24vYXBpc2l4X2NvbnN1bWVyLmdv) | `67.74% <ø> (ø)` | |
| [pkg/providers/apisix/translation/apisix\_ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9hcGlzaXgvdHJhbnNsYXRpb24vYXBpc2l4X3NzbC5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/providers/apisix/translation/translator.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9hcGlzaXgvdHJhbnNsYXRpb24vdHJhbnNsYXRvci5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/providers/gateway/translation/gateway.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9nYXRld2F5L3RyYW5zbGF0aW9uL2dhdGV3YXkuZ28=) | `0.00% <ø> (ø)` | |
| [.../providers/gateway/translation/gateway\_tlsroute.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9nYXRld2F5L3RyYW5zbGF0aW9uL2dhdGV3YXlfdGxzcm91dGUuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/providers/gateway/translation/translator.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9nYXRld2F5L3RyYW5zbGF0aW9uL3RyYW5zbGF0b3IuZ28=) | `0.00% <ø> (ø)` | |
| [pkg/providers/ingress/provider.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9pbmdyZXNzL3Byb3ZpZGVyLmdv) | `0.00% <0.00%> (ø)` | |
| [...s/ingress/translation/annotations/authorization.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9pbmdyZXNzL3RyYW5zbGF0aW9uL2Fubm90YXRpb25zL2F1dGhvcml6YXRpb24uZ28=) | `0.00% <ø> (ø)` | |
| [.../providers/ingress/translation/annotations/cors.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3Byb3ZpZGVycy9pbmdyZXNzL3RyYW5zbGF0aW9uL2Fubm90YXRpb25zL2NvcnMuZ28=) | `100.00% <ø> (ø)` | |
| ... and [40 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1222/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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