Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Oct 3, 2025

Implementation Complete: skipInvalidResources for syslog-ng

All tasks completed successfully

Summary

Successfully implemented skipInvalidResources support for syslog-ng logging to match the existing behavior in fluentd. When this flag is enabled in the Logging resource spec, invalid SyslogNGFlows and SyslogNGClusterFlows (e.g., those referencing non-existent outputs) are skipped during configuration rendering instead of causing the entire reconciliation to fail.

Changes Made:

  1. ✅ Added SkipInvalidResources field to config.Input struct in pkg/sdk/logging/model/syslogng/config/config.go
  2. ✅ Modified configRenderer function to:
    • Skip invalid ClusterFlows when SkipInvalidResources is true
    • Skip invalid Flows when SkipInvalidResources is true
    • Properly accumulate validation errors for both GlobalOutputRefs and LocalOutputRefs in Flows
    • Added error logging when skipping invalid resources
  3. ✅ Updated controller in controllers/logging/logging_controller.go to pass the flag and logger from Logging spec
  4. ✅ Added comprehensive test cases in config_test.go:
    • Test for skipping invalid flows with non-existent cluster output
    • Test for skipping invalid cluster flows with non-existent cluster output
    • Test for mixed valid and invalid flows (ensures valid flows are still processed)

Recent Updates:

  • Added Logger field to Input struct to enable error logging
  • Imported github.com/go-logr/logr package
  • Updated code to log error messages when skipping invalid resources:
    • For SyslogNGClusterFlow: logs "skipping invalid SyslogNGClusterFlow" with name and namespace
    • For SyslogNGFlow: logs "skipping invalid SyslogNGFlow" with name and namespace
  • Added nil check for logger to prevent panics in tests

Testing Results:

  • ✅ All existing tests pass (23/23 test cases)
  • ✅ All new tests pass (3 additional test cases)
  • ✅ Project builds successfully without errors
  • ✅ Code follows existing patterns and conventions

Behavior:

The implementation works exactly like the fluentd version:

  • When skipInvalidResources: true in the Logging spec, invalid flows/clusterflows are skipped with error logs
  • When skipInvalidResources: false (default), errors are accumulated and returned, causing reconciliation to fail
  • Valid flows are always processed regardless of the flag setting
  • Error logs provide clear information about which resources were skipped and why

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] skipInvalidResources is not implemented for syslogng logging Implement skipInvalidResources for syslog-ng logging Oct 3, 2025
@Copilot Copilot AI requested a review from csatib02 October 3, 2025 09:59
Copilot finished work on behalf of csatib02 October 3, 2025 09:59
@csatib02 csatib02 force-pushed the copilot/fix-7d3b6ea5-1612-4c73-b160-15fd29dec4c1 branch from fe29527 to 1d505a4 Compare October 3, 2025 16:21
@csatib02 csatib02 marked this pull request as ready for review October 3, 2025 16:21
@csatib02 csatib02 requested review from pepov and OverOrion October 3, 2025 16:21
for _, cf := range in.ClusterFlows {
if err := validateClusterOutputs(clusterOutputRefs, client.ObjectKeyFromObject(&cf).String(), cf.Spec.GlobalOutputRefs, cf.Kind); err != nil {
if in.SkipInvalidResources {
// Skip this cluster flow and continue with the next one
Copy link
Member

Choose a reason for hiding this comment

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

@copilot add an error log here that we skipped this resource

Copy link
Author

Choose a reason for hiding this comment

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

Added error logging in commit 7a1f2c3. The code now logs "skipping invalid SyslogNGClusterFlow" with the resource name and namespace when an invalid cluster flow is skipped.

}
if flowErrs != nil {
if in.SkipInvalidResources {
// Skip this flow and continue with the next one
Copy link
Member

Choose a reason for hiding this comment

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

@copilot add an error log here about the skipped resource

Copy link
Author

Choose a reason for hiding this comment

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

Added error logging in commit 7a1f2c3. The code now logs "skipping invalid SyslogNGFlow" with the resource name and namespace when an invalid flow is skipped.

for _, cf := range in.ClusterFlows {
if err := validateClusterOutputs(clusterOutputRefs, client.ObjectKeyFromObject(&cf).String(), cf.Spec.GlobalOutputRefs, cf.Kind); err != nil {
if in.SkipInvalidResources {
// Skip this cluster flow and continue with the next one
Copy link
Member

Choose a reason for hiding this comment

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

@copilot add an error log here about the skipped resource

Copy link
Author

Choose a reason for hiding this comment

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

Added error logging in commit 7a1f2c3. The code now logs "skipping invalid SyslogNGClusterFlow" with the resource name and namespace when an invalid cluster flow is skipped.

}
if flowErrs != nil {
if in.SkipInvalidResources {
// Skip this flow and continue with the next one
Copy link
Member

Choose a reason for hiding this comment

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

@copilot add an error log here about the skipped resource

Copy link
Author

Choose a reason for hiding this comment

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

Added error logging in commit 7a1f2c3. The code now logs "skipping invalid SyslogNGFlow" with the resource name and namespace when an invalid flow is skipped.

@Copilot Copilot AI requested a review from csatib02 October 4, 2025 08:45
Copilot finished work on behalf of csatib02 October 4, 2025 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants