Skip to content

fix: move conf/cert to t/certs and disable ssl by default#2112

Merged
moonming merged 32 commits into
apache:masterfrom
Yiyiyimu:certs
Nov 20, 2020
Merged

fix: move conf/cert to t/certs and disable ssl by default#2112
moonming merged 32 commits into
apache:masterfrom
Yiyiyimu:certs

Conversation

@Yiyiyimu

Copy link
Copy Markdown
Member

What this PR does / why we need it:

fix #2110

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?

@moonming moonming added this to the 2.1 milestone Sep 16, 2020
@membphis

Copy link
Copy Markdown
Member

@Yiyiyimu please fix the issue of CI

@Yiyiyimu

Copy link
Copy Markdown
Member Author

Hi @membphis the CI error is due to there're some problems unsolved. Discussion is on the original issue #2110.

Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
@membphis membphis changed the title fix: move conf/cert to t/certs chore: move conf/cert to t/certs Nov 18, 2020
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
Signed-off-by: yiyiyimu <[email protected]>
@Yiyiyimu
Yiyiyimu requested a review from moonming November 19, 2020 03:00
@Yiyiyimu
Yiyiyimu requested a review from nic-chen November 19, 2020 03:00
@Yiyiyimu

Yiyiyimu commented Nov 19, 2020

Copy link
Copy Markdown
Member Author

Kind of stuck here and need some help. Thx~

Comment thread conf/config-default.yaml
# - "::/64"
# port_admin: 9180 # use a separate port
https_admin: true # enable HTTPS when use a separate port for Admin API.
# https_admin: true # enable HTTPS when use a separate port for Admin API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the default value?

@Yiyiyimu Yiyiyimu Nov 19, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

nil I think

@nic-chen

Copy link
Copy Markdown
Member

@Yiyiyimu

It is another issue now.

I think it is caused by ssl not being enabled by default:
https://github.com/apache/apisix/pull/2112/files#diff-8d872babc717e9d733641b56bfc530ef98751fbe4e68f08d79b2b83109c22fffR105

You can modify the test case accordingly and move on.

Signed-off-by: yiyiyimu <[email protected]>
@Yiyiyimu

Copy link
Copy Markdown
Member Author

Thank you for the fix and problem pointing out @nic-chen! All test passed and ready to be reviewed~

Comment thread conf/config-default.yaml Outdated
# ssl_trusted_certificate: /path/to/ca-cert # Specifies a file path with trusted CA certificates in the PEM format
# used to verify the certificate when APISIX needs to do SSL/TLS handshaking
# with external services (e.g. etcd)
ssl_cert: ""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here is the path or content of the certificate? we need to add comments.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you! fixed

@Yiyiyimu Yiyiyimu changed the title chore: move conf/cert to t/certs fix: move conf/cert to t/certs and disable ssl by default Nov 20, 2020
@moonming

Copy link
Copy Markdown
Member

@nic-chen please take a look

@moonming
moonming merged commit 36162e3 into apache:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

request help: move conf/cert to t/cert

6 participants