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 nil pointer references #65

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Fix nil pointer references #65

merged 1 commit into from
Sep 19, 2022

Conversation

janboll
Copy link
Contributor

@janboll janboll commented Sep 19, 2022

The vpc.go sometimes crashes with nil pointer reference. The stacktrace indicates, that it's related to nil values returned for certain AWS calls

@janboll janboll requested a review from mrWinston September 19, 2022 07:50
Copy link
Contributor

@mrWinston mrWinston left a comment

Choose a reason for hiding this comment

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

/lgtm

I'm wondering if we should provide a more precise error message in the logs in case that error happens. But its not a blocker for me as we can find out if it happened by searching for error logs that have "err=nil".

@patjlm
Copy link
Contributor

patjlm commented Sep 19, 2022

For my knowledge, in which case do we get nil there, without any error ?

@janboll
Copy link
Contributor Author

janboll commented Sep 19, 2022

That's a very good question. I was fingering from the fact, that the function returns a pointer. Also the code is failing at i.e. line 154, if you check that line the only thing, that can be nil is the output. Not sure, but it might be an issue with the SDK, found for instance: aws/aws-sdk-go#3619
I'll go ahead and upgrade the SDK to the latest 1.x release.

@janboll
Copy link
Contributor Author

janboll commented Sep 19, 2022

Tested locally using our stage and prod AWS accounts.

@janboll
Copy link
Contributor Author

janboll commented Sep 19, 2022

Removed the nil check and just bumped the SDK to check if was only an upstream issue.

Copy link
Contributor

@patjlm patjlm left a comment

Choose a reason for hiding this comment

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

LGTM

@janboll janboll merged commit f3336e9 into app-sre:master Sep 19, 2022
@janboll janboll deleted the fix-crash branch September 19, 2022 13:50
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.

3 participants