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.

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

fix(kubernetes): tolerate an empty manifest in KubernetesDeployManifestOperation #6337

Merged
merged 1 commit into from
Jan 23, 2025
Merged

fix(kubernetes): tolerate an empty manifest in KubernetesDeployManifestOperation #6337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions ...tflix/spinnaker/clouddriver/kubernetes/op/manifest/KubernetesDeployManifestOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,16 @@ private List<KubernetesManifest> getManifestsFromDescription() {
// check `manifest` for backwards compatibility until all existing stages have been updated.
@SuppressWarnings("deprecation")
KubernetesManifest manifest = description.getManifest();
log.warn(
"Relying on deprecated single manifest input (account: {}, kind: {}, name: {})",
accountName,
manifest.getKind(),
manifest.getName());
inputManifests = ImmutableList.of(manifest);

// manifest may be null as well, so check
if (manifest != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Hrmm, wondering if shouldn't log "Warning! Input manifest is null and description is null. Something is incorrect on the deployment most likely!" or similar. I'd not have though BOTH would be null... ALSO... won't this immediately fail below on lin 242 with an NPEsince inputManifests would be null in this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forget how this all works with description.getManifest() vs description.getManifests(), but apparently inputManifests isn't null in this case, so things work. If this did end up generating a NullPointerException, we'd have seen it. I have a vague memory that @mattgogerly may know something about this, but I think this change is OK. It's at least an improvement if it doesn't fix everything.

Copy link
Member

Choose a reason for hiding this comment

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

MOSTLY calling out the next line:

inputManifests = inputManifests.stream().filter(Objects::nonNull).collect(Collectors.toList());

inputManifests would be null, triggering an NPE if it got to the point of your null check. ....

Copy link
Member

Choose a reason for hiding this comment

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

I guess if the inputManifests.isEmpty() would allow this to "work" without an NPE... that may be how this is working...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figure that's what's happening.

Copy link
Member

Choose a reason for hiding this comment

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

SIGH I hate "it's working through a glitch" situations... but... approving. I could see a "TOOD" to note to fix this... but as long as it works it works... and tests'll catch it hopefully!

log.warn(
"Relying on deprecated single manifest input (account: {}, kind: {}, name: {})",
accountName,
manifest.getKind(),
manifest.getName());
inputManifests = ImmutableList.of(manifest);
}
}
inputManifests = inputManifests.stream().filter(Objects::nonNull).collect(Collectors.toList());
return inputManifests;
Expand Down
7 changes: 7 additions & 0 deletions ...om/netflix/spinnaker/clouddriver/kubernetes/op/KubernetesDeployManifestOperationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,13 @@ void replaceStrategyWithLabelSelector() {
anyString());
}

@Test
void deployEmptyResource() {
KubernetesDeployManifestDescription deployManifestDescription =
baseDeployDescription("deploy/empty-resource.yml");
deploy(deployManifestDescription);
}

private static KubernetesDeployManifestDescription baseDeployDescription(String manifest) {
return baseDeployDescription(manifest, false);
}
Expand Down
1 change: 1 addition & 0 deletions .../test/resources/com/netflix/spinnaker/clouddriver/kubernetes/op/deploy/empty-resource.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

Loading