CCXDEV-16214: obfuscation preference#1259
Conversation
opokornyy
changed the title
📝 WalkthroughWalkthroughUpdates docs with obfuscation precedence; implements union-based merging of ConfigMap, InsightsDataGather, and DataGather DataPolicy in the periodic controller (helper extracted); and updates tests to inject mocks and assert merged-policy behavior. ChangesPeriodic controller + docs + tests
sequenceDiagram
participant PeriodicController
participant ConfigMap
participant InsightsDataGather
participant DataGather
PeriodicController->>ConfigMap: read dataReporting.obfuscation (getConfigMapObfuscation)
PeriodicController->>InsightsDataGather: read gatherConfig.dataPolicy
PeriodicController->>DataGather: read spec.dataPolicy
PeriodicController->>PeriodicController: merge (ConfigMap base + DataGather or InsightsDataGather) de-duplicate
PeriodicController->>DataGather: write merged spec.dataPolicy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-16214 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opokornyy 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
|
@opokornyy: This pull request references CCXDEV-16214 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
/cc @ncaak |
![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
🧹 Nitpick comments (1)
pkg/controller/periodic/periodic_test.go (1)
1974-1991: Add thenilvs[]on-demand precedence cases here.This setup now reaches
prepareDataGatherCRWithImage(), but it still doesn't lock down the documented edge case: omittedspec.dataPolicyshould inheritInsightsDataGather, whiledataPolicy: []should suppress it. A regression case here would catch the current len-based bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/periodic/periodic_test.go` around lines 1974 - 1991, Add two test cases in periodic_test.go that exercise prepareDataGatherCRWithImage(): one where spec.dataPolicy is omitted and one where spec.dataPolicy is present as an empty slice ([]). Using the existing test setup (NewWithTechPreview, NewInsightsDataGatherObserverMock and mockConfigMapConfigurator), assert that the omitted spec.dataPolicy case inherits the InsightsDataGather default while the explicit dataPolicy: [] case suppresses it; this will catch the current len-based bug by verifying behavior differences between the absent field and an empty slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/periodic/periodic.go`:
- Around line 468-474: The current code calls c.updateDataGather(...) after
merging defaultDataPolicy and returns immediately on error which can silently
drop the on‑demand gather; change the error path in the block after
c.updateDataGather to either retry the write (e.g., return the error so the
controller will requeue with backoff) or explicitly mark the DataGather CR as
failed by setting a failed condition/status and persisting that change (use the
same DataGather object returned from prepareDataGatherCRWithImage and call the
appropriate status/update helper instead of returning nil). Specifically, modify
the error handling around slices.Equal/defaultDataPolicy -> c.updateDataGather
so that on err you persist a failure condition on dataGather (or requeue by
returning the error) rather than simply returning nil.
- Around line 430-447: The merge logic treats an omitted
DataGather.Spec.DataPolicy the same as an explicit empty list; change the
conditional in periodic.go to check for nil (dataGather.Spec.DataPolicy != nil)
instead of len(...) > 0 so an explicit empty slice suppresses gatherConfig
contribution while a nil/omitted field allows fallthrough to
gatherConfig.DataPolicy; keep the existing append/dedup logic that uses
slices.Contains on dataPolicy and the conversion to insightsv1.DataPolicyOption
when iterating gatherConfig.DataPolicy.
---
Nitpick comments:
In `@pkg/controller/periodic/periodic_test.go`:
- Around line 1974-1991: Add two test cases in periodic_test.go that exercise
prepareDataGatherCRWithImage(): one where spec.dataPolicy is omitted and one
where spec.dataPolicy is present as an empty slice ([]). Using the existing test
setup (NewWithTechPreview, NewInsightsDataGatherObserverMock and
mockConfigMapConfigurator), assert that the omitted spec.dataPolicy case
inherits the InsightsDataGather default while the explicit dataPolicy: [] case
suppresses it; this will catch the current len-based bug by verifying behavior
differences between the absent field and an empty slice.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 866bf64b-6ce8-485f-a7b6-4264f1d89fb1
📒 Files selected for processing (3)
docs/arch.mdpkg/controller/periodic/periodic.gopkg/controller/periodic/periodic_test.go
| if !slices.Equal(defaultDataPolicy, dataGather.Spec.DataPolicy) { | ||
| klog.Infof("Updating DataGather %s with merged DataPolicy from ConfigMap and InsightsDataGather", dgName) | ||
| dataGather, err = c.updateDataGather(ctx, dataGather) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't let the spec rewrite silently drop the on-demand gather.
If the new Update() at Line 470 fails, prepareDataGatherCRWithImage() returns an error and onDemandGather() just stops processing that resource. Because this flow is driven by create events, the DataGather can be left forever with no job and no failed condition. Please retry this write or mark the CR failed instead of returning immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/periodic/periodic.go` around lines 468 - 474, The current code
calls c.updateDataGather(...) after merging defaultDataPolicy and returns
immediately on error which can silently drop the on‑demand gather; change the
error path in the block after c.updateDataGather to either retry the write
(e.g., return the error so the controller will requeue with backoff) or
explicitly mark the DataGather CR as failed by setting a failed condition/status
and persisting that change (use the same DataGather object returned from
prepareDataGatherCRWithImage and call the appropriate status/update helper
instead of returning nil). Specifically, modify the error handling around
slices.Equal/defaultDataPolicy -> c.updateDataGather so that on err you persist
a failure condition on dataGather (or requeue by returning the error) rather
than simply returning nil.
d563d93 to
abf1f47
Compare
|
@opokornyy: This pull request references CCXDEV-16214 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
/retest |
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
CCXDEV-16214-obfuscation-preference
branch
from
abf1f47 to
222a07c
Compare
|
@opokornyy: This pull request references CCXDEV-16214 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
Implement obfuscation precedence for on-demand and periodic jobs to match the rules defined in docs/arch.md. This ensures consistent precedence across: - ConfigMap - DataGather - InsightsDataGather Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
CCXDEV-16214-obfuscation-preference
branch
from
222a07c to
bfc76cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controller/periodic/periodic.go (1)
435-443:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit empty
DataGather.Spec.DataPolicysemantics.Line 435 still uses
len(dataGather.Spec.DataPolicy) > 0, which treats omitted and explicit empty policy the same. That causes explicitdataPolicy: []to incorrectly fall through toInsightsDataGatherdefaults.Suggested fix
- if len(dataGather.Spec.DataPolicy) > 0 { + if dataGather.Spec.DataPolicy != nil { for _, dataPolicyOption := range dataGather.Spec.DataPolicy { if !slices.Contains(dataPolicy, dataPolicyOption) { dataPolicy = append(dataPolicy, dataPolicyOption) } } - // If DataGather has no policy, check InsightsDataGather for cluster-wide defaults } else if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { for _, dataPolicyOption := range gatherConfig.DataPolicy { if !slices.Contains(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) { dataPolicy = append(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) } } }🤖 Prompt for 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. In `@pkg/controller/periodic/periodic.go` around lines 435 - 443, The code currently treats an explicit empty DataGather.Spec.DataPolicy the same as an omitted one by using len(dataGather.Spec.DataPolicy) > 0; change that branch to detect presence vs omission by checking for nil (e.g. if dataGather.Spec.DataPolicy != nil) so an explicit empty slice preserves semantics (do not fall through to gatherConfig.DataPolicy). Update the conditional that iterates and appends into dataPolicy (and keep the else if for gatherConfig.DataPolicy) so explicit [] in dataGather.Spec.DataPolicy prevents using gatherConfig.DataPolicy defaults; reference the symbols dataGather.Spec.DataPolicy, dataPolicy, and gatherConfig.DataPolicy when making the change.
🤖 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 `@pkg/controller/periodic/periodic_test.go`:
- Around line 1972-1973: Replace the deprecated fake constructors used in the
test: change the calls to insightsFakeCli.NewSimpleClientset and
kubefake.NewSimpleClientset to the supported fake client builders provided by
the respective fake packages (e.g., use the fake.NewSimpleClientset entrypoint
from each package's "fake" subpackage), update the imports to reference those
fake builders, and run the tests/linter to confirm SA1019 is resolved; target
the instantiation sites in periodic_test.go where insightsClientset and
kubeClientset are created.
---
Duplicate comments:
In `@pkg/controller/periodic/periodic.go`:
- Around line 435-443: The code currently treats an explicit empty
DataGather.Spec.DataPolicy the same as an omitted one by using
len(dataGather.Spec.DataPolicy) > 0; change that branch to detect presence vs
omission by checking for nil (e.g. if dataGather.Spec.DataPolicy != nil) so an
explicit empty slice preserves semantics (do not fall through to
gatherConfig.DataPolicy). Update the conditional that iterates and appends into
dataPolicy (and keep the else if for gatherConfig.DataPolicy) so explicit [] in
dataGather.Spec.DataPolicy prevents using gatherConfig.DataPolicy defaults;
reference the symbols dataGather.Spec.DataPolicy, dataPolicy, and
gatherConfig.DataPolicy when making the change.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ec066f67-321e-499f-aa48-fa6ec8261241
📒 Files selected for processing (3)
docs/arch.mdpkg/controller/periodic/periodic.gopkg/controller/periodic/periodic_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/arch.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controller/periodic/periodic.go (1)
435-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the nil vs. empty slice check to honor the documented empty-array contract.
Line 435 uses
len(dataGather.Spec.DataPolicy) > 0, which cannot distinguish between an omitteddataPolicyfield (nil) and an explicitly set empty array (dataPolicy: []). According to the documented precedence rules, an explicit emptyDataGather.Spec.DataPolicymust suppress theInsightsDataGathercontribution, while a nil/omitted field should allow fallthrough togatherConfig.DataPolicy.This issue was flagged in a previous review but remains unfixed.
🔧 Proposed fix
- if len(dataGather.Spec.DataPolicy) > 0 { + if dataGather.Spec.DataPolicy != nil { for _, dataPolicyOption := range dataGather.Spec.DataPolicy { if !slices.Contains(dataPolicy, dataPolicyOption) { dataPolicy = append(dataPolicy, dataPolicyOption) } } - // If DataGather has no policy, check InsightsDataGather for cluster-wide defaults } else if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 {🤖 Prompt for 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. In `@pkg/controller/periodic/periodic.go` around lines 435 - 448, The code must distinguish an explicit empty DataPolicy slice from a nil/omitted one: replace the len(dataGather.Spec.DataPolicy) > 0 check with a nil check (dataGather.Spec.DataPolicy != nil) so an explicitly-set empty slice suppresses fallback to gatherConfig.DataPolicy, but a nil/omitted field still falls through; keep the existing loop that appends unique entries to dataPolicy (in the block referencing dataGather.Spec.DataPolicy) and leave the gatherConfig fallback block unchanged (the block that iterates gatherConfig.DataPolicy and converts entries to insightsv1.DataPolicyOption).
🧹 Nitpick comments (1)
pkg/controller/periodic/periodic.go (1)
435-448: ⚡ Quick winConsider extracting the de-duplication merge logic into a shared helper.
The pattern of iterating a slice, checking
slices.Contains, and appending unique items appears in bothprepareDataGatherCRWithImage(lines 435-440, 443-447) andcreateDataGatherAttributeValues(lines 941-945). Extracting this into a generic helper (e.g.,mergeUniqueDataPolicies) would reduce duplication and centralize the merge precedence logic for easier maintenance and testing.♻️ Proposed refactor
Add a new helper function:
// mergeUniqueDataPolicies appends items from source to base, skipping duplicates. func mergeUniqueDataPolicies(base []insightsv1.DataPolicyOption, source []insightsv1.DataPolicyOption) []insightsv1.DataPolicyOption { result := append([]insightsv1.DataPolicyOption(nil), base...) for _, option := range source { if !slices.Contains(result, option) { result = append(result, option) } } return result }Then refactor the merge sites:
In
prepareDataGatherCRWithImage:dataPolicy := c.getConfigMapObfuscation() - // Merge obfuscation settings from multiple sources with correct precedence: - // 1. Start with ConfigMap obfuscation settings (base configuration) - // 2. Merge with DataGather.Spec.DataPolicy if present (resource-specific override) - // 3. Fall back to InsightsDataGather configuration if DataGather has no policy set - // All unique values from each source are combined to ensure comprehensive obfuscation. - if len(dataGather.Spec.DataPolicy) > 0 { - for _, dataPolicyOption := range dataGather.Spec.DataPolicy { - if !slices.Contains(dataPolicy, dataPolicyOption) { - dataPolicy = append(dataPolicy, dataPolicyOption) - } - } - // If DataGather has no policy, check InsightsDataGather for cluster-wide defaults - } else if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { - for _, dataPolicyOption := range gatherConfig.DataPolicy { - if !slices.Contains(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) { - dataPolicy = append(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) - } - } - } + // Merge obfuscation: ConfigMap base + (DataGather.Spec OR InsightsDataGather fallback) + if dataGather.Spec.DataPolicy != nil { + dataPolicy = mergeUniqueDataPolicies(dataPolicy, dataGather.Spec.DataPolicy) + } else if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { + // Convert gatherConfig.DataPolicy to []insightsv1.DataPolicyOption + insightsDataPolicy := make([]insightsv1.DataPolicyOption, 0, len(gatherConfig.DataPolicy)) + for _, opt := range gatherConfig.DataPolicy { + insightsDataPolicy = append(insightsDataPolicy, insightsv1.DataPolicyOption(opt)) + } + dataPolicy = mergeUniqueDataPolicies(dataPolicy, insightsDataPolicy) + }In
createDataGatherAttributeValues:dataPolicy := c.getConfigMapObfuscation() - // Merge obfuscation settings from multiple sources with correct precedence: - // 1. Start with ConfigMap obfuscation settings (base configuration) - // 2. Merge with InsightsDataGather if present (resource-specific override) - // All unique values from each source are combined to ensure comprehensive obfuscation. - if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { - for _, dataPolicyOption := range gatherConfig.DataPolicy { - if !slices.Contains(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) { - dataPolicy = append(dataPolicy, insightsv1.DataPolicyOption(dataPolicyOption)) - } - } - } + // Merge ConfigMap base with InsightsDataGather cluster-wide defaults + if gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { + insightsDataPolicy := make([]insightsv1.DataPolicyOption, 0, len(gatherConfig.DataPolicy)) + for _, opt := range gatherConfig.DataPolicy { + insightsDataPolicy = append(insightsDataPolicy, insightsv1.DataPolicyOption(opt)) + } + dataPolicy = mergeUniqueDataPolicies(dataPolicy, insightsDataPolicy) + }Also applies to: 940-946
🤖 Prompt for 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. In `@pkg/controller/periodic/periodic.go` around lines 435 - 448, Duplicate de-duplication logic around merging DataPolicy slices appears in prepareDataGatherCRWithImage and createDataGatherAttributeValues; extract it into a single helper (e.g., mergeUniqueDataPolicies) that accepts and returns []insightsv1.DataPolicyOption, uses slices.Contains to skip duplicates, and preserves precedence by appending non-duplicates from the source onto a copy of the base; then replace the inline loops in prepareDataGatherCRWithImage and createDataGatherAttributeValues with calls to this helper to centralize behavior and simplify testing.
🤖 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.
Duplicate comments:
In `@pkg/controller/periodic/periodic.go`:
- Around line 435-448: The code must distinguish an explicit empty DataPolicy
slice from a nil/omitted one: replace the len(dataGather.Spec.DataPolicy) > 0
check with a nil check (dataGather.Spec.DataPolicy != nil) so an explicitly-set
empty slice suppresses fallback to gatherConfig.DataPolicy, but a nil/omitted
field still falls through; keep the existing loop that appends unique entries to
dataPolicy (in the block referencing dataGather.Spec.DataPolicy) and leave the
gatherConfig fallback block unchanged (the block that iterates
gatherConfig.DataPolicy and converts entries to insightsv1.DataPolicyOption).
---
Nitpick comments:
In `@pkg/controller/periodic/periodic.go`:
- Around line 435-448: Duplicate de-duplication logic around merging DataPolicy
slices appears in prepareDataGatherCRWithImage and
createDataGatherAttributeValues; extract it into a single helper (e.g.,
mergeUniqueDataPolicies) that accepts and returns []insightsv1.DataPolicyOption,
uses slices.Contains to skip duplicates, and preserves precedence by appending
non-duplicates from the source onto a copy of the base; then replace the inline
loops in prepareDataGatherCRWithImage and createDataGatherAttributeValues with
calls to this helper to centralize behavior and simplify testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eb48f827-da05-4829-b5ae-e72ad238d22c
📒 Files selected for processing (2)
pkg/controller/periodic/periodic.gopkg/controller/periodic/periodic_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/periodic/periodic_test.go
|
/retest |
|
@opokornyy: The following tests failed, say
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
This PR implements obfuscation precedence for on-demand and periodic jobs to match the rules defined in
docs/arch.md.Categories
Sample Archive
NoneDocumentation
docs/arch.mdUnit Tests
pkg/controller/periodic/periodic_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
https://redhat.atlassian.net/browse/CCXDEV-16214
Summary by CodeRabbit
Documentation
Refactor
Tests