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

fix: nodes convert failed #1222

Merged
merged 2 commits into from
Aug 17, 2022
Merged

fix: nodes convert failed #1222

merged 2 commits into from
Aug 17, 2022

Conversation

AlinsRan
Copy link
Contributor

@AlinsRan AlinsRan commented Aug 5, 2022

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

Question: At present, the implementation only supports the conversion of array types.

Resolved: Add conversion of object type.

Pre-submission checklist:

  • 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 first

Copy link

@marchlhw marchlhw left a comment

Choose a reason for hiding this comment

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

反序列化兼容处理后,从UpstreamNode序列化为json,apisix本身是否兼容,如果不兼容的话,apisix-ingress-controller是否要做同样的序列化处理?

@AlinsRan
Copy link
Contributor Author

AlinsRan commented Aug 5, 2022

反序列化兼容处理后,从UpstreamNode序列化为json,apisix本身是否兼容,如果不兼容的话,apisix-ingress-controller是否要做同样的序列化处理?

Nodes has different forms of expression, and only one is supported in the implementation. For different forms of expression, their functions are actually the same.
Of course, we can consider implementing a data structure that is fully compatible with APISIX.

@tao12345666333 tao12345666333 linked an issue Aug 5, 2022 that may be closed by this pull request
Comment on lines 246 to 247
// according to APISIX upstream nodes policy, port is optional
port := "0"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

@bzp2010 bzp2010 Aug 16, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@codecov-commenter
Copy link

Codecov Report

Merging #1222 (5b0179d) into master (cccad72) will increase coverage by 13.22%.
The diff coverage is 33.16%.

❗ Current head 5b0179d differs from pull request most recent head 34bb7c8. Consider uploading reports for the commit 34bb7c8 to get more accurate results

@@             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 Coverage Δ
...viders/apisix/translation/apisix_cluster_config.go 50.00% <ø> (ø)
...kg/providers/apisix/translation/apisix_consumer.go 67.74% <ø> (ø)
pkg/providers/apisix/translation/apisix_ssl.go 0.00% <0.00%> (ø)
pkg/providers/apisix/translation/translator.go 0.00% <0.00%> (ø)
pkg/providers/gateway/translation/gateway.go 0.00% <ø> (ø)
.../providers/gateway/translation/gateway_tlsroute.go 0.00% <0.00%> (ø)
pkg/providers/gateway/translation/translator.go 0.00% <ø> (ø)
pkg/providers/ingress/provider.go 0.00% <0.00%> (ø)
...s/ingress/translation/annotations/authorization.go 0.00% <ø> (ø)
.../providers/ingress/translation/annotations/cors.go 100.00% <ø> (ø)
... and 40 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tao12345666333 tao12345666333 added this to the v1.6.0 milestone Aug 17, 2022
@tao12345666333 tao12345666333 merged commit 35c9f6b into apache:master Aug 17, 2022
@tao12345666333
Copy link
Member

I will cherry-pick it into v1.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request help: upstreamNode json解析异常
6 participants