provider/aws: Fix. Handle missing AMI name when matching against image name. #9033
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes the issues where in a very rare cases the Amazon Machine Image (AMI) would not have an image name set causing regular expression match to fail with a nil pointer dereference. Also, the logic of if-else statements was simplified (reduced branching since return is used a lot). Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
|
This resolves #8786. |
|
Acceptance test is passing: |
kwilczynski
changed the title
|
In the |
|
I also testes the use case reported by @moritzheiber, as per: It completes successfully now. |
| if image.Name == nil || *image.Name == "" { | ||
| log.Printf("[WARN] Unable to find AMI name to match against "+ | ||
| "for image ID %q owned by %q, nothing to do.", | ||
| *image.ImageId, *image.OwnerId) |
There was a problem hiding this comment.
Given the problem here I'm now feeling paranoid: will image.OwnerId ever be null? 😀
I'm probably over-thinking it, since it seems pretty unlikely that an image would ever not have an owner, but perhaps we should cut this down to just reporting the image id to reduce the chance that we'll crash here trying to prevent a crash.
There was a problem hiding this comment.
A little bit of healthy paranoia is what makes you question things, which is often good.
You need to have an account to be able to create and/or upload an image to use later, plus this is provided by the backend, nothing that user can control and/or set. In other words, I believe we are fine with both image ID and owner.
|
Thanks, @kwilczynski! I got some warnings when running the acceptance tests: But this (occuring in the AWS client) didn't seem directly related to your change, so I merged in spite of it. |
|
@apparentlymart speaking of paranoia ... After you mentioned the warnings, I re-tested the code locally too, seem to be fine, as per: Built with I am not seeing the same warning. Although, this is a bit unsettling. |
|
@apparentlymart do you enable extra logging to get the warning out? |
|
@kwilczynski I did not... I just ran the acceptance tests using the same command line as you originally posted. Curiously, after doing exactly the same thing again I didn't see the problem, so perhaps it truly is a race condition! |
|
@apparentlymart thank you! I suppose, we should leave it to be one day solved, alongside the Loch Ness monster, crop circles, etc. |
kwilczynski
added a commit
to kwilczynski/terraform
that referenced
this pull request
Oct 7, 2016
This commit resolves a regression introduced in hashicorp#9033 that caused an unfiltered image to be returned despite a search criteria being set accordingly. Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
This was referenced Oct 7, 2016
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes the issues where in a very rare cases the Amazon Machine
Image (AMI) would not have an image name set causing regular expression match
to fail with a nil pointer dereference. Also, the logic of if-else statements
was simplified (reduced branching since return is used a lot).
Signed-off-by: Krzysztof Wilczynski krzysztof.wilczynski@linux.com