Skip to content

Conversation

@fghanmi
Copy link
Member

@fghanmi fghanmi commented Nov 18, 2025

Summary by Sourcery

Enable optional monitoring of CT log transparency service by adding configurable TLog monitor resources (statefulset, service, and service monitor), updating CRDs, images, and controller logic.

New Features:

  • Add optional TLog monitor StatefulSet, Service, and ServiceMonitor for CT log transparency logs.
  • Expose 'tlog' configuration in CTlog and SecureSign CRDs to enable/disable monitoring and set check intervals.
  • Introduce RELATED_IMAGE_CTLOG_MONITOR image reference and corresponding CLI flag.

Enhancements:

  • Extend the CTlog controller reconciliation to include new monitor actions and update constants for monitor components and ports.
  • Update kustomization, manager images patch, and internal images list to support CTlog monitor resources.

@sourcery-ai
Copy link

sourcery-ai bot commented Nov 18, 2025

Reviewer's Guide

Adds configurable CTlog monitoring by extending operator constants, CRD schemas, default config, and CTlog controller, and implementing a new monitor sub-package with statefulset, service, and ServiceMonitor actions.

Entity relationship diagram for updated CTlog CRD monitoring configuration

erDiagram
    CTlog {
        string name
        MonitoringWithTLogConfig monitoring
    }
    MonitoringWithTLogConfig {
        MonitoringConfig config
        TLogConfig tlog
    }
    TLogConfig {
        boolean enabled
        string interval
    }
    CTlog ||--|{ MonitoringWithTLogConfig : has
    MonitoringWithTLogConfig ||--|{ TLogConfig : has
Loading

Class diagram for new CTlog monitor actions

classDiagram
    class statefulSetAction {
        +Name() string
        +CanHandle(context.Context, CTlog) bool
        +Handle(context.Context, CTlog) *action.Result
        -ensureTLS(tlsConfig, name)
        -ensureMonitorStatefulSet(instance, sa, labels, ctlogServerHost, tufServerHost)
        -ensureInitContainer(ctlogServerHost, tufHost)
    }
    class createServiceAction {
        +Name() string
        +CanHandle(context.Context, CTlog) bool
        +Handle(context.Context, CTlog) *action.Result
    }
    class monitoringAction {
        +Name() string
        +CanHandle(context.Context, CTlog) bool
        +Handle(context.Context, CTlog) *action.Result
    }
    class helper {
        +enabled(instance CTlog) bool
    }
    statefulSetAction --|> action.BaseAction
    createServiceAction --|> action.BaseAction
    monitoringAction --|> action.BaseAction
    helper ..> CTlog
Loading

Class diagram for updated CTlogSpec and monitoring config

classDiagram
    class CTlogSpec {
        +Monitoring MonitoringWithTLogConfig
    }
    class MonitoringWithTLogConfig {
        +TLog TLogConfig
    }
    class TLogConfig {
        +Enabled bool
        +Interval string
    }
    CTlogSpec --> MonitoringWithTLogConfig
    MonitoringWithTLogConfig --> TLogConfig
Loading

File-Level Changes

Change Details Files
Extend CTlog constants for monitoring
  • Add MonitorStatefulSetName and MonitorComponentName constants
  • Add MonitorCondition constant
  • Add MonitorMetricsPortName and MonitorMetricsPort constants
internal/controller/ctlog/actions/constants.go
Add tlog monitoring config to CRD schemas
  • Define tlog.enabled and tlog.interval properties in ctlogs CRD
  • Mirror tlog configuration additions in securesigns CRD
config/crd/bases/rhtas.redhat.com_ctlogs.yaml
config/crd/bases/rhtas.redhat.com_securesigns.yaml
Expose and patch RELATED_IMAGE_CTLOG_MONITOR
  • Add kustomize replacement for RELATED_IMAGE_CTLOG_MONITOR in default kustomization
  • Include RELATED_IMAGE_CTLOG_MONITOR in manager_images_patch
  • Register CTLogMonitor image in internal/images
  • Add ctlog-monitor-image flag in cmd/main
config/default/kustomization.yaml
config/default/manager_images_patch.yaml
internal/images/images.go
cmd/main.go
Invoke monitor actions in CTlog controller
  • Import monitor package
  • Append NewStatefulSetAction, NewCreateServiceAction, NewCreateMonitorAction to reconciliation sequence
internal/controller/ctlog/ctlog_controller.go
Update CTlogSpec Monitoring type for tlog config
  • Rename MonitoringConfig to MonitoringWithTLogConfig in CTlogSpec
api/v1alpha1/ctlog_types.go
Implement CTlog monitor sub-package with statefulset, service, and ServiceMonitor
  • Create statefulset action for monitor
  • Create service action
  • Create ServiceMonitor action
  • Add helper to enable monitoring
internal/controller/ctlog/actions/monitor/statefulset.go
internal/controller/ctlog/actions/monitor/svc.go
internal/controller/ctlog/actions/monitor/monitoring.go
internal/controller/ctlog/actions/monitor/helper.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 18, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Audit Coverage: The new monitoring actions and init flow do not explicitly log critical actions (e.g.,
creation of Service/StatefulSet/ServiceMonitor, init container steps) with user/context,
making it unclear if sufficient audit trails exist.

Referred Code
func (i statefulSetAction) Handle(ctx context.Context, instance *rhtasv1alpha1.CTlog) *action.Result {
	var (
		err    error
		result controllerutil.OperationResult
	)

	var protocol string
	if tls.UseTlsClient(instance) {
		protocol = "https"
	} else {
		protocol = "http"
	}

	ctlogServerHost := fmt.Sprintf("%s://%s.%s.svc", protocol, actions.ComponentName, instance.Namespace)
	tufServerHost := fmt.Sprintf("http://%s.%s.svc", tufConstants.ComponentName, instance.Namespace)

	labels := labels.For(actions.MonitorComponentName, actions.MonitorStatefulSetName, instance.Name)
	if result, err = kubernetes.CreateOrUpdate(ctx, i.Client,
		&v1.StatefulSet{
			ObjectMeta: metav1.ObjectMeta{
				Name:      actions.MonitorStatefulSetName,


 ... (clipped 35 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Init Curl Robustness: The init container uses curl polling without timeouts/backoff and lacks handling for TLS
verification toggles or unreachable hosts, which may cause indefinite waits without
actionable error context.

Referred Code
		initContainer.Command = []string{
			"/bin/sh",
			"-c",
			fmt.Sprintf(`
                echo "Waiting for ctlog-server...";
                until curl -sSf -k %s > /dev/null 2>&1; do
                    echo "ctlog-server not ready...";
                    sleep 5;
                done;

                echo "Waiting for TUF server...";
                until curl %s > /dev/null 2>&1; do
                    echo "TUF server not ready...";
                    sleep 5;
                done;

                echo "Downloading root.json";
                curl %s/root.json > %s/root.json

                echo "tuf-init completed."
            `, ctlogServerHost, tufHost, tufHost, mountPath),


 ... (clipped 1 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
TLS Handling: The init container unconditionally uses curl without -k for TUF and with -k only for ctlog
readiness, and the main container constructs URLs from namespace without validation, which
may create inconsistent TLS validation and potential insecure requests depending on
cluster setup.

Referred Code
		initContainer.Command = []string{
			"/bin/sh",
			"-c",
			fmt.Sprintf(`
                echo "Waiting for ctlog-server...";
                until curl -sSf -k %s > /dev/null 2>&1; do
                    echo "ctlog-server not ready...";
                    sleep 5;
                done;

                echo "Waiting for TUF server...";
                until curl %s > /dev/null 2>&1; do
                    echo "TUF server not ready...";
                    sleep 5;
                done;

                echo "Downloading root.json";
                curl %s/root.json > %s/root.json

                echo "tuf-init completed."
            `, ctlogServerHost, tufHost, tufHost, mountPath),


 ... (clipped 1 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes - here's some feedback:

  • In ctlog_controller.go you register both actions.NewCreateMonitorAction() and monitor.NewCreateMonitorAction(); remove or consolidate the redundant call to avoid running the wrong monitor action twice.
  • In statefulSetAction.ensureInitContainer you’re using the RekorMonitor image constant—swap that out for the CTLogMonitor image so the init container runs the correct binary.
  • The spec type MonitoringWithTLogConfig is quite verbose; consider renaming it (for example to TLogMonitoringConfig) to better align with existing config types and improve readability.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In ctlog_controller.go you register both actions.NewCreateMonitorAction() and monitor.NewCreateMonitorAction(); remove or consolidate the redundant call to avoid running the wrong monitor action twice.
- In statefulSetAction.ensureInitContainer you’re using the RekorMonitor image constant—swap that out for the CTLogMonitor image so the init container runs the correct binary.
- The spec type MonitoringWithTLogConfig is quite verbose; consider renaming it (for example to TLogMonitoringConfig) to better align with existing config types and improve readability.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Mount CA certificate for client

Modify the ensureTLS function to mount only the trusted CA certificate for the
client-side ctlog-monitor, instead of incorrectly mounting the server's private
key and certificate, to prevent a security risk.

internal/controller/ctlog/actions/monitor/statefulset.go [114-121]

 func (i statefulSetAction) ensureTLS(tlsConfig rhtasv1alpha1.TLS, name string) func(sts *v1.StatefulSet) error {
 	return func(sts *v1.StatefulSet) error {
-		if err := tlsensure.TLS(tlsConfig, name)(&sts.Spec.Template); err != nil {
+		if err := tlsensure.TrustedCA(tls.TrustedCASecretName, constants.TrustedCaCrtKey)(&sts.Spec.Template); err != nil {
 			return err
 		}
 		return nil
 	}
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a security vulnerability where the ctlog server's private key is unnecessarily mounted into the monitor pod, and proposes a correct fix to mount only the trusted CA certificate.

High
Remove insecure curl flag

Remove the insecure -k flag from the curl command in the init container's script
and use the mounted CA certificate for proper TLS validation to prevent
potential man-in-the-middle attacks.

internal/controller/ctlog/actions/monitor/statefulset.go [194-226]

 func (i statefulSetAction) ensureInitContainer(ctlogServerHost string, tufHost string) func(*v1.StatefulSet) error {
 	return func(ss *v1.StatefulSet) error {
 		initContainer := kubernetes.FindInitContainerByNameOrCreate(&ss.Spec.Template.Spec, "tuf-init")
 
 		initContainer.Image = images.Registry.Get(images.RekorMonitor)
 		volumeMount := kubernetes.FindVolumeMountByNameOrCreate(initContainer, storageVolumeName)
 		volumeMount.MountPath = mountPath
 
 		initContainer.Command = []string{
 			"/bin/sh",
 			"-c",
 			fmt.Sprintf(`
                 echo "Waiting for ctlog-server...";
-                until curl -sSf -k %s > /dev/null 2>&1; do
+                until curl -sSf --cacert %s/%s %s > /dev/null 2>&1; do
                     echo "ctlog-server not ready...";
                     sleep 5;
                 done;
 
                 echo "Waiting for TUF server...";
                 until curl %s > /dev/null 2>&1; do
                     echo "TUF server not ready...";
                     sleep 5;
                 done;
 
                 echo "Downloading root.json";
                 curl %s/root.json > %s/root.json
 
                 echo "tuf-init completed."
-            `, ctlogServerHost, tufHost, tufHost, mountPath),
+            `, constants.SecretMountPath, constants.TrustedCaCrtKey, ctlogServerHost, tufHost, tufHost, mountPath),
 		}
 		return nil
 	}
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a security issue where curl -k is used, disabling TLS verification, and provides a correct fix to enforce secure communication.

Medium
High-level
Refactor monitoring actions for clarity

Rename the new monitoring actions in the monitor sub-package to avoid naming
conflicts with existing actions. This will improve clarity and make the
controller logic easier to understand.

Examples:

internal/controller/ctlog/ctlog_controller.go [120-122]
		monitor.NewStatefulSetAction(),
		monitor.NewCreateServiceAction(),
		monitor.NewCreateMonitorAction(),
internal/controller/ctlog/actions/monitor/monitoring.go [21-23]
func NewCreateMonitorAction() action.Action[*rhtasv1alpha1.CTlog] {
	return &monitoringAction{}
}

Solution Walkthrough:

Before:

// internal/controller/ctlog/ctlog_controller.go
func (r *ctlogReconciler) defineActions() []action.Action[*rhtasv1alpha1.CTlog] {
  return []action.Action[*rhtasv1alpha1.CTlog]{
    // ...
    actions.NewServiceAction(),
    actions.NewCreateMonitorAction(), // Creates ServiceMonitor for ctlog
    // ...
    monitor.NewStatefulSetAction(),
    monitor.NewCreateServiceAction(), // Creates Service for ctlog-monitor
    monitor.NewCreateMonitorAction(), // Creates ServiceMonitor for ctlog-monitor
    // ...
  }
}

After:

// internal/controller/ctlog/ctlog_controller.go
func (r *ctlogReconciler) defineActions() []action.Action[*rhtasv1alpha1.CTlog] {
  return []action.Action[*rhtasv1alpha1.CTlog]{
    // ...
    actions.NewServiceAction(),
    actions.NewCreateMonitorAction(),
    // ...
    monitor.NewMonitorStatefulSetAction(),
    monitor.NewMonitorServiceAction(),
    monitor.NewMonitorServiceMonitorAction(),
    // ...
  }
}
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly identifies ambiguous naming for new actions, which harms code readability and maintainability, and the proposed refactoring would improve clarity in the controller logic.

Low
Possible issue
Remove redundant reconciliation action

Remove the redundant actions.NewCreateMonitorAction() call from the CTlog
reconciliation loop to avoid duplicate work and streamline the process.

internal/controller/ctlog/ctlog_controller.go [115-124]

 		actions.NewRBACAction(),
 		actions.NewDeployAction(),
 		actions.NewServiceAction(),
-		actions.NewCreateMonitorAction(),
 
 		monitor.NewStatefulSetAction(),
 		monitor.NewCreateServiceAction(),
 		monitor.NewCreateMonitorAction(),
 
 		transitions.NewToInitializePhaseAction[*rhtasv1alpha1.CTlog](),

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a redundant action in the reconciliation loop. Removing it improves code clarity and avoids unnecessary processing, though the functional impact is minor as the action would likely not execute due to its CanHandle condition.

Low
  • Update

@securesign securesign deleted a comment from qodo-merge-pro bot Nov 18, 2025
@osmman osmman added the enhancement New feature or request label Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants