feat: set certificate size limits in CTLog #1243
Conversation
Reviewer's GuideIntroduce a configurable maximum certificate chain size in CTLog by adding a new spec field, updating CRD schemas, and passing the flag into the deployment container arguments. ER diagram for CTlog CRD schema updateerDiagram
CTlogSpec {
int64 maxCertChainSize
}
Class diagram for updated CTlogSpec structclassDiagram
class CTlogSpec {
TLS TLS
+int64 MaxCertChainSize
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fghanmi - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `internal/controller/ctlog/actions/deployment.go:152` </location>
<code_context>
}
container.Args = appArgs
+ container.Args = append(container.Args, "--max_cert_chain_size", fmt.Sprintf("%d", *instance.Spec.MaxCertChainSize))
volumeMount := kubernetes.FindVolumeMountByNameOrCreate(container, volumeName)
</code_context>
<issue_to_address>
Potential nil pointer dereference for MaxCertChainSize.
Add a nil check for MaxCertChainSize before dereferencing, or use a default value if it is nil, to prevent a runtime panic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| container.Args = appArgs | ||
| container.Args = append(container.Args, "--max_cert_chain_size", fmt.Sprintf("%d", *instance.Spec.MaxCertChainSize)) |
There was a problem hiding this comment.
issue (bug_risk): Potential nil pointer dereference for MaxCertChainSize.
Add a nil check for MaxCertChainSize before dereferencing, or use a default value if it is nil, to prevent a runtime panic.
There was a problem hiding this comment.
Please add a nil check before dereferencing values. We've encountered bugs previously due to improper handling of nil values like this. The simplest approach would be to conditionally add the --max... statement only when the value is not nil.
There was a problem hiding this comment.
You're absolutely right—since MaxCertChainSize is a pointer, it can be nil if not set in the CR, even though the CRD and Go struct specify a default. The safest approach is to only append the --max_cert_chain_size argument if MaxCertChainSize is not nil. Here’s a suggested code snippet for the nil check:
if instance.Spec.MaxCertChainSize != nil {
container.Args = append(container.Args, "--max_cert_chain_size", fmt.Sprintf("%d", *instance.Spec.MaxCertChainSize))
}This will prevent any nil pointer dereference and runtime panics. If you want to always set a value, you could also provide a fallback default in the else branch, but the above is the minimal and safest fix. Let me know if you need a more defensive pattern or want to discuss defaulting strategies!
There was a problem hiding this comment.
I thought that since we have a default value of MaxCertChainSize, it won't be nil.
There was a problem hiding this comment.
That's a great question! While the CRD does specify a default for maxCertChainSize, this default is only applied by the Kubernetes API server when a resource is created or updated through the API. In the Go struct, MaxCertChainSize is a pointer (*int64), and if the field is omitted in the YAML or the object is created programmatically (e.g., in tests or by direct instantiation), the pointer can still be nil.
So, unless you can guarantee that every instance of the struct always comes from the API server (with defaulting applied), it's safest to check for nil before dereferencing. This is a common pattern in Kubernetes controllers to avoid panics from nil pointers.
Let me know if you'd like more details on how defaulting works or how to handle this defensively in your code!
osmman
changed the title
osmman
changed the title
2 participants
Summary by Sourcery
Add configurable maximum certificate chain size support for CTLog by introducing a new CRD parameter and passing it to the CTFE deployment.
New Features: