Skip to content

Conversation

afdesk
Copy link
Contributor

@afdesk afdesk commented Aug 6, 2025

Description

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@@ -54,7 +54,7 @@ type Config struct {
WebhookBroadcastTimeout *time.Duration `env:"OPERATOR_WEBHOOK_BROADCAST_TIMEOUT" envDefault:"30s"`
WebhookBroadcastCustomHeaders string `env:"OPERATOR_WEBHOOK_BROADCAST_CUSTOM_HEADERS"`
WebhookSendDeletedReports bool `env:"OPERATOR_SEND_DELETED_REPORTS" envDefault:"false"`
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job"`
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job,PersistentVolume"`
Copy link
Member

Choose a reason for hiding this comment

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

If this is configurable today, why can't the user scan them now? I must be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was impossible because specific Kind values were hard-coded in multiple places.
When the actual type didn’t match one of those hard-coded cases, the code returned an error.
for example:
https://github.com/aquasecurity/trivy-operator/blob/main/pkg/kube/object.go#L204-L237
https://github.com/aquasecurity/trivy-operator/blob/main/pkg/kube/object.go#L336-L385
and other

@simar7
Copy link
Member

simar7 commented Aug 7, 2025

Just thinking out loud how to test this change: As part of our integration tests, we can specify the new PV resource and have it get scanned and create reports based on that. WDYT?

@simar7
Copy link
Member

simar7 commented Aug 7, 2025

Another point about testing, I added perf support on a feature branch (to enable profiling) but I think I'll enhance it by adding support for it to be enabled with a configuration option. That we the performance team can test this branch to see if there's a big performance overhead or not.

I updated that PR here #2666

@afdesk
Copy link
Contributor Author

afdesk commented Aug 25, 2025

Just thinking out loud how to test this change: As part of our integration tests, we can specify the new PV resource and have it get scanned and create reports based on that. WDYT?

tried to add it here 37dd96d

@afdesk afdesk marked this pull request as ready for review August 25, 2025 18:32
@afdesk afdesk requested a review from Copilot August 25, 2025 18:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for any kinds of resources in policy checks by implementing a new scope resolution system. It removes hardcoded resource type validation and introduces dynamic resource discovery based on the Kubernetes scheme.

  • Introduces a new K8sScope resolver for dynamic scope detection of Kubernetes resources
  • Replaces hardcoded resource lists with dynamic resource discovery in controllers
  • Adds support for PersistentVolume and PersistentVolumeClaim resources in configuration audits

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/kube/scope.go New scope resolver implementation for dynamic resource scope detection
pkg/kube/scope_test.go Comprehensive tests for the new scope resolver functionality
pkg/kube/object.go Major refactoring to support dynamic resource discovery and remove hardcoded validations
pkg/kube/object_test.go Updated tests to use new scope resolver instead of deprecated functions
pkg/configauditreport/controller/resource.go Updated to use dynamic resource discovery instead of hardcoded resource lists
pkg/configauditreport/controller/policyconfig.go Simplified resource setup using new dynamic discovery
pkg/utils/util.go Removed kind validation to allow any resource types
pkg/operator/etc/config.go Added PersistentVolume to default target workloads
tests/itest/trivy-operator/behavior/behavior.go Added integration tests for PV and PVC resources
tests/itest/trivy-operator/suite_test.go Updated test configuration to include PVC in target workloads

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -202,11 +248,11 @@ func ObjectRefFromKindAndObjectKey(kind Kind, name client.ObjectKey) ObjectRef {
// adding it as the trivy-operator.LabelResourceSpecHash label to an instance of a
// security report.
func ComputeSpecHash(obj client.Object) (string, error) {
switch t := obj.(type) {
switch obj.(type) {
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The variable t is declared but no longer used after the switch statement change. Consider removing the unused variable declaration.

Copilot uses AI. Check for mistakes.

@@ -54,7 +54,7 @@ type Config struct {
WebhookBroadcastTimeout *time.Duration `env:"OPERATOR_WEBHOOK_BROADCAST_TIMEOUT" envDefault:"30s"`
WebhookBroadcastCustomHeaders string `env:"OPERATOR_WEBHOOK_BROADCAST_CUSTOM_HEADERS"`
WebhookSendDeletedReports bool `env:"OPERATOR_SEND_DELETED_REPORTS" envDefault:"false"`
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job"`
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job,PersistentVolume"`
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

Adding PersistentVolume to the default target workloads should also include PersistentVolumeClaim for consistency, as both are tested in the integration tests.

Suggested change
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job,PersistentVolume"`
TargetWorkloads string `env:"OPERATOR_TARGET_WORKLOADS" envDefault:"Pod,ReplicaSet,ReplicationController,StatefulSet,DaemonSet,CronJob,Job,PersistentVolume,PersistentVolumeClaim"`

Copilot uses AI. Check for mistakes.

@@ -456,6 +457,79 @@ func ConfigurationCheckerBehavior(inputs *Inputs) func() {
})

})

Copy link
Member

Choose a reason for hiding this comment

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

Can we also add another case where we don't specify the scanning of a PVC but create one? In such a case the assertion should ensure that no report is generated.

Another case should be that we create a new resource (something other than what we support by default) it shouldn't generate any reports either.

@afdesk afdesk added this to the v0.29.0 milestone Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants