feat: add support for image pull secrets for all components #1427
Conversation
Reviewer's GuideThis PR extends the operator to support configurable image pull secrets by augmenting CRD schemas and Spec types, adding deep copy logic and a merge utility, and integrating the secrets into RBAC ServiceAccounts and subresource ensure logic. ER diagram for imagePullSecrets in CRDserDiagram
SECURESIGN_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
CTLOG_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
FULCIO_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
REKOR_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
TRILLIAN_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
TIMESTAMPAUTHORITY_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
TUF_SPEC ||--o{ LOCAL_OBJECT_REFERENCE : "imagePullSecrets"
LOCAL_OBJECT_REFERENCE {
string name
}
Class diagram for updated Spec types with imagePullSecretsclassDiagram
class SecuresignSpec {
Tuf: TufSpec
Ctlog: CTlogSpec
TimestampAuthority: TimestampAuthoritySpec
ImagePullSecrets: LocalObjectReference[]
}
class TufSpec {
Pvc: TufPvc
ImagePullSecrets: LocalObjectReference[]
}
class CTlogSpec {
MaxCertChainSize: int64
ImagePullSecrets: LocalObjectReference[]
}
class FulcioSpec {
TrustedCA: LocalObjectReference
ImagePullSecrets: LocalObjectReference[]
}
class RekorSpec {
MaxRequestBodySize: int64
ImagePullSecrets: LocalObjectReference[]
}
class TrillianSpec {
MaxRecvMessageSize: int64
ImagePullSecrets: LocalObjectReference[]
}
class TimestampAuthoritySpec {
MaxRequestBodySize: int64
ImagePullSecrets: LocalObjectReference[]
}
SecuresignSpec --> TufSpec
SecuresignSpec --> CTlogSpec
SecuresignSpec --> TimestampAuthoritySpec
TufSpec --> TufPvc
FulcioSpec --> LocalObjectReference
CTlogSpec --> LocalObjectReference
RekorSpec --> LocalObjectReference
TrillianSpec --> LocalObjectReference
TimestampAuthoritySpec --> LocalObjectReference
SecuresignSpec --> LocalObjectReference
TufSpec --> LocalObjectReference
Class diagram for rbacAction and WithImagePullSecretsclassDiagram
class rbacAction {
componentName: string
rbacName: string
rules: PolicyRule[]
canHandle: func(context.Context, T) bool
imagePullSecrets: func(context.Context, T) []LocalObjectReference
}
class WithImagePullSecrets {
<<function>>
}
rbacAction --> PolicyRule
rbacAction --> LocalObjectReference
rbacAction --> WithImagePullSecrets
Flow diagram for merging imagePullSecrets in ensure logicflowchart TD
A["SecuresignSpec.ImagePullSecrets"] --> C["MergeImagePullSecrets"]
B["ComponentSpec.ImagePullSecrets"] --> C
C --> D["ComponentSpec.ImagePullSecrets (merged)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- handleServiceAccount does not clear existing ImagePullSecrets when the function returns an empty slice or nil on updates—consider explicitly setting or clearing the field to avoid stale secrets.
- MergeImagePullSecrets builds the result using a map, causing non-deterministic ordering; consider preserving a stable order (e.g. base entries first, then overrides) for predictable output.
- The CRD schema additions for ImagePullSecrets are copy-pasted across all component specs—consider factoring out common definitions or using schema templates to reduce repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- handleServiceAccount does not clear existing ImagePullSecrets when the function returns an empty slice or nil on updates—consider explicitly setting or clearing the field to avoid stale secrets.
- MergeImagePullSecrets builds the result using a map, causing non-deterministic ordering; consider preserving a stable order (e.g. base entries first, then overrides) for predictable output.
- The CRD schema additions for ImagePullSecrets are copy-pasted across all component specs—consider factoring out common definitions or using schema templates to reduce repetition.
## Individual Comments
### Comment 1
<location> `api/v1alpha1/securesign_types.go:39-40` </location>
<code_context>
//+optional
MaxCertChainSize *int64 `json:"maxCertChainSize,omitempty"`
+
+ // ImagePullSecrets is an optional list of references to secrets for pulling container images.
+ //+optional
+ ImagePullSecrets []core.LocalObjectReference `json:"imagePullSecrets,omitempty"`
</code_context>
<issue_to_address>
**suggestion:** ImagePullSecrets field in SecuresignSpec is not marked as optional.
Adding '+optional' to ImagePullSecrets will ensure consistent CRD generation and validation, matching other spec fields.
```suggestion
// ImagePullSecrets is an optional list of references to secrets for pulling container images.
//+optional
ImagePullSecrets []core.LocalObjectReference `json:"imagePullSecrets,omitempty"`
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
| Ctlog CTlogSpec `json:"ctlog,omitempty"` | ||
| TimestampAuthority *TimestampAuthoritySpec `json:"tsa,omitempty"` | ||
|
|
||
| ServiceAccountRequirements `json:",inline"` |
There was a problem hiding this comment.
ImagePullSecrets for this CRD behave differently compare to other CRDs. It will require document that behavior and it will be good to provide some tests to not broke it in feature changes.
There was a problem hiding this comment.
Yes to documentation, I've already reached out to Aron about creating the docs issues. Or are you referring to having a comment?
I'll add some tests
There was a problem hiding this comment.
I add a comment to SecuresignSpec and some higher level tests to complement the lower level ones
There was a problem hiding this comment.
Unfortunately adding comment like you dud will not modify CRD's OpenAPI which is main source for documentation of CRDs.
For example:
oc explain securesign.spec
There was a problem hiding this comment.
Ah okay, now I understand what you are after. I'll take a look
There was a problem hiding this comment.
@osmman crdify is complaining about the extra text, so I've reverted that
There was a problem hiding this comment.
I know about the limitations of inlining that is reason why I give you two options how it could be solved. I do not see any changes in SecuresignSpec which will document usage of that parameter for Securesign CRD.
Crdify failures in descriptions aro not problem, these are mainly introduces from kubernetes changes in API spec and most of time could be waived.
There was a problem hiding this comment.
okay, I can waive them.
There was a problem hiding this comment.
Looking at this previous changes, it seems to be a different issue and I rushed the change while at KubeCon. I'll fix it properly and update the PR.
Update: double checking the change it was correct, so it does need waiving
There was a problem hiding this comment.
@osmman I've worked out how to waive the crdify checks for documentation, the PR has been updated to include that change
knrc
force-pushed
the
securesign-3185
branch
5 times, most recently
from
9cf907a to
3387d51
Compare
internal/action/rbac/action.go
Outdated
| rbacName string | ||
| rules []rbacv1.PolicyRule | ||
| canHandle func(context.Context, T) bool | ||
| imagePullSecrets func(context.Context, T) []v1.LocalObjectReference |
There was a problem hiding this comment.
Please remove context.Context parameter from function. The parameter is not used and I do not expect that will be useful in feature.
There was a problem hiding this comment.
It's usually good practice to pass that in, for example many functions may get the logger from the context. This is presumably the reason for canHandle doing the same.
There was a problem hiding this comment.
I appreciate the point about passing context.Context for future use cases like logging. However, I'd still recommend removing it for now because there is no current usage. We can easily add it back when need arises.
There was a problem hiding this comment.
I'll keep it in, if it needs to be used then I can add logging in
There was a problem hiding this comment.
@osmman I decided just to remove the context, as you say this can be added back in future if necessary.
knrc
force-pushed
the
securesign-3185
branch
from
3387d51 to
40ec690
Compare
Signed-off-by: Kevin Conner <kev.conner@gmail.com>
knrc
force-pushed
the
securesign-3185
branch
from
40ec690 to
d782f49
Compare
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
2 participants
This pull request adds support for image pull secrets, with the ability to specify them at the top level (inherited by all) or within individual components.
Summary by Sourcery
Add support for image pull secrets across all operator components
New Features:
Enhancements:
Tests: