fix: Fix repeated 403 due to app namespace being undefined (#20699) #20819
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
andrii-korotkov-verkada
force-pushed
the
20699-fix-repeated-403-due-to-app-namespace-undefined
branch
from
7f93899
to
99c2819
Compare
andrii-korotkov-verkada
force-pushed
the
20699-fix-repeated-403-due-to-app-namespace-undefined
branch
3 times, most recently
from
163d59e
to
d9d7110
Compare
…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>
andrii-korotkov-verkada
force-pushed
the
20699-fix-repeated-403-due-to-app-namespace-undefined
branch
from
d9d7110
to
2c04134
Compare
/bns:deploy |
/bns:deploy |
|
I've checked Bunnyshell and didn't get any 403 while inspecting network stuff. The resource tree endpoint returns 200. |
|
Asked UI team to help with review. Thanks Andrii for fixing it, once we cherry-pick it , i will create release |
|
@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 |
| onClick={e => | ||
| ctx.navigation.goto( | ||
| typeof app.metadata.namespace !== 'undefined' | ||
| ? `/applications/${app.metadata.namespace}/${app.metadata.name}` |
There was a problem hiding this comment.
I see these links in two places, so it's better to move this logic into dedicated function
| let targetUrl; | ||
| // Use not namespaced endpoint if namespace is undefined | ||
| if (typeof applications[selectedApp].metadata.namespace !== 'undefined') { | ||
| targetUrl = `/applications/${applications[selectedApp].metadata.namespace}/${applications[selectedApp].metadata.name}`; |
There was a problem hiding this comment.
re-use dedicated function for that
| ctx.navigation.goto(`applications/${app.metadata.namespace}/${app.metadata.name}`, {view: pref.appDetails.view}, {event: e}) | ||
| ctx.navigation.goto( | ||
| typeof app.metadata.namespace !== 'undefined' | ||
| ? `/applications/${app.metadata.namespace}/${app.metadata.name}` |
There was a problem hiding this comment.
re-use dedicated function for that
| onClick={() => | ||
| ctx.navigation.goto( | ||
| // Use not namespaced endpoint if namespace is undefined | ||
| typeof app.metadata.namespace !== 'undefined' |
There was a problem hiding this comment.
use isUndefined from lodash
| private getAppNamespace() { | ||
| if (typeof this.props.match.params.appnamespace === 'undefined') { | ||
| return ''; | ||
| } else { |
There was a problem hiding this comment.
no need to wrap it it else {}
just return
| @@ -116,6 +110,14 @@ export class ApplicationDetails extends React.Component<RouteComponentProps<{app | |||
| services.extensions.removeEventListener('topBar', this.onExtensionsUpdate); | |||
| } | |||
|
|
|||
| private getAppNamespace() { | |||
| if (typeof this.props.match.params.appnamespace === 'undefined') { | |||
There was a problem hiding this comment.
use isUndefined from lodash in all places
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.appnamespacecould also fail if it's undefined. As a fix, create and use a helper functiongetAppNamespacewhich 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: