OLS-3509: Redesign audit CRD and wire app-server OTEL to collector#1830
Conversation
📝 WalkthroughWalkthroughThe PR separates service audit settings from collector configuration, updates the audit CRD contract and generated OLS configuration, documents always-on collector behavior, and adds OTEL collector image configuration through operator and reconciler wiring. ChangesAudit and OTEL collector configuration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant OLSConfig
participant buildServiceAuditConfig
participant olsconfigYaml
participant OTELCollector
OLSConfig->>buildServiceAuditConfig: auditEventsEnabled
buildServiceAuditConfig->>olsconfigYaml: audit.logging and secure OTLP endpoint
olsconfigYaml->>OTELCollector: OTLP traces
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/crd-api.md:
- Around line 224-226: Continue renumbering all subsequent rules after the
quotaHandlersConfig entries to avoid duplicate rule numbers, including shifting
the existing period-pattern and storage rules and every later rule as needed.
Update rule 5’s regex reference to point unambiguously to the renumbered
limitersConfig rule, and preserve sequential numbering throughout the document.
- Around line 38-43: Update rule 54 in .ai/spec/what/crd-api.md to state that
spec.audit always serializes as at least an empty object {} rather than being
omitted; in api/v1alpha1/olsconfig_types.go, update the Audit field’s JSON tag
to remove the inert omitempty while keeping AuditConfig as a value type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef1c07a1-40cb-47d5-ab4c-84e037c7f88b
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.deepcopy.goconfig/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (14)
.ai/spec/what/audit-logging.md.ai/spec/what/crd-api.md.ai/spec/what/templog.mdapi/v1alpha1/olsconfig_types.goapi/v1alpha1/olsconfig_types_test.gocmd/main.gointernal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/olsconfig_helpers.gointernal/controller/reconciler/interface.gointernal/controller/utils/constants.gointernal/controller/utils/resource_defaults_test.gointernal/controller/utils/testing.gointernal/controller/utils/types.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/crd-api.md:
- Around line 40-43: Make behavioral-rule numbering globally unique in
.ai/spec/what/crd-api.md: retain audit rules 54–57 at the anchor range 40-43,
renumber the status rules at sibling range 332-336 to continue after rule 57,
and update every dependent reference to the renumbered status rules.
- Around line 313-314: Update the configuration-surface entry near the MCP rule
reference to point to validation rules 51–52 instead of 49–50, leaving the
surrounding validation descriptions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc192671-c35c-4475-b2cf-fe20ae1b5a3d
📒 Files selected for processing (2)
.ai/spec/what/crd-api.mdapi/v1alpha1/olsconfig_types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1alpha1/olsconfig_types.go
blublinsky
force-pushed
the
audit-crd-operator
branch
from
5035337 to
364c3b1
Compare
|
/lgtm |
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Review: OLS-3509 — pass (8/8 AC met)
Score: 100/100 | Mode: jira | Round 1
All acceptance criteria verified against the diff:
| # | Criterion | Status |
|---|---|---|
| 1 | spec.audit redesigned (logging *bool, tracingEndpoint); old types/helpers removed |
PASS |
| 2 | spec.ols.auditEventsEnabled added |
PASS |
| 3 | spec.ols.deployment.otelCollector added |
PASS |
| 4 | make manifests / make generate update CRD and deepcopy |
PASS |
| 5 | App server always emits collector OTLP endpoint; stdout audit from auditEventsEnabled |
PASS |
| 6 | --otel-collector-image flag wired through reconciler interface |
PASS |
| 7 | Unit tests pass | PASS |
| 8 | Design docs updated (templog.md, audit-logging.md, crd-api.md) |
PASS |
Code quality: No issues found. Clean separation between collector-only spec.audit and service-only spec.ols.auditEventsEnabled. buildServiceAuditConfig() correctly hardwires the in-cluster collector endpoint with TLS. Image flag plumbing complete through the full reconciler interface chain.
|
/override "ci/prow/bundle-e2e-4-21" /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@blublinsky: Overrode contexts on behalf of blublinsky: ci/prow/bundle-e2e-4-21 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
blublinsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3509 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/override "ci/prow/bundle-e2e-4-21" |
|
@blublinsky: Overrode contexts on behalf of blublinsky: ci/prow/bundle-e2e-4-21 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@blublinsky: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
Summary
Redesigns
OLSConfigaudit configuration for the OTEL Collector epic (OLS-3505) and wires lightspeed-service audit settings accordingly.spec.audit) — collector-only:logging(*bool, default true) and optionaltracingEndpoint. Removes the old service-orientedaudit.loggingenum andaudit.otelblock.spec.ols.auditEventsEnabled) — new*boolfield (default true) for stdout compliance audit JSON.spec.ols.deployment.otelCollector) — deployment overrides for the collector operand (consumed in OLS-3510).buildServiceAuditConfigmapsauditEventsEnabled→olsconfig.yamlaudit.logging; always sets OTLP export tolightspeed-otel-collector.<ns>.svc:4317with TLSSecure. Does not readspec.audit.--otel-collector-imageflag andGetOtelCollectorImage()plumbing for the collector reconciler (OLS-3510)..ai/spec/what/templog.md,audit-logging.md, andcrd-api.md.Out of scope (follow-up PRs)
make bundle, CSV descriptors,related_images.jsonotel entry, deployment--otel-collector-imagetemplogsbootstrapType of change
Related Tickets & Documents
https://redhat.atlassian.net/browse/OLS-3509
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
ols.auditEventsEnabledto control structured audit events on service stdout independently of collector behavior.audit.tracingEndpoint, routed through the collector; in-cluster trace export is always configured.Breaking Changes
audit.logging/audit.otelconfiguration blocks and thespec.templogoption.auditconfig model (stdout audit vs collector behavior) and corresponding defaults/mapping behavior.