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

hossman · 2022-01-11T21:18:29Z

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 Support for the expiration of the token #2112 (comment)
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

The text was updated successfully, but these errors were encountered:

hossman · 2022-01-11T21:26:29Z

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 #2112 (comment) , 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 · 2022-01-13T00:06:54Z

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 · 2022-04-13T00:32:25Z

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 · 2022-04-13T05:07:07Z

This bug still exists, and is trivial to reproduce.

markusheiden · 2022-05-05T17:42:29Z

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 · 2022-05-05T17:50:49Z

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

stale · 2022-08-05T03:07:02Z

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 · 2022-09-02T08:25:08Z

Hi @hossman,
I managed to reproduce the issue in a unit test and I am proposing this fix #4381
Let me know if it works for you.

sunix · 2022-09-02T08:39:21Z

@markusheiden I am very sorry, I am just seeing now your draft PRs. Any feedback on this #4381 would be appreciated. Thanks !

hossman · 2022-09-02T17:39:17Z

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 · 2022-09-03T20:48:10Z

@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 · 2022-09-05T13:23:04Z

@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

stale bot added the status/stale label Apr 13, 2022

stale bot removed the status/stale label Apr 13, 2022

This was referenced May 5, 2022

Do not discard auth command if no args #4121

Closed

Do not discard auth command if no args #4122

Closed

stale bot added the status/stale label Aug 5, 2022

manusa added bug and removed status/stale labels Aug 10, 2022

sunix self-assigned this Aug 31, 2022

sunix mentioned this issue Sep 2, 2022

fix: Config client authentication exec command should not fail when args is null #4381

Merged

11 tasks

manusa added this to the 6.2.0 milestone Sep 22, 2022

manusa closed this as completed in #4381 Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hossman commented Jan 11, 2022 •

edited

hossman commented Jan 11, 2022

hossman commented Jan 13, 2022

stale bot commented Apr 13, 2022

hossman commented Apr 13, 2022

markusheiden commented May 5, 2022

markusheiden commented May 5, 2022 •

edited

stale bot commented Aug 5, 2022

sunix commented Sep 2, 2022

sunix commented Sep 2, 2022

hossman commented Sep 2, 2022 •

edited by manusa

markusheiden commented Sep 3, 2022

sunix commented Sep 5, 2022 •

edited

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

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

Comments

hossman commented Jan 11, 2022 • edited

Describe the bug

Fabric8 Kubernetes Client version

Steps to reproduce

Expected behavior

Runtime

Kubernetes API Server version

Environment

Fabric8 Kubernetes Client Logs

Additional context

hossman commented Jan 11, 2022

hossman commented Jan 13, 2022

stale bot commented Apr 13, 2022

hossman commented Apr 13, 2022

markusheiden commented May 5, 2022

markusheiden commented May 5, 2022 • edited

stale bot commented Aug 5, 2022

sunix commented Sep 2, 2022

sunix commented Sep 2, 2022

hossman commented Sep 2, 2022 • edited by manusa

markusheiden commented Sep 3, 2022

sunix commented Sep 5, 2022 • edited

hossman commented Jan 11, 2022 •

edited

markusheiden commented May 5, 2022 •

edited

hossman commented Sep 2, 2022 •

edited by manusa

sunix commented Sep 5, 2022 •

edited