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: fix and optimize tls in upstream_schema #10269

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

wzy0618
Copy link
Contributor

@wzy0618 wzy0618 commented Sep 27, 2023

Description

Removed two redundant configurations.
Fixed the wrong variables mentioned in the issue.

Fixes #10260

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@Revolyssup Revolyssup added the bug Something isn't working label Sep 27, 2023
@Revolyssup Revolyssup self-requested a review September 27, 2023 08:27
@@ -402,16 +402,10 @@ local upstream_schema = {
},
},
dependencies = {
client_cert = {
required = {"client_key"},
["not"] = {required = {"client_cert_id"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you remove the not condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

@soulbird please help to check this

Copy link
Contributor

Choose a reason for hiding this comment

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

The not condition is not required at both places client_cert and client_cert_id, having the not in clien_cert_id is enough to make both fields mutually exclusive. @monkeyDluffy6017

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @Revolyssup said, there should only be the following three cases of the three properties(client_cert_id, client_cert, client_key) here:

  1. None of them exist
  2. Only client_cert, client_key
  3. Only client_cert_id
    The modified conditions can ensure that "client_cert" and "client_key" must exist at the same time, and three properties will not exist at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some test cases to cover these conditions?

Revolyssup
Revolyssup previously approved these changes Sep 27, 2023
@monkeyDluffy6017
Copy link
Contributor

hi @wzy0618 , are you still working on this?

@wzy0618
Copy link
Contributor Author

wzy0618 commented Oct 9, 2023

hi @wzy0618 , are you still working on this?

yeah, I just finished my holiday and I will submit it this week.

@monkeyDluffy6017 monkeyDluffy6017 merged commit 11ee894 into apache:master Oct 12, 2023
31 checks passed
Revolyssup pushed a commit to Revolyssup/apisix that referenced this pull request Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug: wrong json schema configuration
4 participants