Support custom paths for cli cert checks #31988
Conversation
The `HasCertificate` check now requires the parsed services.xml object.
| if pkg.HasCertificate() { | ||
| servicesXML, err := readServicesXML(pkg) | ||
| if err != nil { | ||
| return fmt.Errorf("Error reading services.xml: %w", err) |
There was a problem hiding this comment.
Nit: Error messages should be lower-case as this may be concatenated with other errors further up.
| for _, certPath := range certPaths { | ||
| if ap.hasFile(certPath) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
This should check that all files are present, not just one.
| } | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func (ap *ApplicationPackage) HasMatchingCertificate(certificatePEM []byte) (bool, error) { |
There was a problem hiding this comment.
This needs to consider all certificates now, not just clients.pem. I think ApplicationPackage could have a method that reads the certificate paths from services XML, then both HasCertificate and HasMatchingCertificate can use this method and you won't have to pass around certPaths everywhere.
There was a problem hiding this comment.
Thanks @mpolden, I'll look at refactoring for this. The issue I had originally was that the xml/config.go file imports the vespa package, leading to a circular import if vespa/application.go wants to make use of the file parsing from xml/config.go. I'll revisit and see what I can do to make it work
There was a problem hiding this comment.
Hey, I've been playing around with this and think I could do with some steer on solving it.
I've been trying to get around the circular imports by changing the vespa package. xml/config.go imports System, from vespa, but moving that to its own package ended up with a lot of files changed without solving the issue. I also tried extracting ApplicationPackage, but again couldn't get that to work on its own. There might be a way of solving it this way but I don't feel confident enough to do it as it seems it would take a more dramatic refactoring then I'm comfortable with.
The only other option I could think of was to recreate some of the xml parsing functionality within ApplicationPackage. It wouldn't be very dry, but would enable this. What do you think is the best way forward?
(btw I'm on the vespa slack so let me know if its easier to chat this through there)
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.
This aims to resolve: #31978
I'm hoping I took this in the right direction! I was keen to give this a go. I'm very happy to take feedback and/or see maintainers make necessary changes to this, etc.
The tldr behaviour that i've sought to add is that cert checks will now use the cert paths defined in
services.xmlif any are found, otherwise it looks for the default cert path.Look forward to hearing your review!