WebDiscover: allow setting labels when enrolling aws app

[kimlisa]

part of #46976

requires #50975

with some labels

changelog: Adds support for defining labels in the web UI for AWS console access application

[avatus]

i think the new UI should always use the new endpoint regardless if labels exist or not. The only time an "old" endpoint should be used is if we want some sort of graceful fallback in some feature which we don't care about here.

go all in!

[kimlisa]

i might be just confused, but i added the fallback so that the user has the option to proceed without the labels if they keep hitting the old proxy?

[avatus]

Thats right. But this block of code reads like it will hit v1 if they didnt add any labels. I think they should always hit v2 even without labels

[kimlisa]

But this block of code reads like it will hit v1 if they didnt add any labels.

i think that's okay, since either endpoint in new proxy uses the same handler

I think they should always hit v2 even without labels

hrm, but what if they hit an old proxy? get an error, and they get stuck? (the fallback is then to just remove labels and it will use the old endpoint in case it hits the old proxy again)

[avatus]

i mean they should always hit v2 as the happy path, even if they dont want to add labels. they are optional, so they should always go v2 since v2 should be able to handle it.
if the web hits the 404, they can be given the option to fallback to v1, sure. but we should never be hitting v1 if we havent already got the 404

[kimlisa]

ah i think i finally understand the problem. calling v1 without labels will have issues once we remove the old endpoint, great catch 👍

[flyinghermit]

Make V2 the first API call the default one here as well? And looks like we can remove the const [requiresProxyUpgrade, setRequiresProxyUpgrade] = useState(false); state.

const [attempt, createApp] = useAsync(async () => {
    const labelsMap: Record<string, string> = {};
    labels.forEach(l => (labelsMap[l.name] = l.value));
    let app: App;
    try {
      app = await integrationService.createAwsAppAccessV2(
        awsIntegration.name,
        { labels: labelsMap }
      );
      }
    } catch (err) {

      if (err.message.includes(ProxyRequiresUpgrade)) {
      try {
        // retry to V1 endpoint
        app = await integrationService.createAwsAppAccess(awsIntegration.name);
      } catch (e) {
        emitErrorEvent(err.message);
        throw err;
      }
      emitErrorEvent(err.message);
      throw err;
    }
    updateAgentMeta({
      ...agentMeta,
      app,
      resourceName: app.name,
    });
  });

we can abstract v1 async call so catch(err) block is a bit simpler.

[kimlisa]

hrmmm, wouldn't this re-try for the user? i think it's better if user intentionally retries (by removing labels, which means they understand labels aren't going to be added) or abort altogether

[flyinghermit]

You are right this auto retries. Also letting user manually do that is a good idea.
I didn't see any label/copy updated for user based on requiresProxyUpgrade so recommended that alternative.
How would the retry UX look like?

[kimlisa]

here is a recording of retry (the error message is there after the first attempt that seemed to have cut off from recording):

Screen.Recording.2025-01-13.at.10.09.52.PM.mov

I didn't see any label/copy updated for user based on requiresProxyUpgrade so recommended that alternative.

unless you feel strongly, i will leave it as is. i think it makes things too complicated

[flyinghermit]

Gotcha. That looks good to me. The only extra thing I could suggest is to mark the labels fields as error so it becomes more apparent but i wouldn't stress that if it begs more changes. Its good as is too.

[public-teleport-github-review-bot]

@kimlisa See the table below for backport results.

Branch	Result
branch/v17	Failed