Skip to content

Commit

Permalink
fix: avoids memory leak in istio sidecar termination (#972)
Browse files Browse the repository at this point in the history
## Description
Taking a stab at fixing a memory leak in core sidecar job termination
logic. This change creates a dummy stream to pass into the k8s.exec call
instead of using process.stdin.

Additional changes to the `shouldTerminate` condition were added to
account for the scenario where the sidecar was already terminated from a
previous watch firing. This was previously not an issue because we were
never removing items from `inProgress` until we added [these
lines](https://github.com/defenseunicorns/uds-core/blob/main/src/pepr/istio/index.ts#L87-L89).

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [ ] Test, docs, adr added or updated as needed
- [ ] [Contributor
Guide](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)
followed

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Micah Nagel <micah.nagel@gmail.com>
Co-authored-by: Micah Nagel <micah.nagel@defenseunicorns.com>
  • Loading branch information
4 people authored Nov 6, 2024
1 parent 4636a38 commit bfd415e
Showing 1 changed file with 26 additions and 11 deletions.
37 changes: 26 additions & 11 deletions src/pepr/istio/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { Exec, KubeConfig } from "@kubernetes/client-node";
import { Capability, a } from "pepr";
import { Readable } from "stream";
import { Component, setupLogger } from "../logger";

// configure subproject logger
Expand Down Expand Up @@ -49,16 +50,22 @@ When(a.Pod)
log.error(pod, `Invalid container status in Pod`);
return;
}
const shouldTerminate = pod.status.containerStatuses
// Ignore the istio-proxy container
.filter(c => c.name != "istio-proxy")
// and if ALL are terminated AND restartPolicy is Never or is OnFailure with a 0 exit code then shouldTerminate is true
.every(
c =>
c.state?.terminated &&
(pod.spec?.restartPolicy == "Never" ||
(pod.spec?.restartPolicy == "OnFailure" && c.state.terminated.exitCode == 0)),

// if ALL (non istio-proxy) are terminated AND restartPolicy is Never
// or is OnFailure with a 0 exit code
// and istio-proxy is not already terminated then shouldTerminate is true
const shouldTerminate = pod.status.containerStatuses.every(c => {
// handle scenario where proxy was already terminated
if (c.name == "istio-proxy") {
return c.state?.terminated == undefined;
}

return (
c.state?.terminated &&
(pod.spec?.restartPolicy == "Never" ||
(pod.spec?.restartPolicy == "OnFailure" && c.state.terminated.exitCode == 0))
);
});

if (shouldTerminate) {
// Mark the pod as seen
Expand All @@ -70,15 +77,23 @@ When(a.Pod)
kc.loadFromDefault();
const exec = new Exec(kc);

// Trying to avoid passing in process.stdin (this stream read is a no-op)
// The exec call fails with null stdin stream
const dummyStream = new Readable({
read() {
this.push(null);
},
});

await exec.exec(
namespace,
name,
"istio-proxy",
["pilot-agent", "request", "POST", "/quitquitquit"],
null, // Could capture exec stdout here
null, // Could capture exec stderr here
process.stdin,
true,
dummyStream,
false,
);

log.info(`Terminated sidecar for ${key}`);
Expand Down

0 comments on commit bfd415e

Please sign in to comment.