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.

Sign up for GitHub

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

Jump to bottom

fix: UAA delete user endpoint returns false error during upgrade canary deployment #2790

Closed
wants to merge 1 commit into from
Closed

fix: UAA delete user endpoint returns false error during upgrade canary deployment #2790

wants to merge 1 commit into from

Conversation

peterhaochen47
Copy link
Member

[#187240345]

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187263715

The labels on this github issue will be updated when the story is started.

Tallicia
Tallicia approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore looks good

@peterhaochen47 peterhaochen47 force-pushed the pr/develop/fix-canary-deploy-user-delete branch from 0b90aea to dc585a5 Compare March 18, 2024 21:32
@Tallicia Tallicia added the accepted Accepted the issue label Mar 18, 2024
@peterhaochen47 peterhaochen47 force-pushed the pr/develop/fix-canary-deploy-user-delete branch from dc585a5 to d702099 Compare March 18, 2024 21:37
@peterhaochen47
Copy link
Member Author

Some tests are failing on uploading test results:

Run actions/upload-artifact@v4
/usr/bin/docker exec  d78f1e7ca0ca2ef54c3345e3b8c1e9465354a02daefa4d92294c23975801e2f3 sh -c "cat /etc/*release | grep ^ID"
With the provided path, there will be 73 files uploaded
Artifact name is valid!
Root directory input is valid!
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

Not sure what's wrong.

@strehle
Copy link
Member

strehle commented Mar 19, 2024

Not sure what's wrong.

sql syntax for postgresl/mysql is "CREATE TABLE IF NOT EXISTS", corrected it

@peterhaochen47 peterhaochen47 force-pushed the pr/develop/fix-canary-deploy-user-delete branch from 703c849 to 4d4a9c5 Compare March 19, 2024 17:02
@peterhaochen47 @strehle
fix: UAA delete user endpoint returns false error during upgrade cana…
c14fd73 
…ry deployment

- fixes #2789 (see bug root
  cause in the issue)
- by bringing back the MFA-related tables (but without the index
  as these tables won't actually be used, as the MFA feature itself
  has been removed)

[#187240345]

Co-authored-by: Markus Strehle <11627201+strehle@users.noreply.github.com>
@peterhaochen47 peterhaochen47 force-pushed the pr/develop/fix-canary-deploy-user-delete branch from 4d4a9c5 to c14fd73 Compare March 19, 2024 17:03
strehle
strehle reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you have not merged it yesterday for the release I dont see a reason now to have it, correct ?

...rc/main/resources/org/cloudfoundry/identity/uaa/db/postgresql/V4_108__Restore_MFA_Tables.sql
@@ -0,0 +1,29 @@
--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4_108 is now used from recent merge, so if you want this please incr. number

@peterhaochen47
Copy link
Member Author

Since you have not merged it yesterday for the release I dont see a reason now to have it, correct ?

We have not merged it because this is still under discussion in slack, and this PR has not received enough approvals.

@strehle
Copy link
Member

strehle commented Mar 20, 2024

Since you have not merged it yesterday for the release I dont see a reason now to have it, correct ?

We have not merged it because this is still under discussion in slack, and this PR has not received enough approvals.

ok, so you would get an approval, but my personal opinion is, now it is too late and I would not restore tables. UAA was not crashed,.... So too much effort for too less impact

strehle
strehle approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take care for correct versioning of flyway file ... 108 is used alredy.

my personal opinion is, now it is too late and I would not restore tables. UAA was not crashed,.... So too much effort for too less impact

However you can have an approval

@peterhaochen47
Copy link
Member Author

Let's keep this PR as a draft, and wait until next week, then I'll discuss this with @bruce-ricard, who worked on this issue with me.

@peterhaochen47
Copy link
Member Author

I have discussed with @bruce-ricard, and decided that if SAP, the party that reported this issue, does not care about this fix, and given that our side has not received any reports of this issue, we can close this PR for now. If there's any new complaints, we can always reopen this PR.

For now, we have documented this issue in release notes and this open issue: #2789 And we see that as sufficient for now.

@cf-gitbot cf-gitbot added delivered and removed unscheduled accepted Accepted the issue labels Apr 2, 2024
@peterhaochen47 peterhaochen47 mentioned this pull request Apr 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tallicia Tallicia Tallicia approved these changes

@strehle strehle strehle approved these changes

@torsten-sap torsten-sap Awaiting requested review from torsten-sap

Assignees
No one assigned
Labels
delivered
Projects
Foundational Infrastructure Working Group
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

During v76 -> v77 upgrade canary deployment, UAA delete user endpoint returns false error
4 participants
@peterhaochen47 @cf-gitbot @strehle @Tallicia