Add JavaScript hooks

[tsmetana]

What type of PR is this?
/kind feature

What this PR does / why we need it:
In order to be able to test also various failure situations I've added a JavaScript interpreter and hooks to the CSI methods. The hooks content is meant to be configured by a ConfigMap containing JavaScript snippets. Furthermore if the snippet evaluates to an integer value it gets interpreted as a gRPC code to be immediately returned from the particular method. This allows for things like "fail twice with DeadlineExceeded then once with Aborted and then succeed" or even more complicated logic.

Does this PR introduce a user-facing change?:

The CSI mock driver behaviour can be tweaked with JavaScript hooks in the CSI calls.

[k8s-ci-robot]

Welcome @tsmetana!

It looks like this is your first PR to kubernetes-csi/csi-test 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-test has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @tsmetana. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pohly]

This looks promising, thanks for starting this work. Has this been discussed somewhere already?

Can you add documentation on how to use this feature? That probably would answer a lot of the questions that I now have 😁

For example, can (and if so, how) hooks keep persistent state between invocations? This is important for the "fail every second call" use case from #249.

Perhaps you could even use that use case as example and show how to do it?

It doesn't look like the hooks have access to the method parameters, nor can they fabricate a full response message, right? Would it be possible to add that? Just mocking error codes based on call counts is already a good first start, but I suppose there will be scenarios where that is too limited.

[pohly]

/ok-to-test

[pohly]

/ok-to-test

[tsmetana]

Thanks fro the comment -- it's what I was hoping for. :)

The idea comes from kitchen chats with @jsafrane and @bertinatto and that's about all the discussions I lead about it so far... Actually getting some opinions whether this makes sense and what exactly do we need is a main point of this PR.

Here's an usage example for illustration: First, prepare a YAML file defining some JavaScript hooks. Let's call it hooks.yaml:

Globals: |
  count = 0;
  console.log("Globals loaded, count set to " + count);
CreateVolumeStart: |
  count = count + 1;
  console.log("CreateVolumeStart: The value of count is " + count);
  if (count > 2) { __grpcCode.OK; } else {  __grpcCode.DEADLINEEXCEEDED; };
CreateVolumeEnd: |
  console.log("CreateVolumeEnd");

I turn it to a ConfigMap:

kubectl create configmap hooks-config --from-file hooks.yaml

Make my CSI mock driver to use it -- this is the sidecar definition YAMLs:

...
containers:
  ...
  - name: csi-mock-plugin
    ...
    args:
      - --hooks-file=/var/lib/hooks/hooks.yaml
    ...
    volumeMounts:
      ...
      - name: hooks-volume
        mountPath: /var/lib/hooks
...
volumes:
  - name: hooks-volume
    configMap:
      name: hooks-config
...

Now the driver finds the hooks definitions file and creates a JavaScript VM. Just one instance of the VM exists through the life of the driver. Then it executes the pieces of code as per the YAML file. One special case is the Globals hook that is executed right after loading the file and it's intended for variable initializations, common function definitions, etc. I also hardcoded injection of the gRPC code constants for convenience.

Each code snippet is executed at given place in the code: in our example CreateVolumeStart at the beginning of the CreateVolume method in the driver. The result of the hook snipped is evaluated: it would be the value of the last expression executed. If the result is an integer value it gets interpreted as gRPC code and in case it's non-zero (not code.OK), the CSI driver method would return this as its response error.

In our example: I initialized the variable count in the Globals hook to zero. Each time we get into the CreateVolume method in the driver the hook is executed so count is increased by one (it's always the same variable) and for the first two calls the last expression of the code snipped is __grpcCode.DEADLINEEXCEEDED which causes the CreateVolume method to fail with the given code. Subsequent calls would return 0 so things would work normally. Also -- the console.out() allows me to log to stdout so I can see the messages from the hooks among the output from the driver itself...

Now, is this useful? Could I make it useful? I understand that passing one number in one direction is quite limited ability but I also don't want to make the code too complicated. It really depends on use-cases we might have.

[pohly]

Now, is this useful?

I find it useful already with the current limitations. My only concern is the amount of additional dependencies for the JS interpreter, but that cannot be avoided and IMHO isn't too bad.

Let's hear from others... @jsafrane, @msau42?

[jsafrane]

I like the javascript hooks. I'm adding tests for NodeStage error cases in https://github.com/kubernetes/kubernetes/pull/89041/files and it would not be possible without the hooks.

Adding the hooks to the test is easy and looks quite natural in go. What makes the test complicated is matching of actual CSI driver calls parsed from mock driver stout with calls expected by the test. It's unrelated to the javascript hooks.

The only one thing I struggle with is javascript itself. I accidentally used __grpcCode.INVALID_ARGUMENT in a hook and I was surprised the CSI call returned success. It took me quite some time to realize what was wrong and that non-existing INVALID_ARGUMENT was translated to 0 (__grpcCode.INVALIDARGUMENT is the correct value).

[jsafrane]

Regarding javascript interpreter dependencies. Yes, they're huge and it does not look that the interpreter is well maintained. On the other hand, it is used only for testing and any bugs there (or even CVEs) should not hurt anyone.

[jsafrane]

Found two nits, looks good otherwise

[jsafrane]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, tsmetana

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment