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.

Sign up for GitHub

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

support cert by reference (#268) #269

Merged
merged 7 commits into from
Feb 13, 2021
Merged

support cert by reference (#268) #269

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6b484b
support cert by reference
Feb 4, 2021
cb5f5ef
fix lint issues
Feb 4, 2021
8f6af95
upd to pull in all available certs into state for validation
Feb 9, 2021
8f29981
fix lint issues
Feb 9, 2021
27a07d6
rollback proposed change and remove validation
Feb 10, 2021
45eac8f
upd builder test
Feb 10, 2021
99c0a8b
cleanup unused code for certID
Feb 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions file/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,15 @@ func (b *stateBuilder) services() {

if s.ClientCertificate != nil && !utils.Empty(s.ClientCertificate.ID) {
if _, ok := b.certIDs[*s.ClientCertificate.ID]; !ok {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, true! Have removed those lines.

b.err = errors.Errorf("client certificate not found: %v",
*s.ClientCertificate.ID)
return
_, err := b.currentState.Certificates.Get(*s.ClientCertificate.ID)
if err == state.ErrNotFound {
b.err = errors.Errorf("client certificate not found: %v",
*s.ClientCertificate.ID)
return
} else if err != nil {
b.err = err
return
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ikethecoder ikethecoder Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I retested and you are correct - this looks like it is a bit more involved to get it supported :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the prompt response here.

The current patch doesn't seem acceptable to me:

  • --select-tags is 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.

}
}

Expand Down
72 changes: 68 additions & 4 deletions file/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package file

import (
"encoding/hex"
"fmt"
"math/rand"
"os"
"reflect"
Expand Down Expand Up @@ -281,9 +282,11 @@ func Test_stateBuilder_services(t *testing.T) {
currentState *state.KongState
}
tests := []struct {
name string
fields fields
want *utils.KongRawState
name string
fields fields
wantErr bool
err string
want *utils.KongRawState
}{
{
name: "matches ID of an existing service",
Expand Down Expand Up @@ -345,6 +348,61 @@ func Test_stateBuilder_services(t *testing.T) {
},
},
},
{
name: "process an existing client certificate for service",
fields: fields{
targetContent: &Content{
Services: []FService{
{
Service: kong.Service{
Name: kong.String("foo"),
ClientCertificate: &kong.Certificate{
ID: kong.String("4bfcb11f-c962-4817-83e5-9433cf20b663"),
},
},
},
},
},
currentState: existingCertificateState(),
},
wantErr: false,
want: &utils.KongRawState{
Services: []*kong.Service{
{
ClientCertificate: &kong.Certificate{
ID: kong.String("4bfcb11f-c962-4817-83e5-9433cf20b663"),
},
ID: kong.String("5b1484f2-5209-49d9-b43e-92ba09dd9d52"),
Name: kong.String("foo"),
Port: kong.Int(80),
Protocol: kong.String("http"),
ConnectTimeout: kong.Int(60000),
WriteTimeout: kong.Int(60000),
ReadTimeout: kong.Int(60000),
},
},
},
},
{
name: "process a non-existent client certificate for service",
fields: fields{
targetContent: &Content{
Services: []FService{
{
Service: kong.Service{
Name: kong.String("foo"),
ClientCertificate: &kong.Certificate{
ID: kong.String("00000000-c962-4817-83e5-9433cf20b663"),
},
},
},
},
},
currentState: emptyState(),
},
wantErr: true,
err: "client certificate not found: 00000000-c962-4817-83e5-9433cf20b663",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -355,7 +413,13 @@ func Test_stateBuilder_services(t *testing.T) {
d, _ := utils.GetKongDefaulter()
b.defaulter = d
b.build()
assert.Equal(tt.want, b.rawState)

if (b.err != nil) && tt.wantErr {
assert.Equal(tt.err, fmt.Sprintf("%v", b.err))
} else {
assert.Equal(tt.want, b.rawState)
}

})
}
}
Expand Down