-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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(certificates): validate the certificates schema failed if snis
was in the request body
#13357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective it would be better to keep consistent logic between real creation of certificates entity and validation of entity rather than simply removing sni
field. To do this, we can refer to/reuse the same validation logics which are adopted in the dao/certificates and dao/sni
The necessity I think is to ensure that entity which is proved to be valid by endpoint /validate can be accepted by the upcoming post method.
Nice. |
To this extent, the /validate endpoint just carry out some preliminary validations, which also makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Convert to draft for now as it requires more design discussion. |
0c81566
to
69b55e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Let's have someone from the Gateway team review the code as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether transient
is the best name here 🤔 . What I think we're trying to achieve is adding a field to schema that's actually the reverse of type = "foreign"
what sometimes is called backref
.
transient
sounds to me like a field that could be derived from other fields. For example we'd have somewhere a network address field which values would be:
{
network_address = "127.0.0.1:8001"
}
Then transient
fields might be:
{
network_address = "127.0.0.1:8001",
ip = "127.0.0.1",
port = 8001
}
something that exists in another field but is returned for convenience and is not actually stored in database.
In future it might open us a door to realize backref
dereferencing. Like certificates:each({ with = { "snis" } })
. This way the certificates
entity could pull also snis
. Something that right now is done by: SNIS:list_for_certificate
but could be just an addition to DAO functionality if we defined the interface in schema.
@nowNick |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok the Java explanation makes sense 👍 In the future I think we might benefit from backref type in metaschema but let's move one step at a time.
Rebased on master as CI is always red |
Schema changed. CC @GGabriele |
@raoxiaoyan IMHO, the |
This method adds schema field and allows Suppose a scenario:
Thus, we are in a situation where the validation API passes but the SSL handshake fails. |
I didn't catch it. What do you mean? |
671be51
to
d1b7864
Compare
…was in the request body
Successfully created cherry-pick PR for |
…less mode (#13516) This PP is to fix the problem that was caused by #13357. The problem was recorded here. Kong/kubernetes-ingress-controller#6370
Summary
the endpoint POST
/schemas/certificates/validate
with entity data:It throws an error.
Currently, the endpoint POST
/certificates
can batch-create sni entities withsnis
. But thesnis
is not part of the certificate schema. so we need to remove it from the request body.Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
KM-223