support cert by reference (#268) #269
Conversation
|
|
file/builder.go
Outdated
| } else if err != nil { | ||
| b.err = err | ||
| return | ||
| } |
There was a problem hiding this comment.
Could you explain what is the case that you are trying to fix here?
I can read the diff but I can't understand why this is needed.
There was a problem hiding this comment.
Hi Harry, sure.. the particular scenario we faced was that we want to setup mTLS between a Service and the Upstream endpoint, so a Client Cert/Key needs to be configured on the Gateway so that the Service can reference it. We also want to separate concerns so that the Client Cert/Key is updated in a different process than the Service/Route - for example having the Cert/Key updated/rotated by a security team, and the Service/Route updated by an application team. We use the --select-tag option as a way to segment configuration by different application teams. There is an example in issue #268 as well.
There was a problem hiding this comment.
Ah, I missed the reference to that issue before (please include it in your PR message body next time).
So the use case you are trying to achieve makes a lot of sense.
Have you actually tested this patch against it?
The reason I ask is because currentState shouldn't have the Certificate either because it won't be part of the configuration based on select-tag.
I could be missing something here, it has been a while since I have dug in here.
There was a problem hiding this comment.
I have tested it; and there are a couple of unit tests included. You are correct that the currentState may not have the Certificate and that is why the extra check is done - it will throw an error if: (1) the Certificate is missing from the select-tag config or (2) the Certificate is missing from the currentState. It has to be in one or the other.
There was a problem hiding this comment.
Ah sorry - I follow what you are saying now - you are saying that currentState is scoped as well by select-tag. Let me test that again then and see what is going on.
There was a problem hiding this comment.
Yes, I retested and you are correct - this looks like it is a bit more involved to get it supported :(.
There was a problem hiding this comment.
Harry, have a look at the latest commit and see if you agree with the solution. I added code to pull in all certs into the state in a different table (allAvailableCertificates) and use that to validate the existence of the cert. If its not suitable, my only other option that I can think of would be to remove the certificate validation all together (basically would behave similarly to the ca_certificates attribute).
There was a problem hiding this comment.
Thanks for the prompt response here.
The current patch doesn't seem acceptable to me:
--select-tagsis a hard filter. decK shouldn't go outside it's scope in most cases. Going out of scope for validation seems wrong.- You made a very good and helpful point around ca_certificate attribute. We should be treating both as same.
Amongst the two options, I think removing validation makes the most sense here. I do not like reducing validation but given the safety net that this will be caught by Kong's API later, this is okay.
Foreign-key references are hard to catch in decK and are performed on a best effort basis. I also think it makes sense to add the validation you have in your patch but it requires some more machinery in the code-base to ensure maintainability. Managing in-memory state in decK is very hairy and tricky, I don't want to further increase the tech debt in this area.
@ikethecoder Could you update the patch to remove the validation of Certificate from Service resource?
Note for future-selves:
Even though we are relaxing validation here, the same argument can't be used for relaxing validation for route-service or route/service/consumer-plugin foreign relations. Certificate and Service entity are loosely related when it comes to operations. There is a compelling use-case to relax this validation. I do not think separating routes from services or plugins from where are used makes much sense and should be avoided at all cost and is in the best interest of all users. Separating out these strongly related entities makes decK and Kong's UX much worse and can easily become a maintenance nightmare.
|
@hbagdi Great thanks Harry, I've updated the PR to just remove the validation check. Please review. |
| @@ -430,14 +430,6 @@ func (b *stateBuilder) services() { | |||
| utils.MustMergeTags(&s.Service, b.selectTags) | |||
| b.defaulter.MustSet(&s.Service) | |||
|
|
|||
| if s.ClientCertificate != nil && !utils.Empty(s.ClientCertificate.ID) { | |||
| if _, ok := b.certIDs[*s.ClientCertificate.ID]; !ok { | |||
There was a problem hiding this comment.
It seems b.certIDs was used only in this one place and can now be completely removed.
Can you delete the following lines as well?
20: certIDs map[string]bool
48: b.certIDs = map[string]bool{}
109: b.certIDs[*c.ID] = true
There was a problem hiding this comment.
Ah yes, true! Have removed those lines.
|
Thank you for your contribution. Please fill out the following form to claim your contributor swag: |
No description provided.