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

Use created namespace name during run --wait #930

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

zubron
Copy link
Contributor

@zubron zubron commented Oct 1, 2019

What this PR does / why we need it:
When using a manifest from a file and the --wait flag with run, a
segfault would happen because the value is retrieved from the
config.Config which is part of the GenConfig within the RunConfig.
When using a file, this config is nil, so an attempt to retrieve the
namespace value resulted in a segfault. Instead, we can capture the
namespace from the objects within the manifest file when they are being
created by the Kubernetes client.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

Which issue(s) this PR fixes

Special notes for your reviewer:
I've written added an integration level test to check this and confirmed that the segfault exists before the change and is fixed by it.

Release note:

Fixed a bug where a segfault would occur with `sonobuoy run --wait` when providing a manifest with a file.

When using a manifest from a file and the `--wait` flag with `run`, a
segfault would happen because the value is retrieved from the
`config.Config` which is part of the `GenConfig` within the `RunConfig`.
When using a file, this config is nil, so an attempt to retrieve the
namespace value resulted in a segfault. Instead, we can capture the
namespace from the objects within the manifest file when they are being
created by the Kubernetes client.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
@codecov-io
Copy link

Codecov Report

Merging #930 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
- Coverage   47.59%   47.56%   -0.03%     
==========================================
  Files          76       76              
  Lines        5175     5178       +3     
==========================================
  Hits         2463     2463              
- Misses       2562     2565       +3     
  Partials      150      150
Impacted Files Coverage Δ
pkg/client/run.go 15.46% <0%> (-0.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c799770...e1f48c8. Read the comment docs.

// It may not be available within the RunConfig but we can retrieve it from the
// namespace of objects within the manifest.
if createdNamespace == "" && namespace != "" {
createdNamespace = namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use namespace? I'm not clear on what createdNamespace adds since by default it is the empty string, and it is set to namespace if it isn't the empty string. So won't it just always match namespace?

Copy link
Contributor Author

@zubron zubron Oct 2, 2019

Choose a reason for hiding this comment

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

This loop is iterating over all the resources in the yaml output. Some of those are cluster-scoped (e.g. ClusterRole) which means that they do not have an associated namespace. If the yaml was edited to have these resources be the last ones to be iterated over, namespace would be empty at the end of the loop. You can see this in the logs from Sonobuoy run, some of the resources it logs don't have the namespace field set in the log. Storing the first non empty namespace was an attempt to combat that. This approach isn't fool proof, but seemed good enough for now as it avoids the need to fully parse the yaml and find the Namespace resource, or the contents of the config map (we do this in gen_test).

@johnSchnake johnSchnake merged commit 7218d76 into vmware-tanzu:master Oct 2, 2019
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.

Segfault during run --wait when reading manifest from stdin/file
3 participants