Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR adds a ChangesCertificate Debugging
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Operator
participant DebugCertCommand
participant CertificateSource
participant certdisplay
Operator->>DebugCertCommand: run debug cert-inspect
DebugCertCommand->>CertificateSource: retrieve file, Secret, Certificate resource, or directory certificate
CertificateSource-->>DebugCertCommand: certificate bytes and metadata
DebugCertCommand->>certdisplay: parse and format certificate information
certdisplay-->>Operator: formatted certificate output
Merge Risk: ⚪ Minimal · up to This change adds a new read-only certificate inspection subcommand for debugging. Existing behavior is unaffected, and no blocking problems were found in the certificate collection or display paths. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3ca8a54f-8e13-4328-8ee7-d77e5f804bfb
📒 Files selected for processing (8)
internal/cmd/skupper/common/flags.gointernal/cmd/skupper/debug/cert/display.gointernal/cmd/skupper/debug/cert/display_test.gointernal/cmd/skupper/debug/debug.gointernal/cmd/skupper/debug/debug_test.gointernal/cmd/skupper/debug/kube/cert.gointernal/cmd/skupper/debug/kube/cert_test.gointernal/cmd/skupper/debug/nonkube/cert.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Run go fmt on cert_test.go, include email/URI SANs in cert output, and fix non-kube input cert deduplication by display name.
|
Suggest sub-command name convey the scope e.g. "cert-inspect" or similar |
There was a problem hiding this comment.
Along with my other comments, I have two other suggestions that could be useful here:
- Flag to export the public certificate as a file
Something like --dump-certificate=tls.crt.
- Flag to validate-cert-authority
If the provided certificate is a CA we could also verify if it has authority over the provided --validate-cert-authority=tls.crt.
@ajssmith @AryanP123 let me know your thoughts on this. And if you guys believe it is useful, feel free to use better names for the suggested flags.
| } | ||
|
|
||
| if cmd.certName != "" { | ||
| certificate, err := cmd.Client.Certificates(cmd.Namespace).Get(context.TODO(), cmd.certName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Should we return an error here, requiring a "Certificate" resource to exist, or should we try to read a Secret with the given name?
The reason why I am asking this, is to validate a Secret resource that might be used by a Link resource, which won't have a corresponding Certificate resource.
| basePath api.InternalPath | ||
| prefix string | ||
| }{ | ||
| {api.CertificatesPath, ""}, |
There was a problem hiding this comment.
Also include: api.IssuersPath, api.InputIssuersPath.
Along with that, I have a similar comment to add.
System site users can also provide custom certificates through Secrets.
Maybe we should also include Secrets provided through: api.InputSiteStatePath.
There was a problem hiding this comment.
Thanks @fgiorgetti makes sense.
For kube, I'll try the Certificate CR first, then fall back to a TLS Secret with the same name, and include standalone TLS secrets in list output.
For non-kube, I'll add runtime/issuers, input/issuers, and TLS secrets from input/resources.
Happy to do --dump-certificate / --validate-cert-authority in a follow-up unless you want them here in this PR.
There was a problem hiding this comment.
@AryanP123 it can be done separately for sure, thanks!
Rename cert to cert-inspect, fall back to TLS Secrets on kube, and scan issuers and input resource secrets on non-kube.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: f2429f07-994b-489a-8d7b-fa621d013e89
📒 Files selected for processing (4)
internal/cmd/skupper/debug/debug.gointernal/cmd/skupper/debug/kube/cert.gointernal/cmd/skupper/debug/kube/cert_test.gointernal/cmd/skupper/debug/nonkube/cert.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
Given that some users bring their own certs, does it make sense to also report 'signing capability', or as LLM says: Since |
Surface Is Signing Cert from Certificate CR (kube) or issuers path (non-kube), and only ignore missing directories when scanning certs.
Fixes #2569
Summary by CodeRabbit
debug cert-inspectcommand for inspecting certificates.