fix: Fix repeated 403 due to app namespace being undefined (#20699) #20819

andrii-korotkov-verkada · 2024-11-18T05:16:14Z

Fixes #20699

Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use this.props.match.params.appnamespace could also fail if it's undefined. As a fix, create and use a helper function getAppNamespace which has a special case handling for undefined.

Also, use a namespaced endpoint when namespace is not undefined.

It needs to be cherry-picked to v2.11-2.13.

Checklist:

bunnyshell · 2024-11-18T05:16:21Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

andrii-korotkov-verkada · 2024-11-18T17:09:14Z

✅ Preview Environment deployed on Bunnyshell

Component Endpoints
argocd https://argocd-m3vpi2.bunnyenv.com/
argocd-ttyd https://argocd-web-cli-m3vpi2.bunnyenv.com/

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):
* 🔴 `/bns:stop` to stop the environment

* 🚀 `/bns:deploy` to redeploy the environment

* ❌ `/bns:delete` to remove the environment

/bns:deploy

crenshaw-dev · 2024-11-18T17:17:19Z

✅ Preview Environment deployed on Bunnyshell

Component Endpoints
argocd https://argocd-m3vpi2.bunnyenv.com/
argocd-ttyd https://argocd-web-cli-m3vpi2.bunnyenv.com/
See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔴 /bns:stop to stop the environment

🚀 /bns:deploy to redeploy the environment

❌ /bns:delete to remove the environment

/bns:deploy

andrii-korotkov-verkada · 2024-11-18T17:41:33Z

I've checked Bunnyshell and didn't get any 403 while inspecting network stuff. The resource tree endpoint returns 200.

pasha-codefresh · 2024-11-18T19:20:44Z

Asked UI team to help with review. Thanks Andrii for fixing it, once we cherry-pick it , i will create release

andrii-korotkov-verkada · 2024-11-18T20:16:48Z

@pasha-codefresh, let's also include argoproj/gitops-engine#640 and follow-up PR to use it. That's another nasty bug that should be fixed in 2.13.1

ui/src/app/applications/components/applications-list/applications-table.tsx

ui/src/app/applications/components/applications-list/applications-tiles.tsx

ui/src/app/applications/components/application-details/application-details-app-dropdown.tsx

ui/src/app/applications/components/application-details/application-details.tsx

ui/src/app/applications/components/applications-list/applications-table.tsx

andrii-korotkov-verkada · 2024-11-19T12:07:42Z

@oleksandr-codefresh, @plakyda-codefresh, I've addressed the feedback. Thanks!

andrii-korotkov-verkada · 2024-11-19T12:41:52Z

Hm, jest is unhappy with using lodash-es.
Also, it uses

function isUndefined(value) {
  return value === undefined;
}

which may not work in older browsers. I'll switch back to using a custom implementation.

pasha-codefresh · 2024-11-19T12:58:26Z

/bns:deploy

oleksandr-codefresh · 2024-11-19T13:16:24Z

@andrii-korotkov-verkada still, create dedicated utils function isUndefined to avoid duplication

andrii-korotkov-verkada · 2024-11-19T13:37:32Z

But it's so simple, that it'd be more code to import a helper function and use it comparing to just check inline.

andrii-korotkov-verkada · 2024-11-19T13:38:03Z

@pasha-codefresh, the bns commands on PR don't seem to do anything. And it seems not possible to comment on comment.

ui/src/app/applications/components/utils.tsx

…20699) Fixes argoproj#20699 Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use `this.props.match.params.appnamespace` could also fail if it's undefined. As a fix, create and use a helper function `getAppNamespace` which has a special case handling for undefined. Also, use a namespaced endpoint when namespace is not undefined. It needs to be cherry-picked to v2.11-2.13. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>

pasha-codefresh · 2024-11-19T14:45:17Z

@andrii-korotkov-verkada let me know once it is ready to review, i will run it locally and see if it works

andrii-korotkov-verkada · 2024-11-19T17:51:45Z

@pasha-codefresh, it should be ready to review.

pasha-codefresh · 2024-11-19T23:17:14Z

/bns:deploy

pasha-codefresh · 2024-11-19T23:20:00Z

✅ Preview Environment deployed on Bunnyshell

Component Endpoints
argocd https://argocd-m3vpi2.bunnyenv.com/
argocd-ttyd https://argocd-web-cli-m3vpi2.bunnyenv.com/
See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔴 /bns:stop to stop the environment

🚀 /bns:deploy to redeploy the environment

❌ /bns:delete to remove the environment

/bns:deploy

pasha-codefresh · 2024-11-20T10:54:24Z

@andrii-korotkov-verkada i am fine with this fix, but any reason why appNamespace should be undefined when you use search bar? i think we also should fix this in order to make it consistent. I can take a look into it

pasha-codefresh · 2024-11-20T10:57:31Z

/cherry-pick release-2.13

…20819) Fixes #20699 Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use `this.props.match.params.appnamespace` could also fail if it's undefined. As a fix, create and use a helper function `getAppNamespace` which has a special case handling for undefined. Also, use a namespaced endpoint when namespace is not undefined. It needs to be cherry-picked to v2.11-2.13. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>

andrii-korotkov-verkada · 2024-11-20T11:04:22Z

@pasha-codefresh, I haven't figured that out.

pasha-codefresh · 2024-11-20T11:05:31Z

I will check it

…20819) (#20860) Fixes #20699 Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use `this.props.match.params.appnamespace` could also fail if it's undefined. As a fix, create and use a helper function `getAppNamespace` which has a special case handling for undefined. Also, use a namespaced endpoint when namespace is not undefined. It needs to be cherry-picked to v2.11-2.13. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> Co-authored-by: Andrii Korotkov <137232734+andrii-korotkov-verkada@users.noreply.github.com>

…20699) (argoproj#20819) Fixes argoproj#20699 Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use `this.props.match.params.appnamespace` could also fail if it's undefined. As a fix, create and use a helper function `getAppNamespace` which has a special case handling for undefined. Also, use a namespaced endpoint when namespace is not undefined. It needs to be cherry-picked to v2.11-2.13. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>

…20699) (argoproj#20819) Fixes argoproj#20699 Constructor may not get called every time the application changes, so previous this.appNamespace could be stale. But the update to use `this.props.match.params.appnamespace` could also fail if it's undefined. As a fix, create and use a helper function `getAppNamespace` which has a special case handling for undefined. Also, use a namespaced endpoint when namespace is not undefined. It needs to be cherry-picked to v2.11-2.13. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> Signed-off-by: Adrian Aneci <aneci@adobe.com>

andrii-korotkov-verkada requested a review from a team as a code owner November 18, 2024 05:16

andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Nov 18, 2024

andrii-korotkov-verkada removed the ready-for-review An approver should give a final review and merge the PR label Nov 18, 2024

andrii-korotkov-verkada force-pushed the 20699-fix-repeated-403-due-to-app-namespace-undefined branch from 7f93899 to 99c2819 Compare November 18, 2024 05:20

andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Nov 18, 2024

andrii-korotkov-verkada force-pushed the 20699-fix-repeated-403-due-to-app-namespace-undefined branch 4 times, most recently from d9d7110 to 2c04134 Compare November 18, 2024 16:02