change: remove listen.host from api/conf/conf.yaml #1767

nic-chen · 2021-04-14T01:42:57Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

The configuration item listen.host in conf.yaml often confuses users, because when the configuration dashboard can be accessed non-locally, two places need to be modified (the other place is allow_list).
remove the listen.host configuration item from conf.yaml, but retain the feature, and users can add it when they need it.

What changes will this PR take into?

remove listen.host from api/conf/conf.yaml
set ServerHost in the code to an empty string

Related issues

fix/resolve #1743

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

netlify · 2021-04-14T01:43:03Z

Deploy preview for apisix-dashboard ready!

Built with commit 368163e

https://deploy-preview-1767--apisix-dashboard.netlify.app

juzhiyuan · 2021-04-14T02:09:12Z

bugfix or feature?

juzhiyuan · 2021-04-15T05:46:32Z

may sync master codes to fix CI

codecov-io · 2021-04-15T09:20:43Z

Codecov Report

Merging #1767 (4fbea88) into master (f95324f) will decrease coverage by 10.38%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1767       +/-   ##
===========================================
- Coverage   72.64%   62.25%   -10.39%     
===========================================
  Files         116       47       -69     
  Lines        2738     3129      +391     
  Branches      661        0      -661     
===========================================
- Hits         1989     1948       -41     
- Misses        749      867      +118     
- Partials        0      314      +314

Flag	Coverage Δ
backend-e2e-test	`62.25% <ø> (?)`
backend-e2e-test-ginkgo	`49.21% <ø> (?)`
frontend-e2e-test	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/internal/conf/conf.go	`66.19% <ø> (ø)`
web/src/pages/Route/components/Step2/index.tsx
web/src/pages/Consumer/service.ts
web/src/components/Plugin/UI/limit-conn.tsx
...ages/Route/components/Step2/RequestRewriteView.tsx
...eb/src/components/PluginOrchestration/constants.ts
...omponents/Upstream/components/UpstreamSelector.tsx
web/src/libs/iconfont.js
web/src/app.tsx
web/src/pages/Dashboard/service.ts
... and 116 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e2e0b7...4fbea88. Read the comment docs.

juzhiyuan · 2021-04-15T09:50:41Z

Please sync the master branch

starsz

Why not set “0.0.0.0” as default？

tokers · 2021-04-17T12:00:36Z

.github/workflows/deploy-with-docker.yml

@@ -33,8 +33,9 @@ jobs:
      - name: Modify Config
        run: |
          sed -i 's/127.0.0.1:2379/172.16.238.10:2379/' api/conf/conf.yaml
-          sed -i 's/host: 127.0.0.1/host: 0.0.0.0/' api/conf/conf.yaml


Should add some notes about the default listen IP address(es).

nic-chen · 2021-04-18T04:24:00Z

Why not set “0.0.0.0” as default？

Because in our configuration, listen will only have port.

So it would be strange to display 0.0.0.0:9000 after the manager-api started, :9000 should be better.

What do you think? @starsz

codecov-commenter · 2021-04-18T09:28:03Z

Codecov Report

Merging #1767 (368163e) into master (5d9cd3f) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
+ Coverage   71.81%   71.85%   +0.03%     
==========================================
  Files         172      172              
  Lines        6064     6064              
  Branches      703      703              
==========================================
+ Hits         4355     4357       +2     
+ Misses       1466     1463       -3     
- Partials      243      244       +1

Flag	Coverage Δ
backend-e2e-test	`62.39% <ø> (+0.22%)`	⬆️
backend-e2e-test-ginkgo	`48.83% <ø> (-0.23%)`	⬇️
backend-unit-test	`52.41% <ø> (ø)`
frontend-e2e-test	`72.06% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/internal/conf/conf.go	`66.19% <ø> (-2.82%)`	⬇️
api/internal/core/store/store.go	`86.82% <0.00%> (-1.20%)`	⬇️
api/internal/core/storage/etcd.go	`49.09% <0.00%> (+1.81%)`	⬆️
api/internal/core/store/storehub.go	`74.76% <0.00%> (+3.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d9cd3f...368163e. Read the comment docs.

starsz · 2021-04-18T12:17:23Z

api/conf/conf.yaml

@@ -17,10 +17,15 @@

 conf:
  listen:
-    host: 127.0.0.1     # `manager api` listening ip or host name


We still support configuring the host. So can we comment on this instead of removing them?

@starsz your way is better

membphis · 2021-04-18T12:40:59Z

api/conf/conf.yaml

    port: 9000          # `manager api` listening port
  allow_list:           # If we don't set any IP list, then any IP access is allowed by default.
    - 127.0.0.0/24
+    - 10.0.0.0/8


only one 127.0.0.0/24 is enough.

we can add more comment to show how to set it

starsz · 2021-04-19T03:16:55Z

Emmm.Need to fix the link error. @nic-chen

nic-chen · 2021-04-19T03:20:25Z

Emmm.Need to fix the link error. @nic-chen

sure

change: remove listen.host from conf.yaml

06c8064

johzchen added 3 commits April 15, 2021 13:59

Merge branch 'master' into remove-listen-host

d530504

fix: no need to replace the host value

bb11abd

test: debug

ef6e5f1

johzchen added 5 commits April 15, 2021 17:57

fix: use current branch to build docker image to test

8e56c30

fix: use current branch to build docker image to test

4fbea88

fix: use current branch to build docker image to test

7f7a0fa

fix CI failed

223ff91

Merge branch 'master' into remove-listen-host

6775b6f

nic-chen requested review from starsz, membphis, gxthrj, imjoey, Jaycean and tokers April 17, 2021 09:14

starsz reviewed Apr 17, 2021

View reviewed changes

tokers reviewed Apr 17, 2021

View reviewed changes

chore: add common intranet IP segment

60a7b4e

chore: add comment about the listen IP

962cb77

tokers approved these changes Apr 18, 2021

View reviewed changes

starsz reviewed Apr 18, 2021

View reviewed changes

membphis reviewed Apr 18, 2021

View reviewed changes

fix: update note description

c265f14

johzchen added 3 commits April 18, 2021 22:55

fix CI

2ceb4e7

fix CI

fd6cd99

fix CI

a0f137a

gxthrj approved these changes Apr 19, 2021

View reviewed changes

starsz approved these changes Apr 19, 2021

View reviewed changes

johzchen added 2 commits April 19, 2021 11:35

Merge branch 'master' into remove-listen-host

4a2b248

fix: yaml lint

368163e

nic-chen merged commit 5406ba3 into apache:master Apr 19, 2021

nic-chen deleted the remove-listen-host branch April 20, 2021 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change: remove listen.host from api/conf/conf.yaml #1767

change: remove listen.host from api/conf/conf.yaml #1767

nic-chen commented Apr 14, 2021

netlify bot commented Apr 14, 2021 •

edited

Loading

juzhiyuan commented Apr 14, 2021

juzhiyuan commented Apr 15, 2021

codecov-io commented Apr 15, 2021 •

edited

Loading

juzhiyuan commented Apr 15, 2021

starsz left a comment

tokers Apr 17, 2021

nic-chen Apr 18, 2021

nic-chen commented Apr 18, 2021

codecov-commenter commented Apr 18, 2021 •

edited

Loading

starsz Apr 18, 2021

membphis Apr 18, 2021

nic-chen Apr 19, 2021

membphis Apr 18, 2021

nic-chen Apr 19, 2021

starsz commented Apr 19, 2021

nic-chen commented Apr 19, 2021

change: remove listen.host from api/conf/conf.yaml #1767

change: remove listen.host from api/conf/conf.yaml #1767

Conversation

nic-chen commented Apr 14, 2021

netlify bot commented Apr 14, 2021 • edited Loading

juzhiyuan commented Apr 14, 2021

juzhiyuan commented Apr 15, 2021

codecov-io commented Apr 15, 2021 • edited Loading

Codecov Report

juzhiyuan commented Apr 15, 2021

starsz left a comment

Choose a reason for hiding this comment

tokers Apr 17, 2021

Choose a reason for hiding this comment

nic-chen Apr 18, 2021

Choose a reason for hiding this comment

nic-chen commented Apr 18, 2021

codecov-commenter commented Apr 18, 2021 • edited Loading

Codecov Report

starsz Apr 18, 2021

Choose a reason for hiding this comment

membphis Apr 18, 2021

Choose a reason for hiding this comment

nic-chen Apr 19, 2021

Choose a reason for hiding this comment

membphis Apr 18, 2021

Choose a reason for hiding this comment

nic-chen Apr 19, 2021

Choose a reason for hiding this comment

starsz commented Apr 19, 2021

nic-chen commented Apr 19, 2021

netlify bot commented Apr 14, 2021 •

edited

Loading

codecov-io commented Apr 15, 2021 •

edited

Loading

codecov-commenter commented Apr 18, 2021 •

edited

Loading