Skip to content

Fix: nil pointer dereference in alloydb direct controller #5009

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 5, 2025
Merged

Fix: nil pointer dereference in alloydb direct controller #5009

merged 1 commit into from
Sep 5, 2025

Conversation

gemmahou
Copy link
Collaborator

@gemmahou gemmahou commented Aug 19, 2025 •
edited
Loading

Change description

  • Fix the nil pointer dereference error in AlloyDB direct controller and clear repeated code.
  • Set status.externRef after the creation of AlloyDBCluster.

Special notes for your reviewer:

Does this PR add something which needs to be 'release noted'?

## Bug Fixes:

*   [PR#5009](https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/5009)
     Fix the nil pointer dereference error(error message: "error":"INTERNAL ERROR: panic: observed a panic: runtime error: invalid memory address or nil pointer dereference) in AlloyDB direct controller
  • Reviewer reviewed release note.

Additional documentation e.g., references, usage docs, etc.:

Intended Milestone

1.134
Please indicate the intended milestone.

  • Reviewer tagged PR with the actual milestone.

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@xiaoweim
Copy link
Collaborator

xiaoweim commented Aug 20, 2025 •
edited
Loading

Thank you Gemma!

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Aug 20, 2025
@gemmahou
Copy link
Collaborator Author

cc: @cheftako @justinsb

yuwenma
yuwenma reviewed Aug 29, 2025
View reviewed changes
Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

The nil pointer part looks good. But I don't fully follow the change about UPDATE.
I would suggest adding a test to show it (and prove the change fix the issue)?

@gemmahou
Copy link
Collaborator Author

gemmahou commented Aug 29, 2025 •
edited by yuwenma
Loading

Oh it's just clean up the code(also follow the controller template). I split into a separate commit.

Suggest applying the "single feature per PR" rule especially since this is a bug fix

@gemmahou gemmahou force-pushed the fix-alloydb-nilpointer branch from 8f9ca4f to 6545465 Compare August 29, 2025 18:06
@google-oss-prow google-oss-prow bot removed the lgtm label Aug 29, 2025
@gemmahou gemmahou force-pushed the fix-alloydb-nilpointer branch from 6545465 to 88f1263 Compare September 5, 2025 19:02
yuwenma
yuwenma reviewed Sep 5, 2025
View reviewed changes
Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

We need test to verify the migration process itself (not migrated reconciler) work well. @maqiuyujoyce

@google-oss-prow google-oss-prow bot added the lgtm label Sep 5, 2025
@google-oss-prow Google OSS Prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xiaoweim xiaoweim added this to the 1.135 milestone Sep 5, 2025
@google-oss-prow google-oss-prow bot merged commit 9e1b700 into GoogleCloudPlatform:master Sep 5, 2025
110 checks passed
@gemmahou gemmahou deleted the fix-alloydb-nilpointer branch September 5, 2025 21:19
@maqiuyujoyce
Copy link
Collaborator

/lgtm /approve

We need test to verify the migration process itself (not migrated reconciler) work well. @maqiuyujoyce

Agreed. This is on my radar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuwenma yuwenma yuwenma left review comments

Assignees

@yuwenma yuwenma

@xiaoweim xiaoweim

Labels
approved lgtm service:alloydb
Projects
None yet
Milestone
1.135
Development

Successfully merging this pull request may close these issues.
5 participants
@gemmahou @xiaoweim @maqiuyujoyce @yuwenma @justinsb