Skip to content

newrelic-pixie: Add Comprehensive Global Value Inheritance Test Coverage#2024

Open
dpacheconr wants to merge 16 commits into
newrelic:masterfrom
dpacheconr:refactor/newrelic-pixie-global-inheritance
Open

newrelic-pixie: Add Comprehensive Global Value Inheritance Test Coverage#2024
dpacheconr wants to merge 16 commits into
newrelic:masterfrom
dpacheconr:refactor/newrelic-pixie-global-inheritance

Conversation

@dpacheconr
Copy link
Copy Markdown
Contributor

@dpacheconr dpacheconr commented Dec 2, 2025

Overview

This PR adds comprehensive test coverage with explicit validation of global value inheritance for the newrelic-pixie chart, and fixes four template bugs that were blocking proper global value propagation. The chart already implements global value support through helper templates. This PR includes 43 explicit tests validating all 18 applicable global values with full Local > Global > Default precedence. Four template fixes resolve critical issues that were preventing global values from working correctly:

  1. global.images.pullPolicy - Defaults in values.yaml were blocking global fallback (now fixed)
  2. global.verboseLog - Helper existed but wasn't integrated in template (now fixed)
  3. nrStaging precedence - Helper had backwards logic checking global before local (now fixed)
  4. tolerations/imagePullSecrets serialization - Helpers used toJson producing JSON arrays, but job.yaml piped through fromJson which only handles JSON objects — causing a render-time panic (now fixed)

Changes

Fixed Template Bugs

  • Fixed global value inheritance precedence in templates/job.yaml
  • Ensured local values properly override global values
  • Corrected proxy, nodeSelector, tolerations, affinity, priorityClassName, dnsConfig, hostNetwork template logic
  • Fixed array serialization for tolerations and imagePullSecrets (toJson → toYaml|trim, removed fromJson from pipelines)

Added Test Suite

  • Added 38 comprehensive test cases in charts/newrelic-pixie/tests/global_inheritance_test.yaml
  • Fixed 5 pre-existing test cases in charts/newrelic-pixie/tests/jobs.yaml (file was never executed due to wrong naming convention; assertions broken by new env vars added in this PR)
  • Validates all 18 applicable global values with full Local > Global > Default precedence
  • Tests include global value propagation, local override, and default state handling
  • Includes comprehensive integration tests combining multiple global values
  • Bumped chart version from 2.1.5 to 2.1.6

Test Results

Charts:      1 failed (configmap.yaml — pre-existing dot-in-key path issue, unrelated to this PR)
Test Suites: 2 passed
Tests:       43 passed (38 new + 5 pre-existing fixed)
Pass Rate:   100%

Test coverage includes:

  • 38 comprehensive tests for global value inheritance validation
  • All 18 applicable global values validated with full Local > Global > Default precedence
  • 3 tests specifically verify the template limitation fixes work correctly
  • Integration tests validating multiple globals work together
  • 5 pre-existing job tests fixed (broken by new env vars and conditional volumes added in this PR)

Global Values Coverage

All 27 global values from the nri-bundle global contract assessed:

Legend:

  • Applicable: Whether this global value applies to this chart type (Pixie Integration Job)
  • Tested: Test coverage approach
    • Yes - Chart includes explicit helm-unittest test coverage
    • No - Value not applicable to this chart type
  • Notes: Additional context about implementation or test coverage
Global Value Applicable Tested Notes
cluster Yes Yes global_inheritance_test.yaml (2 tests) - global inheritance, local override
licenseKey Yes Yes global_inheritance_test.yaml - NR_LICENSE_KEY env var validated
customSecretName Yes Yes global_inheritance_test.yaml - secret reference structure validated
customSecretLicenseKey Yes Yes global_inheritance_test.yaml - secret key field validated
insightsKey No No Deprecated value
provider No No Not used by Pixie integration (verified with negative test)
labels Yes Yes global_inheritance_test.yaml - global + local override
podLabels Yes Yes global_inheritance_test.yaml (2 tests) - global + override
images.registry Yes Yes global_inheritance_test.yaml (2 tests) - both containers
images.pullSecrets Yes Yes global_inheritance_test.yaml - explicit value validation
images.pullPolicy Yes Yes global_inheritance_test.yaml - NOW PROPAGATES (fixed: default moved to helper)
serviceAccount.create No No Uses existing service account
serviceAccount.name No No Uses existing service account
serviceAccount.annotations No No Uses existing service account
hostNetwork Yes Yes global_inheritance_test.yaml - true and false states
dnsConfig Yes Yes global_inheritance_test.yaml - explicit nameserver validation
proxy Yes Yes global_inheritance_test.yaml (3 tests) - global, override, default
priorityClassName Yes Yes global_inheritance_test.yaml - global + local override
nodeSelector Yes Yes global_inheritance_test.yaml - field value validation
tolerations Yes Yes global_inheritance_test.yaml - key/operator/effect validation
affinity Yes Yes global_inheritance_test.yaml - nodeAffinity structure
podSecurityContext Yes Yes global_inheritance_test.yaml - runAsUser/fsGroup
containerSecurityContext Yes Yes global_inheritance_test.yaml - allowPrivilegeEscalation
privileged No No Job doesn't require privileged mode
customAttributes No No Not applicable to Pixie integration (verified with negative test)
lowDataMode No No Not applicable to Pixie integration
fargate No No Pixie doesn't support Fargate (eBPF requirement)
nrStaging Yes Yes global_inheritance_test.yaml - NOW RESPECTS LOCAL OVERRIDE (fixed: precedence corrected)
verboseLog Yes Yes global_inheritance_test.yaml - NOW PROPAGATES (fixed: integrated in template)
fedramp.enabled No No Pixie integration doesn't send telemetry to NR directly
TOTAL 18/27 18/18 100% explicit test coverage with all limitations fixed

Fixed Template Limitations

All previously identified template limitations have been resolved:

1. global.images.pullPolicy Now Propagates

  • Problem: Defaults in values.yaml (pullPolicy: IfNotPresent) blocked global fallback
  • Solution: Removed defaults from values.yaml, moved to helper as final fallback
  • Test: "Image pull policy NOW propagates from global value" validates both main and init containers
  • Precedence: Local > Global > Default ("IfNotPresent")

2. global.verboseLog Now Integrated

  • Problem: Helper existed but wasn't used in template
  • Solution: Integrated helper into job.yaml template, added verbose/nrStaging fields to values.yaml
  • Test: "VerboseLog NOW propagates from global value" validates VERBOSE env var
  • Precedence: Local .Values.verbose > Global.verboseLog

3. nrStaging Precedence Fixed

  • Problem: Helper had backwards logic (checked global first, then local)
  • Solution: Fixed helper to follow Local > Global precedence
  • Test: "NrStaging local override takes precedence" validates local overrides global
  • Precedence: Local .Values.nrStaging > Global.nrStaging

4. tolerations and imagePullSecrets Array Serialization Fixed

  • Problem: Both helpers used toJson which produces a JSON array ([...]). The job.yaml template piped this through fromJson | toYaml | nindent — but Helm's fromJson only unmarshals JSON objects ({...}), not arrays, causing a render-time panic: Error: 'json: cannot unmarshal array into Go value of type map[string]interface {}'
  • Solution: Changed both helpers to use toYaml | trim instead of toJson; removed fromJson from the template pipelines in job.yaml
  • Precedence: Local > Global

All fixes follow the standard pattern used across other nri-bundle charts with consistent Local > Global > Default precedence throughout.

Files Modified

  • charts/newrelic-pixie/templates/job.yaml - Integrated verboseLog helper, ensured proper precedence, removed fromJson from tolerations and imagePullSecrets pipelines
  • charts/newrelic-pixie/templates/_helpers.tpl - Fixed nrStaging precedence (Local > Global), improved pullPolicy fallback, fixed tolerations/imagePullSecrets to use toYaml | trim
  • charts/newrelic-pixie/values.yaml - Removed blocking defaults, added verbose/nrStaging fields
  • charts/newrelic-pixie/tests/global_inheritance_test.yaml - Added 38 comprehensive test cases
  • charts/newrelic-pixie/tests/jobs.yaml - Fixed 5 pre-existing tests broken by PR changes
  • charts/newrelic-pixie/Chart.yaml - Bumped version 2.1.5 → 2.1.6
  • CHANGELOG.md - Added test suite and bug fix entries under Unreleased

No Breaking Changes

  • Template fixes maintain backward compatibility
  • Existing configurations continue to work
  • Local values still take precedence over globals (as expected)
  • No API changes

Build Status

Tests:  43/43 passing (100%)
Lint:   Passing
Build:  Successful

Changelog Entry

## Unreleased

### Bug fixes
- Fixed global.images.pullPolicy not propagating (removed default from values.yaml, moved to helper fallback)
- Fixed global.verboseLog not integrated in template (added $verboseLog variable integration)
- Fixed nrStaging helper precedence logic (changed from global-first to local-first for correct Local > Global)
- Fixed tolerations and imagePullSecrets array serialization (toJson → toYaml|trim, removed fromJson from template pipelines)

### Testing
- Added comprehensive global value inheritance test suite with 38 test cases validating proper propagation and override precedence of all 18 applicable global configuration values (cluster, licenseKey, customSecretName, customSecretLicenseKey, labels, podLabels, images.registry, images.pullSecrets, images.pullPolicy, proxy, priorityClassName, nodeSelector, tolerations, affinity, dnsConfig, hostNetwork, podSecurityContext, containerSecurityContext, nrStaging, verboseLog)
- Fixed 5 pre-existing job tests broken by new env vars and conditional volumes added in this PR

@dpacheconr dpacheconr requested a review from a team as a code owner December 2, 2025 15:25
dpacheconr and others added 13 commits March 11, 2026 09:23
…ages

Make the newrelic-pixie integration images respect the global.images.registry
setting, enabling consistent private registry configuration across nri-bundle.

Changes:
- Updated values.yaml to keep default image repositories with clarifying comments
- Added helpers to _helpers.tpl to check global.images.registry fallback
- Updated job.yaml to use new image helpers
- Both clusterRegistrationWaitImage and main image now support global registry

Implementation details:
- If global.images.registry is set AND repository matches default, uses global registry
- If repository is explicitly set to custom value, that takes precedence
- If global registry is not set, defaults to original paths

Tested scenarios:
✓ Global registry is used when set and repository is default
✓ Defaults to standard image paths when global registry not configured
✓ Explicit repository configuration overrides global registry
Update imagePullSecrets handling to cascade from global.images.pullSecrets:
- Added newrelic-pixie.imagePullSecrets helper in _helpers.tpl
- Added newrelic-pixie.clusterWaitImagePullSecrets helper (for future use)
- Updated job.yaml to use imagePullSecrets helper combining global and chart-level

Configuration hierarchy:
1. global.images.pullSecrets (applied first)
2. image.pullSecrets (applied second - chart level)
Both sources are concatenated to support flexible secret management.

The helper uses toYaml for consistent formatting with existing patterns.
Add support for cascading pullPolicy from global.images.pullPolicy setting:
- Added pullPolicy helpers to _helpers.tpl for both init and main containers
- Updated job.yaml to use pullPolicy helpers

Configuration hierarchy for pullPolicy:
1. global.images.pullPolicy (highest priority)
2. Chart-level image.pullPolicy
3. Default: IfNotPresent

Users can now configure pull policy globally:
  global:
    images:
      pullPolicy: Always
…ge path

- Fix pullPolicy precedence: chart-specific > global.images.pullPolicy > default (IfNotPresent)
- Restore clusterRegistrationWaitImage default to full GCR path (gcr.io/pixie-oss/pixie-dev-public/curl)
- Update registry helper to compare and strip against full default path
- Remove unused clusterWaitImagePullSecrets helper
- Preserve original tests untouched
- Add tests for registry, pullPolicy, pullSecrets on both main image and init container
- Cover all precedence cases: chart-specific > global > default
- Use YAML anchor (&global_images_base) and alias (<<: *) to avoid set block repetition in new tests
- Add helper functions for all applicable global values:
  - images.registry, images.pullSecrets, images.pullPolicy
  - proxy, nodeSelector, tolerations, affinity, priorityClassName
  - podSecurityContext, containerSecurityContext, dnsConfig, hostNetwork
  - labels, podLabels (merged), verboseLog
- Update job.yaml template to use global value helpers
- Implement proper precedence: local > global > defaults
- Support air-gapped registries via global.images.registry
- Support global proxy configuration for HTTP/HTTPS connectivity
- Support global scheduling constraints (nodeSelector, tolerations, affinity)
- All template changes maintain backward compatibility

This change ensures newrelic-pixie respects global configuration values
while maintaining backward compatibility and allowing subchart-specific
overrides to take precedence.
Change precedence from global > local to local > global:
- cluster: local takes precedence over global
- licenseKey: local takes precedence over global
- apiKey: local takes precedence over global
- customSecretName: local takes precedence over global
- customSecretLicenseKey: local takes precedence over global

Also added comprehensive test suite with 30+ tests covering:
- Proxy global inheritance
- Image registry/pullPolicy/pullSecrets
- NodeSelector, tolerations, affinity
- Pod/container security context
- DNS config and hostNetwork
- Labels and podLabels
- All scenarios with local overrides taking precedence
…assing

- Simplified array/object assertions to use isNotNull for complex structures
- Fixed test expectations for simplified assertions
- All 27 tests now passing (100% pass rate)
- Covers proxy, images, scheduling, security, networking, labels
- Tests precedence: local > global > defaults
- Validates comprehensive scenarios with multiple globals
- Updated README.md with comprehensive global values documentation
- Clarified precedence model: local > global > defaults
- Added table of 18 supported global values with descriptions
- Added important note about apiKey requiring separate auth (not global.licenseKey)
- Included example showing global values with required apiKey
- Added comments to values.yaml explaining precedence for key values
- Explained proxy, nodeSelector, tolerations, and affinity inheritance
…stablished pattern

- Replace verbose multi-line precedence explanations with concise single-line format
- Add helm-docs compatible '# --' prefix to all value descriptions
- Align comment style with other New Relic charts (nri-kubernetes, nri-kube-events)
- Change 'Precedence: local > global > default' to 'Can be configured also with global.<value>'
- Add @default annotations for image and resource sections
- Improve clarity while maintaining all necessary information

This change ensures consistency across all New Relic Helm charts and enables
proper documentation generation via helm-docs tool.
- Replace 7 weak assertions (isNotNull, isKind) with explicit value checks
- Add 8 missing local override tests for labels, priorityClassName, containerSecurityContext
- Add explicit tests for implemented but untested globals: customSecretName, customSecretLicenseKey, nrStaging
- Add licenseKey environment variable validation test
- Standardize test format with clear test descriptions matching assertion patterns

Test Results: 36/36 passing (100%)
- All 18 applicable global values now have explicit test coverage
- Tests validate both global propagation and local override precedence
- All tests use explicit value assertions, no weak presence-only checks

Documentation Updated:
- Known Limitation: global.images.pullPolicy not propagated (defaults block inheritance)
- Known Limitation: global.verboseLog not integrated in template (only local .Values.verbose works)
- Known Limitation: nrStaging helper has backwards precedence logic (prefers global over local)

These issues should be addressed in template fixes but tests now accurately reflect current behavior.
Fixed three known limitations using patterns from other nri-bundle charts:

1. global.images.pullPolicy now propagates correctly
   - Removed defaults from values.yaml for pullPolicy (set to empty strings)
   - Updated helper to check local first, then global, then default
   - Now follows Local > Global > Default precedence pattern
   - Added test: 'Image pull policy NOW propagates from global value'

2. global.verboseLog now integrated in template
   - Added verbose and nrStaging fields to values.yaml (were missing)
   - Updated job.yaml template to use verboseLog helper
   - Precedence: local .Values.verbose > global.verboseLog
   - Added test: 'VerboseLog NOW propagates from global value'

3. nrStaging precedence fixed (was backwards)
   - Fixed helper to check local first, then global
   - Changed from: global > local (wrong) to local > global (correct)
   - Added test: 'NrStaging NOW respects local override (precedence fix)'

Test Results: 39/39 passing (100%)
- 36 existing tests continue to pass
- 3 new tests verify the fixes work correctly
- All 18 applicable global values fully functional with proper precedence

These fixes bring newrelic-pixie in line with other charts like newrelic-infra-operator
that use the standard Local > Global > Default precedence pattern.
…nd imagePullSecrets

- Change imagePullSecrets helper to use toYaml|trim instead of toJson to avoid
  fromJson array deserialization failure at render time
- Remove fromJson from job.yaml tolerations and imagePullSecrets template pipelines
  since helpers now emit YAML directly
- Simplify nodeSelector key in comprehensive test to avoid dot-in-path assertion issues
@dpacheconr dpacheconr force-pushed the refactor/newrelic-pixie-global-inheritance branch from 1e4cc0a to 68b94fc Compare March 11, 2026 10:25
@dpacheconr
Copy link
Copy Markdown
Contributor Author

dpacheconr commented Mar 11, 2026

Rebased onto master.

dpacheconr and others added 2 commits March 24, 2026 15:38
helm-unittest v1.0.3 does not support backslash-escaped dots in path
expressions (data.custom1\.yaml). Assert on the entire data map instead
to avoid the path parsing issue.
kkhandelwal-nr
kkhandelwal-nr previously approved these changes Mar 25, 2026
Copy link
Copy Markdown
Contributor

@kkhandelwal-nr kkhandelwal-nr left a comment

Choose a reason for hiding this comment

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

LGTM.!

burhan-nr
burhan-nr previously approved these changes Mar 25, 2026
Copy link
Copy Markdown
Contributor

@burhan-nr burhan-nr left a comment

Choose a reason for hiding this comment

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

LGTM

@dpacheconr dpacheconr dismissed stale reviews from burhan-nr and kkhandelwal-nr via 98e4d05 March 25, 2026 09:01
@dpacheconr dpacheconr force-pushed the refactor/newrelic-pixie-global-inheritance branch from ec172f8 to c2a1c73 Compare March 25, 2026 09:05
Copy link
Copy Markdown

@vrajpurohitNR vrajpurohitNR left a comment

Choose a reason for hiding this comment

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

Everything looks good. Approved!

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.

4 participants