Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

andrii-korotkov-verkada
Copy link
Contributor

@andrii-korotkov-verkada andrii-korotkov-verkada commented Nov 18, 2024

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:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Copy link

bunnyshell bot commented Nov 18, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@andrii-korotkov-verkada 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 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
@andrii-korotkov-verkada
Copy link
Contributor Author

✅ 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
Copy link
Member

✅ 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
Copy link
Contributor Author

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

@pasha-codefresh
Copy link
Member

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

@andrii-korotkov-verkada
Copy link
Contributor Author

@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

@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 20699-fix-repeated-403-due-to-app-namespace-undefined branch from 2c04134 to 99152d2 Compare November 19, 2024 12:06
@andrii-korotkov-verkada
Copy link
Contributor Author

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

@andrii-korotkov-verkada
Copy link
Contributor Author

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.

@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 20699-fix-repeated-403-due-to-app-namespace-undefined branch 2 times, most recently from 2619e38 to 4b7784a Compare November 19, 2024 12:49
@pasha-codefresh
Copy link
Member

/bns:deploy

@oleksandr-codefresh
Copy link
Contributor

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

@andrii-korotkov-verkada
Copy link
Contributor Author

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
Copy link
Contributor Author

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

…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 <[email protected]>
@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 20699-fix-repeated-403-due-to-app-namespace-undefined branch from 4b7784a to 5bab9fa Compare November 19, 2024 13:45
@pasha-codefresh
Copy link
Member

@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
Copy link
Contributor Author

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

@pasha-codefresh
Copy link
Member

/bns:deploy

@pasha-codefresh
Copy link
Member

✅ 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
Copy link
Member

@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 pasha-codefresh merged commit 3da5a3d into argoproj:master Nov 20, 2024
23 checks passed
@pasha-codefresh
Copy link
Member

/cherry-pick release-2.13

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Nov 20, 2024
…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 <[email protected]>
@andrii-korotkov-verkada
Copy link
Contributor Author

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

@pasha-codefresh
Copy link
Member

I will check it

pasha-codefresh pushed a commit that referenced this pull request Nov 20, 2024
…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 <[email protected]>
Co-authored-by: Andrii Korotkov <[email protected]>
revitalbarletz pushed a commit to revitalbarletz/argo-cd that referenced this pull request Nov 21, 2024
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

appNamespace=undefined
5 participants