Skip to content

Fix panic in PimRoleEligibilitySchedule with NoExpiration type#4402

Merged
EronWright merged 2 commits into
masterfrom
fix/pim-noexpiration-panic-4351
Nov 9, 2025
Merged

Fix panic in PimRoleEligibilitySchedule with NoExpiration type#4402
EronWright merged 2 commits into
masterfrom
fix/pim-noexpiration-panic-4351

Conversation

@EronWright

@EronWright EronWright commented Nov 7, 2025

Copy link
Copy Markdown
Contributor

Summary

Fixes #4351 - a panic that occurred when creating a PimRoleEligibilitySchedule resource with NoExpiration type without providing a duration field.

Root Cause

The bug was in the inputsToSdk function at custom_pim_eligibility.go:460, where the code unconditionally accessed exp["duration"].StringValue() even when the duration field was nil/not provided.

According to Azure's PIM API schema, the expiration object supports three types:

  • AfterDuration: requires duration field (ISO 8601 duration like "P365D")
  • AfterDateTime: requires endDateTime field
  • NoExpiration: should NOT have either field

Changes

  1. Fixed the bug in custom_pim_eligibility.go:

    • Added proper checks for optional duration and endDateTime fields before accessing them
    • Follows the same pattern used elsewhere in the codebase for optional nested fields (see ticketInfo handling)
  2. Added comprehensive test coverage in custom_pim_eligibility_test.go:

    • Test for NoExpiration type (previously missing)
    • Test for AfterDateTime type (previously missing)
    • Existing test for AfterDuration type continues to pass

Testing

  • ✅ All unit tests pass (make test_provider PROVIDER_TEST_TAGS=unit)
  • ✅ Reproduced the panic before the fix
  • ✅ Verified the fix resolves the panic
  • ✅ Added regression tests for all three expiration types

Notes

This is a P1 panic bug that affects users trying to create permanent PIM role eligibility assignments (which use NoExpiration type).

🤖 Generated with Claude Code

Comment on lines +43 to +45
permissions:
id-token: write
contents: read

@EronWright EronWright Nov 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An extra fix riding in this PR, this fix was applied directly to the v3.10.1 branch, will be needed going forward.

@github-actions

github-actions Bot commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in PIM eligibility schedule expiration handling and adds necessary OIDC permissions to the release workflow.

  • Fixed optional field handling for duration and endDateTime in schedule expiration configuration
  • Added test coverage for NoExpiration and AfterDateTime expiration types
  • Added OIDC permissions to the docs build dispatch job

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
provider/pkg/resources/customresources/custom_pim_eligibility.go Fixed bug where duration field was accessed unconditionally; now properly handles optional duration and endDateTime fields based on expiration type
provider/pkg/resources/customresources/custom_pim_eligibility_test.go Added comprehensive test coverage for NoExpiration and AfterDateTime expiration types
.github/workflows/release.yml Added required id-token: write and contents: read permissions for OIDC authentication in the docs build job

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Nov 8, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.37%. Comparing base (349017b) to head (d087bfd).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...esources/customresources/custom_pim_eligibility.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4402   +/-   ##
=======================================
  Coverage   59.37%   59.37%           
=======================================
  Files          91       91           
  Lines       11444    11450    +6     
=======================================
+ Hits         6795     6799    +4     
- Misses       4014     4015    +1     
- Partials      635      636    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

EronWright and others added 2 commits November 8, 2025 15:13
Add id-token: write permission to dispatch_docs_build job to enable OIDC token minting for Pulumi ESC authentication.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit fixes a panic that occurred when creating a
PimRoleEligibilitySchedule resource with NoExpiration type
without providing a duration field.

The bug was in the inputsToSdk function at line 460, where the code
unconditionally accessed exp["duration"].StringValue() even when the
duration field was nil/not provided.

According to Azure's PIM API schema, the expiration object supports
three types:
- AfterDuration: requires duration field (ISO 8601 duration)
- AfterDateTime: requires endDateTime field
- NoExpiration: should NOT have either field

The fix adds proper checks for optional fields before accessing them,
following the same pattern used elsewhere in the codebase for optional
nested fields (see ticketInfo handling).

Also added comprehensive test coverage for all three expiration types
to prevent regression.

Fixes #4351

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@EronWright EronWright force-pushed the fix/pim-noexpiration-panic-4351 branch from c4d1964 to d087bfd Compare November 8, 2025 23:39
@EronWright EronWright enabled auto-merge (squash) November 8, 2025 23:39
@EronWright EronWright self-assigned this Nov 8, 2025
@EronWright EronWright merged commit 8096db5 into master Nov 9, 2025
24 checks passed
@EronWright EronWright deleted the fix/pim-noexpiration-panic-4351 branch November 9, 2025 00:28
@pulumi-bot

Copy link
Copy Markdown
Contributor

This PR has been shipped in release v3.11.0.

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.

custom_pim_eligibility.go expecting always duration

4 participants