Config execs wrong command when user.exec.args is null (as generated by kubectl config set-credentials)

[hossman]

Describe the bug

kubectl set-credentials (v1.23.1) can generate an exec type user with args: null

But when io.fabric8.kubernetes.client.Config parses that user info, the null args seems to confuse it, causing it to run the wrong command.

kubectl config set-credentials some_user --exec-api-version='client.authentication.k8s.io/v1beta1' --exec-command=/tmp/auth.sh

...resulting ~/.kube/config entry...

- name: some_user
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1beta1
      args: null
      command: /tmp/auth.sh
      env: null
      provideClusterInfo: false

But when io.fabric8.kubernetes.client.Config parses that user info, the null args seems to confuse it, causing it to run the wrong command.

Fabric8 Kubernetes Client version

5.10.1@latest

Steps to reproduce

kubectl config set-credentials some_user --exec-api-version='client.authentication.k8s.io/v1beta1' --exec-command=/tmp/auth.sh

...resulting ~/.kube/config entry...

- name: some_user
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1beta1
      args: null
      command: /tmp/auth.sh
      env: null
      provideClusterInfo: false

Then attempt to use this config with fabric8io/kubernetes-client -- the resulting failure indicates that it is is not invoking the command correctly, and getting a syntax error from sh

Expected behavior

fabric8io/kubernetes-client should work with any kube config generated by the kubectl config command line

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

other (please specify in additional context)

Environment

Linux

Fabric8 Kubernetes Client Logs

i.f.k.c.Config                 [ERROR] Failed to parse the kubeconfig.
    mapping values are not allowed here
     in 'reader', line 1, column 6:
        sh: 0: -c requires an argument
             ^

        at org.yaml.snakeyaml.scanner.ScannerImpl.fetchValue(ScannerImpl.java:890)
        at org.yaml.snakeyaml.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java:379)
        at org.yaml.snakeyaml.scanner.ScannerImpl.checkToken(ScannerImpl.java:248)
        at org.yaml.snakeyaml.parser.ParserImpl$ParseBlockMappingKey.produce(ParserImpl.java:602)
        at org.yaml.snakeyaml.parser.ParserImpl.peekEvent(ParserImpl.java:165)
        at org.yaml.snakeyaml.comments.CommentEventsCollector$1.peek(CommentEventsCollector.java:59)
        at org.yaml.snakeyaml.comments.CommentEventsCollector$1.peek(CommentEventsCollector.java:45)
        at org.yaml.snakeyaml.comments.CommentEventsCollector.collectEvents(CommentEventsCollector.java:140)
        at org.yaml.snakeyaml.comments.CommentEventsCollector.collectEvents(CommentEventsCollector.java:119)
        at org.yaml.snakeyaml.composer.Composer.composeScalarNode(Composer.java:221)
        at org.yaml.snakeyaml.composer.Composer.composeNode(Composer.java:191)
        at org.yaml.snakeyaml.composer.Composer.composeValueNode(Composer.java:313)
        at org.yaml.snakeyaml.composer.Composer.composeMappingChildren(Composer.java:304)
        at org.yaml.snakeyaml.composer.Composer.composeMappingNode(Composer.java:288)
        at org.yaml.snakeyaml.composer.Composer.composeNode(Composer.java:195)
        at org.yaml.snakeyaml.composer.Composer.getNode(Composer.java:115)
        at org.yaml.snakeyaml.composer.Composer.getSingleNode(Composer.java:146)
        at org.yaml.snakeyaml.constructor.BaseConstructor.getSingleData(BaseConstructor.java:151)
        at org.yaml.snakeyaml.Yaml.loadFromReader(Yaml.java:490)
        at org.yaml.snakeyaml.Yaml.load(Yaml.java:429)
        at io.fabric8.kubernetes.client.utils.Serialization.unmarshalYaml(Serialization.java:369)
        at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:310)
        at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:235)
        at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:221)
        at io.fabric8.kubernetes.client.Config.getExecCredentialFromExecConfig(Config.java:678)
        at io.fabric8.kubernetes.client.Config.loadFromKubeconfig(Config.java:638)

Additional context

• server is never involved, so server version is irrelevant
• see also https://github.com/fabric8io/kubernetes-client/issues/2112#issuecomment-1009480516
• work around is to ensure that the command will ignore any arguments, and pass --exec-arg=bogus_unused_bug_workaround to kubectl config set-credentials so that the kube-config does not have a null args ...

kubectl config set-credentials some_user --exec-api-version='client.authentication.k8s.io/v1beta1' --exec-command=/tmp/auth.sh --exec-arg=bogus_unused_bug_workaround

- name: some_user
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1beta1
      args:
      - bogus_unused_bug_workaround
      command: /tmp/auth.sh
      env: null
      interactiveMode: IfAvailable
      provideClusterInfo: false

[hossman]

Here's what i think is happening:

• Config.getAuthenticatorCommandFromExecConfig() is straight up ignoring exec.getCommand() unless exec.getArgs() is non-null.
• ExecConfig.args defaults to new ArrayList<String>(); ... which gets used when the ExecConfig has no args key
• but when args: null is in the YAML, that empty list is (evidently) replaced with null

  protected static List<String> getAuthenticatorCommandFromExecConfig(ExecConfig exec, File configFile, String systemPathValue) {
    String command = exec.getCommand();
    if (command.contains(File.separator) && !command.startsWith(File.separator) && configFile != null) {
      // Appears to be a relative path; normalize. Spec is vague about how to detect this situation.
      command = Paths.get(configFile.getAbsolutePath()).resolveSibling(command).normalize().toString();
    }
    List<String> argv = new ArrayList<>(Utils.getCommandPlatformPrefix());
    command = getCommandWithFullyQualifiedPath(command, systemPathValue);
    List<String> args = exec.getArgs();
    if (args != null) {
      argv.add(command + " " + String.join( " ", args));
    }
    return argv;
  }

(NOTE: as mentioned in https://github.com/fabric8io/kubernetes-client/issues/2112#issuecomment-1009480516 , this command + " " + String.join( " ", args) logic is problematic for a lot of reasons, but the fact that argv contains nothing except ["sh","-c"] when exec.getArgs(); returns null seems to be the crux of this particular bug.)

[hossman]

I'm attaching the simplest possible test patch i can think of that demonstrates (on non-windows OS) the various problems with how the "stringification" of the command + arguments is handled in Config.java ...

arg_problems_with_auth_exec.patch.txt

...this bad behavior -- stemming from the desire to wrap the configured command + args in sh -c definitely seems to have been introduced by issue #2308. I can't really wrap my head around any of these changes, because the entire premise of the change doesn't make any sense to me...

+1 So it's definitely a problem with ProcessBuilder not using your env PATH.

... ProcessBuilder does most definitely uses the system "PATH" that was set when the java process was invoked...

hossman@slate:~/tmp/pb-path-testing$ find .
.
./PbTest.class
./dir-in-path
./dir-in-path/my_cmd.sh
./PbTest.java
hossman@slate:~/tmp/pb-path-testing$ cat ./PbTest.java
public final class PbTest {
    public static void main(String[] args) throws Exception {
        Process p = new ProcessBuilder("my_cmd.sh", "arg1").inheritIO().start();
    }
}
hossman@slate:~/tmp/pb-path-testing$ cat ./dir-in-path/my_cmd.sh 
#!/bin/bash
echo $1
hossman@slate:~/tmp/pb-path-testing$ PATH=/opt/jdk/11/latest//bin java PbTest 
Exception in thread "main" java.io.IOException: Cannot run program "my_cmd.sh": error=2, No such file or directory
	at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1128)
	at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1071)
	at PbTest.main(PbTest.java:3)
Caused by: java.io.IOException: error=2, No such file or directory
	at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
	at java.base/java.lang.ProcessImpl.<init>(ProcessImpl.java:340)
	at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:271)
	at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1107)
	... 2 more
hossman@slate:~/tmp/pb-path-testing$ PATH=/opt/jdk/11/latest//bin:/home/hossman/tmp/pb-path-testing/dir-in-path/ java PbTest 
arg1

...so i don't really understand what exactly this change was expected to accomplish?

I also think it's worth noting: the author of the original bug report (quarkusio/quarkus/issues/10191) that that spawned issue #2308 never confirmed that the "fix" actually resolved anything for them. (As someone who has no idea what quarkus is or how it works, i can't help but wonder if their root problem was that some wrapper script that invoked the java process where kubernetes-client is used had overridden their default PATH?)

[stale[bot]]

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

[hossman]

This bug still exists, and is trivial to reproduce.

[markusheiden]

On macOS it looks like this:

org.yaml.snakeyaml.scanner.ScannerException: mapping values are not allowed here
 in 'reader', line 1, column 7:
    sh: -c: option requires an argument
          ^

	at org.yaml.snakeyaml.scanner.ScannerImpl.fetchValue(ScannerImpl.java:890)
	at org.yaml.snakeyaml.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java:379)
	at org.yaml.snakeyaml.scanner.ScannerImpl.checkToken(ScannerImpl.java:248)
	at org.yaml.snakeyaml.parser.ParserImpl$ParseBlockMappingKey.produce(ParserImpl.java:634)
	at org.yaml.snakeyaml.parser.ParserImpl.peekEvent(ParserImpl.java:165)
	at org.yaml.snakeyaml.comments.CommentEventsCollector$1.peek(CommentEventsCollector.java:59)
	at org.yaml.snakeyaml.comments.CommentEventsCollector$1.peek(CommentEventsCollector.java:45)
	at org.yaml.snakeyaml.comments.CommentEventsCollector.collectEvents(CommentEventsCollector.java:140)
	at org.yaml.snakeyaml.comments.CommentEventsCollector.collectEvents(CommentEventsCollector.java:119)
	at org.yaml.snakeyaml.composer.Composer.composeScalarNode(Composer.java:214)
	at org.yaml.snakeyaml.composer.Composer.composeNode(Composer.java:184)
	at org.yaml.snakeyaml.composer.Composer.composeValueNode(Composer.java:314)
	at org.yaml.snakeyaml.composer.Composer.composeMappingChildren(Composer.java:305)
	at org.yaml.snakeyaml.composer.Composer.composeMappingNode(Composer.java:286)
	at org.yaml.snakeyaml.composer.Composer.composeNode(Composer.java:188)
	at org.yaml.snakeyaml.composer.Composer.getNode(Composer.java:115)
	at org.yaml.snakeyaml.composer.Composer.getSingleNode(Composer.java:142)
	at org.yaml.snakeyaml.constructor.BaseConstructor.getSingleData(BaseConstructor.java:151)
	at org.yaml.snakeyaml.Yaml.loadFromReader(Yaml.java:491)
	at org.yaml.snakeyaml.Yaml.load(Yaml.java:429)
	at io.fabric8.kubernetes.client.utils.Serialization.unmarshalYaml(Serialization.java:318)
	at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:259)
	at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:184)
	at io.fabric8.kubernetes.client.utils.Serialization.unmarshal(Serialization.java:170)
	at io.fabric8.kubernetes.client.Config.getExecCredentialFromExecConfig(Config.java:678)
	at io.fabric8.kubernetes.client.Config.loadFromKubeconfig(Config.java:638)
	at io.fabric8.kubernetes.client.Config.tryKubeConfig(Config.java:557)
	at io.fabric8.kubernetes.client.Config.autoConfigure(Config.java:279)
        ...

[markusheiden]

This is especially annoying because GCP moves to the new gke-gcloud-auth-plugin which uses null args.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

[sunix]

Hi @hossman, I managed to reproduce the issue in a unit test and I am proposing this fix https://github.com/fabric8io/kubernetes-client/pull/4381 Let me know if it works for you.

[sunix]

@markusheiden I am very sorry, I am just seeing now your draft PRs. Any feedback on this https://github.com/fabric8io/kubernetes-client/pull/4381 would be appreciated. Thanks !

[hossman]

I haven't tried out either PR, because frankly I didn't feel like it was worth my time to help test out any solution that continues to use String.join(...) on arguments passed to ProcessBuilder. It's terribly bad smelling code, and any "fix" that keeps it around is almost certainly just a bandaid over the larger problem of how this code is invoking sub-processes with (or w/o) arguments.

If you look at the patch file I previously attached, it not only included a test case for the specific "null argument" problem, but also modified the existing test to demonstrate how the sub-process is not (always) getting the actual list of arguments passed by the user. (or at least: it wasn't when I wrote that test patch)

I'm not really that familia with the code base - but AFAICT, almost everything about PR #2381 should be reverted (the original bug report seems completely invalid, w/o any confirmation of the root cause, and the "fix" was never confirmed by the original reporter) and the code should go back to constructing a ProcessBuilder using a List<String> argv

[markusheiden]

@sunix I didn't test your code (no time yet) but it looks very similar to my patch. Thus I assume it should fix the problem.

@hossman I agree that the code quality of the ProcessBuilder-related stuff could be better: Instead of joining strings, one could simply add the args to the argv list after the command. I don't understand your issue about the bug report though. The problem is described clearly, a real-world use case exists, and I validated that my fix fixes the problem.

[sunix]

@hossman I think it is worth creating a separate issue. IMHO, it is not that straight forward, the process builder is invoking already sh, -c, and the subcommand + args as one argument.

I managed to have the test "WE SAY HELLO WORLD" in your test with the following config. Not sure it is what we expect, so we should have a separate issue to discuss and implement. The problem I see is if you pass a file path with spaces as an argument ... it won't be taken as a single argument...

apiVersion: v1
kind: Config
clusters:
- cluster:
    server: https://wherever
  name: test
contexts:
- context:
    cluster: test
    user: test
  name: test
current-context: test
users:
- name: test
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1alpha1
      args:
      - world
      - "\"We say\""
      command: ./token-generator
      env:
      - name: PART1
        value: hello