CCXDEV-15992: obfuscation config precedence#1223
CCXDEV-15992: obfuscation config precedence#1223openshift-merge-bot[bot] merged 3 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-15992 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. |
openshift-ci
Bot
added
the
approved
|
/retest |
|
/test e2e-gcp-ovn-techpreview |
|
/cc @ncaak |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncaak, 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 |
|
/verified later @BaiyangZhou |
openshift-ci-robot
added
verified-later
verified
|
@opokornyy: This PR has been marked to be verified later by 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 |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
Ensure ConfigMap based obfuscation configuration is applied before InsightsDataGather configuration. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
CCXDEV-15992-obfuscation-config
branch
from
054d206 to
717e954
Compare
openshift-ci-robot
removed
verified
📝 WalkthroughWalkthroughThe changes refactor configuration handling in the periodic controller by introducing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@opokornyy: This pull request references CCXDEV-15992 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/periodic/periodic_test.go (1)
793-861: Split the fallback coverage into nil and empty ConfigMap cases.Line 826 uses
config.Obfuscation{}to represent “no ConfigMap setting”, but in Go that is an explicit empty slice, not the same state asnil. Coveringniland empty separately here would make the intended precedence contract unambiguous.Please align this test with the confirmed nil-vs-empty behavior from the production path.
🤖 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 793 - 861, The test uses config.Obfuscation{} (an empty slice) but doesn't cover the nil ConfigMap case; update TestCreateDataGatherAttributeValues_ConfigMapObfuscationPrecedence to include two distinct cases for "no ConfigMap setting": one with configMapObfuscation set to nil (e.g. var nilObf config.Obfuscation = nil) and one with an explicit empty slice (config.Obfuscation{}), and assert that createDataGatherAttributeValues() (via mockController from NewMockConfigMapConfigurator) yields the expected DataPolicyOption fallback for the nil case and the explicit-empty behavior for the empty case so the precedence contract is unambiguous.
🤖 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 897-910: The current fallback check uses len(dataPolicy) == 0
which cannot distinguish "field omitted" vs "field explicitly set to []" because
YAML unmarshals slices to non-nil empty slices; change the ConfigMap obfuscation
field type from []ObfuscationValue to *[]ObfuscationValue (preserving nil for
omitted fields), update all code that reads/writes that field to handle a nil
pointer (e.g., check cm.Obfuscation == nil rather than len), and replace the
fallback condition in periodic.go (the dataPolicy selection logic that now looks
at dataPolicy variable and gatherConfig) to use the pointer-nil check so an
explicit empty ConfigMap clears obfuscation instead of inheriting
InsightsDataGather.Spec.DataPolicy.
---
Nitpick comments:
In `@pkg/controller/periodic/periodic_test.go`:
- Around line 793-861: The test uses config.Obfuscation{} (an empty slice) but
doesn't cover the nil ConfigMap case; update
TestCreateDataGatherAttributeValues_ConfigMapObfuscationPrecedence to include
two distinct cases for "no ConfigMap setting": one with configMapObfuscation set
to nil (e.g. var nilObf config.Obfuscation = nil) and one with an explicit empty
slice (config.Obfuscation{}), and assert that createDataGatherAttributeValues()
(via mockController from NewMockConfigMapConfigurator) yields the expected
DataPolicyOption fallback for the nil case and the explicit-empty behavior for
the empty case so the precedence contract is unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9649a60-aa35-41be-adef-192c762cc6a1
📒 Files selected for processing (2)
pkg/controller/periodic/periodic.gopkg/controller/periodic/periodic_test.go
| // ConfigMap should take precedence for the obfuscation configuration so use the | ||
| // InsightsDataGather configuration only if there was none set in a ConfigMap | ||
| // If there is not configuration in both then no obfuscation should be applied | ||
| if len(dataPolicy) == 0 && gatherConfig != nil && len(gatherConfig.DataPolicy) > 0 { | ||
| klog.Infof("Using data policy from InsightsDataGather CR because ConfigMap has no obfuscation settings") | ||
| for _, dataPolicyOption := range gatherConfig.DataPolicy { | ||
| switch dataPolicyOption { | ||
| case configv1.DataPolicyOptionObfuscateNetworking: | ||
| dataPolicy = append(dataPolicy, insightsv1.DataPolicyOptionObfuscateNetworking) | ||
| case configv1.DataPolicyOptionObfuscateWorkloadNames: | ||
| dataPolicy = append(dataPolicy, insightsv1.DataPolicyOptionObfuscateWorkloadNames) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "types.go" -type f | head -20Repository: openshift/insights-operator
Length of output: 1055
🏁 Script executed:
fd -t f "config" --path "*pkg/config*" | head -30Repository: openshift/insights-operator
Length of output: 304
🏁 Script executed:
rg -l "DataReporting" --type go | head -20Repository: openshift/insights-operator
Length of output: 939
🏁 Script executed:
cat -n pkg/config/types.go | head -100Repository: openshift/insights-operator
Length of output: 4241
🏁 Script executed:
rg -A 5 "type DataReporting struct" pkg/config/types.goRepository: openshift/insights-operator
Length of output: 283
🏁 Script executed:
rg -l "Observer\|observer\|loader\|Loader" pkg/config --type go | head -20Repository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg -A 10 "type Obfuscation" pkg/config/types.goRepository: openshift/insights-operator
Length of output: 136
🏁 Script executed:
rg -l "Observer\|InsightsConfigurationSerialized" . --type go | grep -v vendor | head -20Repository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg -A 5 "func.*Observer\|func.*Loader" pkg --type go | head -50Repository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg "insights-config" --type go | grep -v vendor | head -20Repository: openshift/insights-operator
Length of output: 1343
🏁 Script executed:
rg -l "configAggregator\|ConfigAggregator" pkg --type go | head -10Repository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg -B 5 -A 10 "json.Unmarshal.*InsightsConfigurationSerialized\|Unmarshal.*dataReporting" --type go | head -80Repository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
cat -n pkg/config/configobserver/configmapobserver.goRepository: openshift/insights-operator
Length of output: 5524
🏁 Script executed:
cat -n pkg/config/configobserver/config_aggregator.goRepository: openshift/insights-operator
Length of output: 11600
🏁 Script executed:
rg -B 5 -A 20 "func.*ToConfig" pkg/config/types.goRepository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg -B 2 -A 10 "DataReportingSerialized.*ToConfig\|func.*DataReporting.*ToConfig" pkg/config --type goRepository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
rg "ToConfig" pkg/config/types.goRepository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
cat pkg/config/types.go | tail -200Repository: openshift/insights-operator
Length of output: 4042
🏁 Script executed:
wc -l pkg/config/types.goRepository: openshift/insights-operator
Length of output: 94
🏁 Script executed:
rg -n "func" pkg/config/types.goRepository: openshift/insights-operator
Length of output: 53
🏁 Script executed:
find pkg/config -name "*.go" -type f | head -20Repository: openshift/insights-operator
Length of output: 677
🏁 Script executed:
rg "func.*ToConfig" pkg/config --type goRepository: openshift/insights-operator
Length of output: 229
🏁 Script executed:
cat -n pkg/config/config.goRepository: openshift/insights-operator
Length of output: 6596
🏁 Script executed:
cat -n pkg/controller/periodic/periodic.go | sed -n '886,920p'Repository: openshift/insights-operator
Length of output: 1935
🏁 Script executed:
cat -n pkg/controller/periodic/periodic.go | sed -n '860,900p'Repository: openshift/insights-operator
Length of output: 2080
Explicit empty ConfigMap obfuscation still falls back to the CR.
Line 900 uses len(dataPolicy) == 0 as the fallback signal. Because Obfuscation is defined as a slice type []ObfuscationValue, YAML unmarshaling conflates "field omitted" and "field explicitly set to []" into the same empty slice. An explicit empty ConfigMap therefore still inherits InsightsDataGather.Spec.DataPolicy instead of clearing it, violating ConfigMap precedence.
The suggested fix in the original comment (checking cmObfuscation == nil) will not work, as YAML unmarshaling never produces nil for slice fields. To fix this, either:
- Change
Obfuscationto a pointer type*[]ObfuscationValueto preservenilfor omitted fields, or - Implement explicit field-set tracking (e.g., a separate
boolflag in the serialized config), or - Use custom YAML unmarshaling to distinguish omitted vs. empty slices
🤖 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 897 - 910, The current
fallback check uses len(dataPolicy) == 0 which cannot distinguish "field
omitted" vs "field explicitly set to []" because YAML unmarshals slices to
non-nil empty slices; change the ConfigMap obfuscation field type from
[]ObfuscationValue to *[]ObfuscationValue (preserving nil for omitted fields),
update all code that reads/writes that field to handle a nil pointer (e.g.,
check cm.Obfuscation == nil rather than len), and replace the fallback condition
in periodic.go (the dataPolicy selection logic that now looks at dataPolicy
variable and gatherConfig) to use the pointer-nil check so an explicit empty
ConfigMap clears obfuscation instead of inheriting
InsightsDataGather.Spec.DataPolicy.
|
/lgtm |
|
/test insights-operator-e2e-tests |
|
/verified later @BaiyangZhou |
openshift-ci-robot
added
verified-later
verified
|
@opokornyy: This PR has been marked to be verified later by 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. |
|
@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. |
|
/test insights-operator-e2e-tests |
4 participants
Ensure ConfigMap based obfuscation configuration is applied before InsightsDataGather configuration.
Categories
Sample Archive
NoneDocumentation
NoneUnit Tests
pkg/controller/periodic/periodic_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
NoneBreaking Changes
No
References
CCXDEV-15992
Summary by CodeRabbit
Refactor
Tests