Skip to content

Commit

Permalink
Revert "k8s: Use TTY to cancel execution"
Browse files Browse the repository at this point in the history
This reverts commit 8d0139b. The
current implementation breaks 'cilium sysdump', so I think we'll need to
rethink that one through.

Signed-off-by: Bruno Miguel Custódio <[email protected]>
  • Loading branch information
bmcustodio authored and tklauser committed Jul 7, 2021
1 parent 0f846e4 commit 6d19810
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 91 deletions.
3 changes: 2 additions & 1 deletion connectivity/check/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ func (a *Action) ExecInPod(ctx context.Context, cmd []string) {

a.Debug("Executing command", cmd)

stdout, stderr, err := pod.K8sClient.ExecInPodWithStderr(ctx,
// Warning: ExecInPod* does not use ctx, command cannot be cancelled.
stdout, stderr, err := pod.K8sClient.ExecInPodWithStderr(context.TODO(),
pod.Pod.Namespace, pod.Pod.Name, pod.Pod.Labels["name"], cmd)

cmdName := cmd[0]
Expand Down
12 changes: 9 additions & 3 deletions connectivity/check/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,9 @@ func (ct *ConnectivityTest) waitForDNS(ctx context.Context, pod Pod) error {
r := time.After(time.Second)

target := "kube-dns.kube-system.svc.cluster.local"
stdout, _, err := pod.K8sClient.ExecInPodWithStderr(ctx, pod.Pod.Namespace, pod.Pod.Name,
// Warning: ExecInPod ignores ctx. Don't pass it here so we don't
// falsely expect the function to be able to be cancelled.
stdout, _, err := pod.K8sClient.ExecInPodWithStderr(context.TODO(), pod.Pod.Namespace, pod.Pod.Name,
"", []string{"nslookup", target})
if err == nil {
return nil
Expand All @@ -527,7 +529,9 @@ func (ct *ConnectivityTest) waitForIPCache(ctx context.Context, pod Pod) error {
// Don't retry lookups more often than once per second.
r := time.After(time.Second)

stdout, err := pod.K8sClient.ExecInPod(ctx, pod.Pod.Namespace, pod.Pod.Name,
// Warning: ExecInPod ignores ctx. Don't pass it here so we don't
// falsely expect the function to be able to be cancelled.
stdout, err := pod.K8sClient.ExecInPod(context.TODO(), pod.Pod.Namespace, pod.Pod.Name,
"cilium-agent", []string{"cilium", "bpf", "ipcache", "list", "-o", "json"})
if err == nil {
var ic ipCache
Expand Down Expand Up @@ -607,7 +611,9 @@ func (ct *ConnectivityTest) waitForService(ctx context.Context, service Service)
// Don't retry lookups more often than once per second.
r := time.After(time.Second)

_, e, err := ct.client.ExecInPodWithStderr(ctx,
// Warning: ExecInPodWithStderr ignores ctx. Don't pass it here so we don't
// falsely expect the function to be able to be cancelled.
_, e, err := ct.client.ExecInPodWithStderr(context.TODO(),
pod.Pod.Namespace, pod.Pod.Name, pod.Pod.Labels["name"],
[]string{"nslookup", service.Service.Name}) // BusyBox nslookup doesn't support any arguments.

Expand Down
2 changes: 1 addition & 1 deletion connectivity/check/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func getCiliumPolicyRevision(ctx context.Context, pod Pod) (int, error) {
}
revision, err := strconv.Atoi(strings.Trim(stdout.String(), "'\n"))
if err != nil {
return 0, fmt.Errorf("revision %q is not valid: %w", stdout.String(), err)
return 0, fmt.Errorf("revision '%s' is not valid: %w", stdout.String(), err)
}
return revision, nil
}
Expand Down
21 changes: 4 additions & 17 deletions internal/k8s/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"context"
"fmt"

"github.com/cilium/cilium-cli/internal/utils"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/remotecommand"
Expand Down Expand Up @@ -50,10 +49,10 @@ func (c *Client) execInPod(ctx context.Context, p ExecParameters) (*ExecResult,
req.VersionedParams(&corev1.PodExecOptions{
Command: p.Command,
Container: p.Container,
Stdin: true,
Stdin: false,
Stdout: true,
Stderr: true,
TTY: true,
TTY: false,
}, parameterCodec)

exec, err := remotecommand.NewSPDYExecutor(c.Config, "POST", req.URL())
Expand All @@ -62,23 +61,11 @@ func (c *Client) execInPod(ctx context.Context, p ExecParameters) (*ExecResult,
}
result := &ExecResult{}

// CtrlCReader sends Ctrl-C/D sequence if context is cancelled
stdin := utils.NewCtrlCReader(ctx)
// Graceful close of stdin once we are done, no Ctrl-C is sent
// if execution finishes before the context expires.
defer stdin.Close()

err = exec.Stream(remotecommand.StreamOptions{
Stdin: stdin,
Stdin: nil,
Stdout: &result.Stdout,
Stderr: &result.Stderr,
Tty: true,
Tty: false,
})

// Replace "\r\n" sequences in stdout with "\n"
if bytes.Contains(result.Stdout.Bytes(), []byte("\r\n")) {
result.Stdout = *bytes.NewBuffer(bytes.ReplaceAll(result.Stdout.Bytes(), []byte("\r\n"), []byte("\n")))
}

return result, err
}
69 changes: 0 additions & 69 deletions internal/utils/ctrlcreader.go

This file was deleted.

0 comments on commit 6d19810

Please sign in to comment.