Harden the role editor by falling back to YAML

[bl-nero]

Enables the user to recover by editing role using YAML in case if an unexpected error happens either in rendering or initial role conversion. Also reorganizes error handling to give users more meaningful messages.

Followed up by https://github.com/gravitational/teleport.e/pull/6076.

Tested along with https://github.com/gravitational/teleport.e/pull/6076 in the following scenarios:

• Error thrown by roleToRoleEditorModel()
• Error thrown while rendering the StandardEditor component
• Access graph server unavailable when opening the role editor or in the middle of editing session

In each case, the user can still edit and save the role.

[ryanclark]

Are these the best error messages to surface to the user? HTTP status code and the URL?

[bl-nero]

Are these the best error messages to surface to the user? HTTP status code and the URL?

@ryanclark I bet they are not, and we may refine it further. However, right now, it's only the HTTP status code and the URL, which gives users much less information about the context.

[avatus]

Are these the best error messages to surface to the user? HTTP status code and the URL?

@ryanclark I bet they are not, and we may refine it further. However, right now, it's only the HTTP status code and the URL, which gives users much less information about the context.

im not sure what this means? "what" is only the status code and URL? what you're displaying? i think thats what ryan is saying. or are you saying that somehow thats the only error we get from X backend?

[bl-nero]

OK, so to clarify. Here is our current UX when I kill the access graph server:

And here's what happens if I inject an error into one of the aforementioned places in our frontend code:

[bl-nero]

@avatus And replying to your question: yes, that's all we got from Teleport backend. Just an error code and an empty response body.

[bl-nero]

And I guess it may be more useful if we said something like "502: bad gateway" instead, but this is how our error-handling code works right now; changing it would have global implications and I'd rather do it separately after having a discussion with the UX team.

[ravicious]

I looked at the test only so far, I haven't had the time to look at the code yet, I'll do so tomorrow.

[ravicious]

We shouldn't mock console functions without also validating that we received the expected calls on them. Otherwise we risk introducing unwanted errors like act warnings without realizing it.

https://github.com/ValentinH/jest-fail-on-console/?tab=readme-ov-file#but-i-have-some-expected-console-errorswarning

Patch

Copy then pbpaste | git apply.

diff --git a/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx
index dce371680e..49af6b8475 100644
--- a/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx
+++ b/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx
@@ -401,6 +401,18 @@ test('YAML editor usable even if the standard one throws', async () => {
   expect(onSave).toHaveBeenCalledWith({
     yaml: '{"modified":1}',
   });
+
+  expect(console.error).toHaveBeenCalledTimes(4);
+  expect(console.error).toHaveBeenCalledWith(
+    expect.objectContaining({
+      message: expect.stringMatching('oh noes, it crashed'),
+    })
+  );
+  expect(console.error).toHaveBeenCalledWith(
+    expect.stringMatching(
+      'The above error occurred in the <mockConstructor> component'
+    )
+  );
 });
 
 it('YAML editor usable even if the initial conversion throws', async () => {
@@ -440,6 +452,16 @@ it('YAML editor usable even if the initial conversion throws', async () => {
   expect(onSave).toHaveBeenCalledWith({
     yaml: '{"modified":1}',
   });
+
+  expect(console.error).toHaveBeenCalledTimes(1);
+  expect(console.error).toHaveBeenCalledWith(
+    expect.any(String),
+    expect.any(String),
+    'Could not convert Role to a standard model',
+    expect.objectContaining({
+      message: expect.stringMatching('oh noes, it crashed'),
+    })
+  );
 });
 
 // Here's a trick: since we can't parse YAML back and forth, we use a

[ravicious]

expect.objectContaining({message: …}) is a workaround I've learned some time ago for matching on error messages in Jest.

[bl-nero]

This seems rather fragile. We rely on how exactly the framework reports exceptions here. In the first snippet, where we assert it's been called 4 times, we also rely on the number of render calls. However, we don't even guarantee that nothing else happens; if the exception was thrown just once, and there was another error that originated from act, for example, this test would still pass (4 times, somewhere the expected error happens - we're clear). I'm not sure how to improve on this, though; if you consider making assertions on console messages important, I'd say we probably can leave with how we rely on the framework internals, but I think I'd rather move the assertions to the implementation block so that the test skips expected errors and throws on others.

[ravicious]

but I think I'd rather move the assertions to the implementation block so that the test skips expected errors and throws on others

That would work too. That's what I initially wanted to do, but I couldn't make it work. I think it's because I was doing expect(foo).toEqual(…) from within mockImplementation.

[bl-nero]

OK, after some back and forth, I wasn't able to make a correct assertion on these errors that would be meaningful and not fragile. I'm in favor of not doing it at all.

[bl-nero]

If we care about other unexpected errors that may have been thrown from our component, we still have a lot of other test cases to catch these.

[ravicious]

I agree that the first bunch of expectations is rather fragile, but the second bunch seems to be more sturdy, as there is an expectation placed on a single call to console.error. Would you consider including at least that one?

[ravicious]

Nit, if it was important for handleYamlPreview to have a stable identity between renders before, then it should continue to use useCallback. useAsync itself cannot make the received callback stable.

[bl-nero]

Good catch.

[ravicious]

Just a theoretical question rather than a request because I don't have too much time to dive deep into this, but would it be possible to organize the code in such a way that we don't have to explicitly check for standardModel.roleModel being undefined?

I see it's repeated in a couple of places, I guess the reason behind it is that !standardModel.roleModel?.requiresReset would have different, unwanted semantics.

But it's not something that's done often and I feel like it's going to throw the reader of this code for a spin whenever they encounter one of those checks. So if it'd be possible to simplify it, then it'd only be better.

I suppose changing the underlying object shape might be difficult, but then maybe encapsulating this in a function would work?

[bl-nero]

Hmm. Actually, I was considering replacing the requiresReset field with a function that would return a boolean looking at the number of conversion errors. So I'll do exactly this.

[bl-nero]

Okay, not exactly. There are three places in which I explicitly compare the model to undefined, and in each case, the logic is slightly different. I don' think that extracting it to a function brings us any closer to this being more understandable.

[ravicious]

Nit, but does using @link enriches the output in your editor? Because mine just renders it as {@link RoleDiffProps.roleDiffAttempt} and we don't generate docs from JSDoc anyway, so if it doesn't do much for other editors either, I'm thinking we could not use @link. 😄

I know we do the square brackets thing in Go (e.g. Use [roleDiffAttempt]), but arguably it's less disruptive to the surrounding text than @link.

[bl-nero]

I'm using VS Code, and it supports @link, but doesn't support square brackets. What do you use? If your one supports square brackets, and this is the convention that we use, perhaps it's about plugin configuration.

[ravicious]

I agree that the first bunch of expectations is rather fragile, but the second bunch seems to be more sturdy, as there is an expectation placed on a single call to console.error. Would you consider including at least that one?