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

Bug fix - Add nil pointer check for LoadBalancerProfile in cluster metrics #3695

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

s-fairchild
Copy link
Collaborator

@s-fairchild s-fairchild commented Jul 15, 2024

Which issue this PR addresses:

ARO-9146

Fixes

What this PR does / why we need it:

If the doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile is nil, checking for a nil doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.ManagedOutboundIPs will cause a panic.
Relevant line from stack trace:

(*openShiftClusterBackend).gatherNetworkMetrics(0xc000a0cff0, 0x5c35fa0?, 0xc00109c600, 0x6?)\n\t/app/pkg/backend/metrics.go:221 +0x930\ngithub.com/Azure/ARO-RP/pkg/backend.

Checking for doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile first will stop a nil doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.ManagedOutboundIPs from being de referenced.

Test plan for issue:

Covered by unit tests.
Will also create a UDR cluster with this document: https://github.com/Azure/ARO-RP/blob/master/docs/deploy-development-rp.md#run-the-rp-and-create-a-cluster

Is there any documentation that needs to be updated for this PR?

No.

How do you know this will function as expected in production?

Unit tests added pass a nil LoadBalancerProfile on this branch, while causing a panic on master.

@s-fairchild s-fairchild force-pushed the s-fairchild/ARO-9146-fix-metrics branch from 70c1286 to 2d3375a Compare July 15, 2024 17:06
@tsatam tsatam added release-blocker next-release To be included in the next RP release rollout labels Jul 15, 2024
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

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

LGTM. It doesn't need to be done immediately within this PR, but as soon as we can, we should introduce a panic guard within this metrics function so that future nil pointer dereferences or other panics cannot halt execution on the backend.

@cadenmarchese
Copy link
Collaborator

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@jhoreman jhoreman left a comment

Choose a reason for hiding this comment

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

Approved, thanks for this!

Copy link
Collaborator

@anshulvermapatel anshulvermapatel 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
Collaborator

@SrinivasAtmakuri SrinivasAtmakuri left a comment

Choose a reason for hiding this comment

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

LGTM!

@sankur-codes
Copy link
Collaborator

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cadenmarchese cadenmarchese merged commit 639fd86 into master Jul 16, 2024
21 checks passed
@SudoBrendan SudoBrendan deleted the s-fairchild/ARO-9146-fix-metrics branch July 24, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chainsaw Pull requests or issues owned by Team Chainsaw next-release To be included in the next RP release rollout ready-for-review release-blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants