feat: SECURESIGN-3184: ensure ServiceMonitors are only created when r… #1466
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…unning on OpenShift or explicitly configured Signed-off-by: Kevin Conner <kev.conner@gmail.com>
Reviewer's GuideAdds a configurable ServiceMonitor flag to monitoring configuration, wires it through CRDs and controllers so ServiceMonitor resources are only created on OpenShift by default or when explicitly enabled, and updates deepcopy logic plus tests to support the new optional field. Updated class diagram for monitoring configuration and related specsclassDiagram
class MonitoringConfig {
bool Enabled
bool* ServiceMonitor
bool IsServiceMonitorEnabled(defaultVal bool)
}
class MonitoringWithTLogConfig {
+MonitoringConfig MonitoringConfig
+TLogConfig TLog
}
class CTlogSpec {
+MonitoringConfig Monitoring
+TrillianSpec Trillian
+[]SecretKeySelector Secrets
+LocalObjectReference* ServerConfigRef
}
class FulcioSpec {
+MonitoringConfig Monitoring
+CTlogSpec Ctlog
+FulcioConfig Config
+FulcioCertificateConfig Certificate
+LocalObjectReference* TrustedCA
}
class RekorSpec {
+MonitoringConfig Monitoring
+TrillianSpec Trillian
+ExternalAccessConfig ExternalAccess
+RekorSearchUIConfig RekorSearchUI
+SignerConfig Signer
+AttestationsConfig Attestations
}
class TimestampAuthoritySpec {
+MonitoringConfig Monitoring
+PodRequirementsConfig PodRequirements
+ExternalAccessConfig ExternalAccess
+SignerConfig Signer
+LocalObjectReference* TrustedCA
}
class TrillianSpec {
+MonitoringConfig Monitoring
+TrillianDBConfig Db
+TrillianLogServerConfig LogServer
+TrillianLogSignerConfig LogSigner
+LocalObjectReference* TrustedCA
}
class RekorMonitoring {
+MonitoringWithTLogConfig Monitoring
}
class Rekor {
+RekorSpec Spec
}
class CTlog {
+CTlogSpec Spec
}
class Fulcio {
+FulcioSpec Spec
}
class TimestampAuthority {
+TimestampAuthoritySpec Spec
}
class Trillian {
+TrillianSpec Spec
}
MonitoringWithTLogConfig --> MonitoringConfig : embeds
RekorMonitoring --> MonitoringWithTLogConfig : has
CTlogSpec --> MonitoringConfig : has
FulcioSpec --> MonitoringConfig : has
RekorSpec --> MonitoringConfig : has
TimestampAuthoritySpec --> MonitoringConfig : has
TrillianSpec --> MonitoringConfig : has
Rekor --> RekorSpec : has
CTlog --> CTlogSpec : has
Fulcio --> FulcioSpec : has
TimestampAuthority --> TimestampAuthoritySpec : has
Trillian --> TrillianSpec : has
Flow diagram for deciding ServiceMonitor creation in resource monitoring actionsflowchart TD
A["Start monitoringAction.CanHandle"] --> B["Check Ready condition reason is Creating or Ready"]
B -->|No| Z["Do not create ServiceMonitor"]
B -->|Yes| C["Check Spec.Monitoring.Enabled == true"]
C -->|No| Z
C -->|Yes| D["Call kubernetes.IsOpenShift() to determine platform"]
D --> E["Call MonitoringConfig.IsServiceMonitorEnabled(defaultVal)"]
subgraph IsServiceMonitorEnabled_logic
direction LR
E --> F{ServiceMonitor is set?}
F -->|Yes| G["Return value of ServiceMonitor"]
F -->|No| H["Return defaultVal based on platform"]
end
G --> I{Result is true?}
H --> I
I -->|No| Z
I -->|Yes| Y["CanHandle returns true, reconciliation creates or updates ServiceMonitor resources"]
Z["CanHandle returns false, skip ServiceMonitor reconciliation"]
Y --> J["End"]
Z --> J
Flow diagram for MonitoringConfig.IsServiceMonitorEnabled behavior across platformsflowchart TD
A["Start IsServiceMonitorEnabled(defaultVal)"] --> B{ServiceMonitor field is nil?}
B -->|No| C["Return *ServiceMonitor (explicit user choice)"]
B -->|Yes| D["Use defaultVal provided by caller"]
subgraph Caller_examples
direction LR
E["On OpenShift"] --> F["Caller passes defaultVal = true"]
G["On non OpenShift Kubernetes"] --> H["Caller passes defaultVal = false"]
end
D --> I["Return defaultVal (platform-specific default)"]
C --> J["End"]
I --> J
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:
- The IsServiceMonitorEnabled helper takes a generic default value, but the tests and call sites treat it as an
isOpenShiftflag; consider renaming the parameter and updating test names to reflect that it is a default value rather than a platform indicator to avoid confusion for future readers. - The repeated
CanHandlecondition combining Ready status,Monitoring.Enabled, andIsServiceMonitorEnabled(kubernetes.IsOpenShift())across multiple controllers could be factored into a shared helper to reduce duplication and keep the monitoring enablement logic in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The IsServiceMonitorEnabled helper takes a generic default value, but the tests and call sites treat it as an `isOpenShift` flag; consider renaming the parameter and updating test names to reflect that it is a default value rather than a platform indicator to avoid confusion for future readers.
- The repeated `CanHandle` condition combining Ready status, `Monitoring.Enabled`, and `IsServiceMonitorEnabled(kubernetes.IsOpenShift())` across multiple controllers could be factored into a shared helper to reduce duplication and keep the monitoring enablement logic in one place.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:
|
||||||||||||
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
…unning on OpenShift or explicitly configured
@osmman This is the alternative approach to handling it, however this suffers from two not insignificant problems
The #1440 pull request, while more complicated, handles this in the right way for kubernetes. That PR makes the service monitor resources a soft dependency for our operator, reacting dynamically to the changes within the cluster to determine when/if the service monitor resources should be created.
PR Type
Enhancement
Description
Add optional
ServiceMonitorfield toMonitoringConfigfor platform-aware controlImplement
IsServiceMonitorEnabled()method with OpenShift-aware defaultsUpdate all monitoring action handlers to check ServiceMonitor enablement
Fix deep copy functions for proper pointer handling in monitoring configs
Update CRD schemas to reflect new optional ServiceMonitor field
Diagram Walkthrough
File Walkthrough
8 files
Add ServiceMonitor field and helper methodAdd ServiceMonitor enablement check to CTlogAdd ServiceMonitor enablement check to FulcioAdd ServiceMonitor enablement check to RekorAdd ServiceMonitor enablement check to Rekor serverAdd ServiceMonitor enablement check to Trillian logserverAdd ServiceMonitor enablement check to Trillian logsignerAdd ServiceMonitor enablement check to TimestampAuthority1 files
Add comprehensive tests for ServiceMonitor logic1 files
Fix deep copy for MonitoringConfig pointer fields6 files
Update CTlog CRD with ServiceMonitor fieldUpdate Fulcio CRD with ServiceMonitor fieldUpdate Rekor CRD with ServiceMonitor fieldUpdate SecureSign CRD with ServiceMonitor fieldsUpdate TimestampAuthority CRD with ServiceMonitor fieldUpdate Trillian CRD with ServiceMonitor field