Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic with Job resource diffing against an unreachable cluster #3024

Merged
merged 4 commits into from
May 28, 2024

Conversation

rquitales
Copy link
Contributor

@rquitales rquitales commented May 24, 2024

Proposed changes

This PR ensures that we do not make a k8s API request during the provider's diff if there is an unreachable cluster. This currently occurs when the Pulumi program contains a Job resource with the replaceUnready annotation set to true. A panic would occur if we attempt to make the API call since our clients are nil.

Testing done:

  1. Created a repro test case that fails with a panic (https://github.com/pulumi/pulumi-kubernetes/actions/runs/9228447658/job/25392833842?pr=3024)
  2. Added logic to prevent the panic, and test passes subsequently without intervention (https://github.com/pulumi/pulumi-kubernetes/actions/runs/9228685506/job/25393667599?pr=3024)
  3. Manual validation to ensure panic isn't trigerred.

Related issues (optional)

Fixes: #3022

Copy link

Does the PR have any schema changes?

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

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 36.48%. Comparing base (b6f6eff) to head (2616e49).

Files Patch % Lines
provider/pkg/provider/provider.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3024   +/-   ##
=======================================
  Coverage   36.47%   36.48%           
=======================================
  Files          70       70           
  Lines        9160     9163    +3     
=======================================
+ Hits         3341     3343    +2     
- Misses       5489     5491    +2     
+ Partials      330      329    -1     

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

@rquitales rquitales requested review from EronWright and a team May 24, 2024 18:58
Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Makes me wonder if we should also check if the cluster is unreachable inside readLiveObject, seems like an easy mistake to make otherwise.

@rquitales
Copy link
Contributor Author

rquitales commented May 28, 2024

Makes me wonder if we should also check if the cluster is unreachable inside readLiveObject, seems like an easy mistake to make otherwise.

Sounds good! Added an assert as a precheck for readLiveObject in https://github.com/pulumi/pulumi-kubernetes/pull/3024/files#diff-418e6e7d93aeacebe7c22d5fb674b48bfa15c6a514eb3ae4744b86d447595d8eR2684.

@rquitales rquitales enabled auto-merge (squash) May 28, 2024 16:10
@rquitales rquitales merged commit 491c036 into master May 28, 2024
18 checks passed
@rquitales rquitales deleted the rquitales/fix-job-panic branch May 28, 2024 16:54
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.

Job with unreachable cluster causes a panic
3 participants