RDX: Work with deployment profiles #4655

mook-as · 2023-05-11T20:34:22Z

This handles #4341 — we now have a preference that can be used to limit the extensions that can be installed.
See attached BATS test case to see how it works.

rdctl set intentionally doesn't support this, as it's more meant for use with locked profiles. But (as seen in the tests) it's possible to set it via the API.

Nothing is hooked up to it yet. Signed-off-by: Mark Yen <[email protected]>

This lets administrators supply a list of image references that describe which extensions are allowed to be installed (optionally with a tag). It's possible to set this to an empty list (disallowing any extensions). Signed-off-by: Mark Yen <[email protected]>

Signed-off-by: Mark Yen <[email protected]>

ericpromislow · 2023-05-12T21:42:14Z

bats/tests/extensions/allow-list.bats

+    fi
+}
+
+write_allow_list() { # list


Maybe a test that passes more than one arg to this function, because currently it only handles 0 or 1 values in the list?

It takes a single argument that is the JSON encoded list.

I realized that, but none of the args contain a comma

Ah, I thought I had one. Will fix.

ericpromislow · 2023-05-12T21:47:33Z

bats/tests/extensions/allow-list.bats

+
+@test 'when an extension is explicitly allowed, it can be installed' {
+    write_allow_list '["rd/extension/basic:latest"]'
+    rdctl extension install rd/extension/basic:latest


I would also run rdctl extension ls and check for success and basic:latest when the extension whitelist is in effect

ericpromislow · 2023-05-12T21:48:26Z

bats/tests/extensions/allow-list.bats

+    rdctl extension uninstall rd/extension/basic
+}
+
+@test 'when an extension is not in the allowe list, it cannot be installed' {


typo: "allowed"

ericpromislow · 2023-05-12T21:48:55Z

bats/tests/extensions/allow-list.bats

+    write_allow_list '["rd/extension/basic"]'
+    ctrctl "${namespace_arg[@]}" tag rd/extension/basic rd/extension/basic:0.0.3
+    rdctl extension install rd/extension/basic:0.0.3
+    rdctl extension uninstall rd/extension/basic


Again, I would verify that the extension is actually installed, not just that ext install didn't fail

ericpromislow · 2023-05-12T22:01:55Z

pkg/rancher-desktop/main/extensions/extensions.ts

@@ -142,9 +143,59 @@ export class ExtensionImpl implements Extension {
    return this._iconName as Promise<string>;
  }

-  async install(): Promise<boolean> {
+  /**


Uncaught typo from an earlier PR: does't at line 28 of pkg/rancher-desktop/main/extensions/extensions.ts

ericpromislow · 2023-05-12T22:55:37Z

e2e test failures:

On macOS:

  1) extensions.e2e.spec.ts:128:3 › Extensions › build and install testing extension ===============

    Error: /Users/ericp/workspace/rancher/desktop/resources/darwin/bin/docker exited with code 1

       at ../pkg/rancher-desktop/utils/childProcess.ts:283

      281 |         return resolve();
      282 |       }
    > 283 |       reject(new SpawnError([command].concat(finalArgs), {
          |              ^
      284 |         code, signal, ...result,
      285 |       }));
      286 |     });

        at ChildProcess.<anonymous> (/Users/ericp/workspace/rancher/desktop/pkg/rancher-desktop/utils/childProcess.ts:283:14)

  2) rdctl.e2e.spec.ts:146:3 › Command server › should load Kubernetes API =========================

    Error: /Users/ericp/workspace/rancher/desktop/e2e/rdctl.e2e.spec.ts: Unexpected token, expected "," (422:6)

      420 |       ok:     resp.ok,
      421 |       status: resp.status,xz
    > 422 |       body:   resp.body.read().toString(),
          |       ^
      423 |     }).toEqual({
      424 |       ok:     true,
      425 |       status: 202,

ericpromislow · 2023-05-15T18:37:48Z

On a second run, extensions failed at line 131 in test build and install testing extension

Only one log file seems useful, `extensions.e2e.spec.log1:

2023-05-15T18:33:24.138Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> Can't get app version: TypeError: Cannot read properties of undefined (reading 'getVersion')
2023-05-15T18:33:24.361Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> Download the Vue Devtools extension for a better development experience:
https://github.com/vuejs/vue-devtools
2023-05-15T18:33:24.361Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> You are running Vue in development mode.
Make sure to turn on production mode when deploying for production.
See more tips at https://vuejs.org/guide/deployment.html
2023-05-15T18:33:24.370Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> [HMR] connected
2023-05-15T18:33:24.370Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> %c[HMR] bundle 'client' has 1 warnings color: #999933;
2023-05-15T18:33:24.370Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> %c./pkg/rancher-desktop/store/prefs.js
Module not found: Error: Can't resolve '~/assets/brand' in '/Users/ericp/workspace/rancher/desktop/pkg/rancher-desktop/store' color: #999933;
2023-05-15T18:33:24.370Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> console.groupEnd
2023-05-15T18:33:24.464Z: extensions.e2e.spec.ts >> Extensions >> should load backend >> %cElectron Security Warning (Insecure Content-Security-Policy) font-weight: bold; This renderer process has either no Content Security
  Policy set or a policy with "unsafe-eval" enabled. This exposes users of
  this app to unnecessary security risks.

For more information and help, consult
https://electronjs.org/docs/tutorial/security.
This warning will not show up
once the app is packaged.

ericpromislow · 2023-05-15T18:38:46Z

In a nutshell, the extensions e2e test is still flakey, would be better if that could get nailed down.

- Typos - When checking allow list, explicitly check that the extensions are installed / not installed, as desired. - Make sure to add test cases with no, and multiple, items in the allow list. Signed-off-by: Mark Yen <[email protected]>

mook-as

Nothing in your logs are relevant; the only thing is failing to get version (which is unrelated to this PR, and is caught elsewhere).
rdctl.e2e.spec.ts:146:3 is because you had a local change (status: resp.status,xz).

Please attach full logs instead.

(I'm still trying to repro the intermittent failure locally.)

Signed-off-by: Mark Yen <[email protected]>

mook-as · 2023-05-16T18:17:34Z

Eric's logs contains:

extensions.e2e.spec.ts >> Extensions >> extension API >> ddClient.extension.vm.service >> can fetch from the backend >> [verbose] GET /etc/os-release error: Error: Error invoking remote method 'extensions/vm/http-fetch': FetchError: request to http://127.0.0.1:49153/etc/os-release

This is on an Intel mac (so nothing to do with the new network stack on Windows), using the moby backend. I'm mildly concerned that it's at 2GB RAM/2 vCPUs (for the VM), but that shouldn't matter still.

Current best guess is that the container is flapping? Note though this is definitely past build and install testing extension, so that might be a different issue…

ericpromislow · 2023-05-17T19:29:05Z

Bats test failure on Linux:

$ git clean -ffdx && npm i
$ npm run build && npm run package
$ export PATH="$PWD/dist/linux-unpacked/resources/resources/linux/bin:$PATH"
$ bats bats/tests/extensions/allow-list.bats
allow-list.bats
 ✓ factory reset
 ✓ start container engine
 ✓ build extension testing image
 ✓ when no extension allow list is set up, all extensions can install
 ✗ empty allow list disables extension installs
   (from function `assert_failure' in file bats/bats-assert/src/assert_failure.bash, line 66,
    in test file bats/tests/extensions/allow-list.bats, line 79)
     `assert_failure' failed
   no changes necessary
   
   -- command succeeded, but it was expected to fail --
   output : Installing image rd/extension/basic: Created
   --
   
 ✓ when an extension is explicitly allowed, it can be installed
 ✗ when an extension is not in the allowed list, it cannot be installed
   (from function `assert_failure' in file bats/bats-assert/src/assert_failure.bash, line 66,
    in test file bats/tests/extensions/allow-list.bats, line 94)
     `assert_failure' failed
   no changes necessary
   
   -- command succeeded, but it was expected to fail --
   output : Installing image rd/extension/basic: Created
   --
   
 ✓ when no tags given, any tag is allowed
 ✗ when tags are given, only the specified tag is allowed
   (from function `assert_failure' in file bats/bats-assert/src/assert_failure.bash, line 66,
    in test file bats/tests/extensions/allow-list.bats, line 112)
     `assert_failure' failed
   no changes necessary
   
   -- command succeeded, but it was expected to fail --
   output : Installing image rd/extension/basic:0.0.3: Created
   --
   
 ✓ extensions can be allowed by organization
 ✓ extensions can be allowed by repository host

11 tests, 3 failures

ericpromislow · 2023-05-17T19:57:02Z

Comment re code from an earlier commit: The `extensions.ts:metadata` async getter. Yeah, I have a bias against complex async getters, because I think getters should generally wrap a small number of lines, and should be sync. What's the gain in writing it as a getter instead of as a regular async method?

I understand now these are getters because they're implementing an interface. Never mind...

ericpromislow · 2023-05-17T20:13:19Z

pkg/rancher-desktop/main/extensions/types.ts

@@ -169,6 +172,7 @@ export const ExtensionErrorMarker = Symbol('extension-error');
 export enum ExtensionErrorCode {


From a recent commit, @param id at line 112 should be @param image

ericpromislow · 2023-05-17T20:24:38Z

pkg/rancher-desktop/main/extensions/manager.ts

@@ -211,6 +211,8 @@ class ExtensionManagerImpl implements ExtensionManager {



Another question about an older commit: why is the block through which urgency is set at line 158 marked as const when it's a temporary object?

as const just means "don't try to be smart, and derive the types as I wrote it"; in this case, it means that it becomes {success: string, warning: string, error: string}, whereas without the as const it's a Record<string, string>. The latter doesn't work because <thing>[level] could end up being undefined (because it lost the information necessary to detect that <thing>[level], where level is "success" | "warning" | "error", would always have a string value).

ericpromislow · 2023-05-17T20:30:24Z

pkg/rancher-desktop/main/extensions/manager.ts

@@ -221,7 +223,7 @@ class ExtensionManagerImpl implements ExtensionManager {

      tasks.push((async(id: string) => {
        try {
-          return (await this.getExtension(id)).install();
+          return (await this.getExtension(id)).install(allowList);
        } catch (ex) {
          console.error(`Failed to install extension ${ id }`, ex);
        }


Why is await Promise.all(tasks) done just before the end of the surrounding for-block and not after it? I would have expected the code to have this structure:

const tasks: Promise<any>[] = []; for (...) { tasks.push((async(id: string) => { ... })(ARGUMENT); } await Promise.all(tasks);

Instead the code is like this:

const tasks: Promise<any>[] = []; for (...) { tasks.push((async(id: string) => { ... })(ARGUMENT); await Promise.all(tasks); }

Good catch; that is a bug.

ericpromislow · 2023-05-17T20:35:41Z

pkg/rancher-desktop/main/extensions/manager.ts

@@ -221,7 +223,7 @@ class ExtensionManagerImpl implements ExtensionManager {

      tasks.push((async(id: string) => {
        try {
-          return (await this.getExtension(id)).install();
+          return (await this.getExtension(id)).install(allowList);
        } catch (ex) {
          console.error(`Failed to install extension ${ id }`, ex);


Another earlier-commit comment:

FWICT, tag ??= undefined; is there just to get typescript to stop complaining that tag is a let-var and not a const? If so, there should be a comment specifying that. But I would prefer putting this line before the let line:

// eslint-disable-next-line prefer-const

Ah, yeah, that works. (Ideally we could use const but that breaks repo.)

ericpromislow

Found some concerns in the code. Maybe even an actual bug (the scope thing with running all the async tasks).

I don't have anything specific about runtime issues, except that e2e fails repeatedly on Windows and bats failed on Linux. e2e passes on linux and macos (most of the time).

- extensions/types: Fix parameter name - extensions/manager: Wait for tasks _after_ starting all of them - extensions/manager: remove unnecessary assignment to `tag` Signed-off-by: Mark Yen <[email protected]>

mook-as

From your failing BATS run:

 ✗ empty allow list disables extension installs
  …
   no changes necessary

I don't understand why it would no changes necessary when we're updating the allowed list. My (successful) run says:

 ✓ empty allow list disables extension installs
   settings updated; no restart required

This actually happens for all of the tests (that is, none of the tests managed to change the allow list), which is why they are failing. I believe

(Unrelated: please use bats/bats-core/bin/bats rather than whatever bats you have installed.)

mook-as · 2023-05-17T20:47:22Z

pkg/rancher-desktop/main/extensions/manager.ts

@@ -221,7 +223,7 @@ class ExtensionManagerImpl implements ExtensionManager {

      tasks.push((async(id: string) => {
        try {
-          return (await this.getExtension(id)).install();
+          return (await this.getExtension(id)).install(allowList);
        } catch (ex) {
          console.error(`Failed to install extension ${ id }`, ex);
        }


Good catch; that is a bug.

mook-as · 2023-05-17T20:51:05Z

pkg/rancher-desktop/main/extensions/manager.ts

@@ -221,7 +223,7 @@ class ExtensionManagerImpl implements ExtensionManager {

      tasks.push((async(id: string) => {
        try {
-          return (await this.getExtension(id)).install();
+          return (await this.getExtension(id)).install(allowList);
        } catch (ex) {
          console.error(`Failed to install extension ${ id }`, ex);


Ah, yeah, that works. (Ideally we could use const but that breaks repo.)

mook-as · 2023-05-17T21:07:18Z

pkg/rancher-desktop/main/extensions/types.ts

@@ -169,6 +172,7 @@ export const ExtensionErrorMarker = Symbol('extension-error');
 export enum ExtensionErrorCode {


ericpromislow · 2023-05-17T22:52:43Z

My bats is a symlink to ~/workspace/rancher/desktop/bats/bats-core/bin/bats

ericpromislow · 2023-05-17T22:53:34Z

I'll retry the bats with the new changes. Everything else is fine now.

ericpromislow · 2023-05-18T19:50:38Z

Bats tests are still failing the same way on linux but when I run the commands manually they pass. Thought I was using everything in the dist/linux-unpacked subtree.

Bats tests fail differently on macOS now.

Running bats tests:

...
  in test file bats/tests/extensions/allow-list.bats, line 85)
     `rdctl extension install rd/extension/basic:latest' failed
   settings updated; no restart required
   Service Unavailable
   {"message":"503 Service Unavailable"}
 ✓ when an extension is not in the allowed list, it cannot be installed
 ✗ when no tags given, any tag is allowed
   (from function `nerdctl' in file bats/tests/helpers/commands.bash, line 73,
    from function `ctrctl' in file bats/tests/helpers/commands.bash, line 50,
    in test file bats/tests/extensions/allow-list.bats, line 100)
     `ctrctl "${namespace_arg[@]}" tag rd/extension/basic rd/extension/basic:0.0.3' failed
   settings updated; no restart required
   time="2023-05-18T19:41:37Z" level=fatal msg="rd/extension/basic: not found"
   Error: exit status 1

When I try to do this manually:

$ rdctl api /settings | jq .application.extensions
{
  "allowed": {
    "enabled": true,
    "list": [
      "irrelevant/image",
      "rd/extension/basic:latest"
    ]
  }
}

$ rdctl extension install rd/extension/basic:latest
The image rd/extension/basic:latest has invalid extension metadata
{"message":"422 Unprocessable Entity"}

$ cat extensions.log
2023-05-18T19:46:42.271Z: Failed to read metadata for rd/extension/basic: Error: /Applications/Rancher Desktop.app/Contents/Resources/resources/darwin/lima/bin/limactl exited with code 1

jandubois · 2023-05-18T20:17:09Z

I don't know if this has been implemented: every allowed extension must internally also be added to the allowed images list, if allowed image checking has been enabled as well.

Could this explain the different results?

mook-as · 2023-05-18T21:29:26Z

We build all the images locally, so the only thing the allowed images list might do is block downloading of the base image; but in that case, it would be a failure in building the image, not actually using it.

ericpromislow · 2023-05-18T22:22:10Z

Linux bats problems were caused by a rogue docker daemon running on the host. Stopping and uninstalling it solved the bats problems on linux.

ericpromislow

looks good now

mook-as added 2 commits May 11, 2023 12:53

RDX: Add preferences for extension allow list

f82f365

Nothing is hooked up to it yet. Signed-off-by: Mark Yen <[email protected]>

mook-as requested a review from ericpromislow May 11, 2023 20:34

BATS: fix lint issues, determine pref version dynamically

3cccdb1

Signed-off-by: Mark Yen <[email protected]>

ericpromislow reviewed May 12, 2023

View reviewed changes

mook-as linked an issue May 15, 2023 that may be closed by this pull request

Extensions should be integrated with deployment profiles #4341

Closed

9 tasks

RDX: Address review comments

2c1896c

- Typos - When checking allow list, explicitly check that the extensions are installed / not installed, as desired. - Make sure to add test cases with no, and multiple, items in the allow list. Signed-off-by: Mark Yen <[email protected]>

mook-as commented May 15, 2023

View reviewed changes

BATS: fix lint error

8cdcbd9

Signed-off-by: Mark Yen <[email protected]>

gaktive requested review from ericpromislow and jandubois May 17, 2023 18:11

ericpromislow reviewed May 17, 2023

View reviewed changes

ericpromislow requested changes May 17, 2023

View reviewed changes

Address review comments

2a8f22e

- extensions/types: Fix parameter name - extensions/manager: Wait for tasks _after_ starting all of them - extensions/manager: remove unnecessary assignment to `tag` Signed-off-by: Mark Yen <[email protected]>

mook-as commented May 17, 2023

View reviewed changes

mook-as requested a review from ericpromislow May 17, 2023 22:51

ericpromislow approved these changes May 18, 2023

View reviewed changes

mook-as merged commit c485f2b into rancher-sandbox:main May 18, 2023

mook-as deleted the extensions/deployment-profiles branch May 18, 2023 23:00

This was referenced Jun 9, 2023

Extensions are not being installed with deployment profiles and allowed images list #4920

Closed

Automatically allow images for allowed extensions to be pulled #4926

Open

		@@ -169,6 +172,7 @@ export const ExtensionErrorMarker = Symbol('extension-error');
		export enum ExtensionErrorCode {

		@@ -211,6 +211,8 @@ class ExtensionManagerImpl implements ExtensionManager {

RDX: Work with deployment profiles #4655

RDX: Work with deployment profiles #4655

Conversation

mook-as commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow commented May 12, 2023

ericpromislow commented May 15, 2023

ericpromislow commented May 15, 2023

mook-as left a comment

Choose a reason for hiding this comment

mook-as commented May 16, 2023

ericpromislow commented May 17, 2023 • edited Loading

ericpromislow commented May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow left a comment

Choose a reason for hiding this comment

mook-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow commented May 17, 2023

ericpromislow commented May 17, 2023

ericpromislow commented May 18, 2023

jandubois commented May 18, 2023

mook-as commented May 18, 2023

ericpromislow commented May 18, 2023

ericpromislow left a comment

Choose a reason for hiding this comment

ericpromislow commented May 17, 2023 •

edited

Loading

ericpromislow commented May 17, 2023 •

edited

Loading

ericpromislow May 17, 2023 •

edited

Loading