Prevent upgrade failures if there are existing annotations permissions #5846

nvazquez · 2022-01-10T12:28:41Z

Description

This PR prevents upgrade failures in the specific case in which the annotations are allowed as part of the role permissions for some roles.
Fixes: #5617

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Create a vanilla environment
Upgrade a 4.15.2 environment without adding any role permission

nvazquez · 2022-01-10T12:29:34Z

@blueorangutan package

blueorangutan · 2022-01-10T12:30:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-10T13:31:20Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2163

nvazquez · 2022-01-10T13:36:53Z

@blueorangutan test

blueorangutan · 2022-01-10T13:38:04Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland

cltgm

GutoVeronezi

CLGTM

blueorangutan · 2022-01-10T22:48:37Z

Trillian test result (tid-2843)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31597 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5846-t2843-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

GabrielBrascher

LGTM

rohityadavcloud · 2022-01-11T11:00:06Z

LGTM - would it make more sense to do via java (to check if these exist?)

nvazquez · 2022-01-12T03:45:42Z

@rohityadavcloud sorry I do not see the benefit of moving it to the java code in this case, do you mean moving the SQL check to the java upgrade code or updating the authorized parameter on the @APICommand annotation for the java classes involved?

DaanHoogland · 2022-01-12T10:13:51Z

@nvazquez there is a benefit in what @rohityadavcloud says; we are in a mess with guest OS mappings because of using IDs in schema upgrade scripts. This could happen for roles as well in the long run. So it might be good to device an upgrade mech that is more robust. This will work for sure, but it is not a mechanism that we should promote.

nvazquez · 2022-01-12T13:58:00Z

@blueorangutan package

blueorangutan · 2022-01-12T13:59:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

nvazquez · 2022-01-12T14:01:01Z

@DaanHoogland @rohityadavcloud I see your point, thanks for clarifying. I have updated the PR accordingly, can you please re-review?

Co-authored-by: sureshanaparti <12028987+sureshanaparti@users.noreply.github.com>

nvazquez · 2022-01-13T10:36:00Z

@blueorangutan package

blueorangutan · 2022-01-13T10:37:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-13T11:32:42Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 2194

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41520to41600.java

Co-authored-by: sureshanaparti <12028987+sureshanaparti@users.noreply.github.com>

nvazquez · 2022-01-13T12:14:34Z

@blueorangutan package

blueorangutan · 2022-01-13T12:15:05Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

sureshanaparti

code LGTM

blueorangutan · 2022-01-13T13:33:58Z

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2198

nvazquez · 2022-01-13T14:26:48Z

@blueorangutan package

blueorangutan · 2022-01-13T14:27:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-13T15:15:14Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2200

nvazquez · 2022-01-13T16:01:36Z

@blueorangutan test

blueorangutan · 2022-01-13T16:02:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-01-14T00:51:11Z

Trillian test result (tid-2871)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30370 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5846-t2871-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland

clgtm,
@nvazquez I know you did some upgrade testing, can you describe that, please?

It seems to me we can use a similar mech for the guest os (mapping) upgrades in the future. Do you agree @nvazquez @sureshanaparti ? see #3609, #5685

sureshanaparti · 2022-01-14T13:31:07Z

clgtm, @nvazquez I know you did some upgrade testing, can you describe that, please?

It seems to me we can use a similar mech for the guest os (mapping) upgrades in the future. Do you agree @nvazquez @sureshanaparti ? see #3609, #5685

agree @DaanHoogland

nvazquez · 2022-01-14T14:32:46Z

@DaanHoogland I have added it to the PR description: tested the vanilla environment creation and also upgrade from a 4.15.2 environment in which I didn't add any additional role permission

DaanHoogland · 2022-01-17T08:13:42Z

@nvazquez @sureshanaparti I'd say one more test and we are good to merge:
4.15.2 env with extra roles/permissions defined -> upgrade it.
??

sureshanaparti · 2022-01-17T08:19:45Z

@nvazquez @sureshanaparti I'd say one more test and we are good to merge:
4.15.2 env with extra roles/permissions defined -> upgrade it.
??

@DaanHoogland I'll perform upgrade test with some rules, and update here.

DaanHoogland · 2022-01-17T09:04:11Z

@nvazquez @sureshanaparti I'd say one more test and we are good to merge:
4.15.2 env with extra roles/permissions defined -> upgrade it.
??

@DaanHoogland I'll perform upgrade test with some rules, and update here.

thanks @sureshanaparti

sureshanaparti · 2022-01-18T05:45:59Z

Verified upgrade from CS 4.15.2 with existing annotations permissions. LGTM.

Manually added the following rules to default roles (Resource Admin: 2, Domain Admin: 3, User: 4) in DB, as add/update rules to the default roles is not allowed from 4.15.

INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 2, 'listAnnotations', 'ALLOW');
INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 3, 'addAnnotation', 'DENY');
INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 3, 'removeAnnotation', 'ALLOW');
INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 4, 'listAnnotations', 'ALLOW');
INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 4, 'addAnnotation', 'DENY');
INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission) VALUES (UUID(), 4, 'removeAnnotation', 'DENY');

Upgraded to 4.16, and noticed the missing rules are added. If there are any existing rules, this is the safe mechanism - to check and add the rules while upgrading.

2022-01-17 21:53:59,522 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Generating uuid for existing ssh key-pairs
2022-01-17 21:53:59,523 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Successfully generated uuid for existing ssh key-pairs
2022-01-17 21:53:59,525 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 2
2022-01-17 21:53:59,528 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 2 and rule: listAnnotations
2022-01-17 21:53:59,531 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 2 and rule: listAnnotations, not updating it
2022-01-17 21:53:59,531 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 2 and rule: addAnnotation
2022-01-17 21:53:59,533 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Inserting role permission for role: 2 and rule: addAnnotation
2022-01-17 21:53:59,534 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 2 and rule: removeAnnotation
2022-01-17 21:53:59,534 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Inserting role permission for role: 2 and rule: removeAnnotation
2022-01-17 21:53:59,535 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 3
2022-01-17 21:53:59,535 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 3 and rule: listAnnotations
2022-01-17 21:53:59,535 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Inserting role permission for role: 3 and rule: listAnnotations
2022-01-17 21:53:59,536 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 3 and rule: addAnnotation
2022-01-17 21:53:59,536 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 3 and rule: addAnnotation, not updating it
2022-01-17 21:53:59,536 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 3 and rule: removeAnnotation
2022-01-17 21:53:59,537 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 3 and rule: removeAnnotation, not updating it
2022-01-17 21:53:59,537 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 4
2022-01-17 21:53:59,537 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 4 and rule: listAnnotations
2022-01-17 21:53:59,538 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 4 and rule: listAnnotations, not updating it
2022-01-17 21:53:59,538 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 4 and rule: addAnnotation
2022-01-17 21:53:59,538 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 4 and rule: addAnnotation, not updating it
2022-01-17 21:53:59,538 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Checking the annotation permissions for the role: 4 and rule: removeAnnotation
2022-01-17 21:53:59,539 DEBUG [c.c.u.d.Upgrade41520to41600] (main:null) (logid:) Found existing role permission for role: 4 and rule: removeAnnotation, not updating it
2022-01-17 21:53:59,553 INFO  [c.c.u.DatabaseUpgradeChecker] (main:null) (logid:) Cleanup upgrade Upgrade41520to41600 to upgrade from 4.15.2.0-4.16.0.0 to 4.16.0.0

Do not fail if there are existing role permissions for annotations

554fc05

nvazquez added type:bug component:upgrade component:database labels Jan 10, 2022

nvazquez added this to the 4.16.1.0 milestone Jan 10, 2022

nvazquez requested review from rohityadavcloud, vladimirpetrov and borisstoyanov January 10, 2022 12:28

nvazquez self-assigned this Jan 10, 2022

DaanHoogland approved these changes Jan 10, 2022

View reviewed changes

GutoVeronezi approved these changes Jan 10, 2022

View reviewed changes

sureshanaparti self-assigned this Jan 11, 2022

GabrielBrascher approved these changes Jan 11, 2022

View reviewed changes

rohityadavcloud approved these changes Jan 11, 2022

View reviewed changes

Refactor

6385e3f

Improve refactor

80594cb

nvazquez requested review from rohityadavcloud and DaanHoogland January 12, 2022 14:36

nvazquez and others added 2 commits January 13, 2022 07:33

Apply suggestions from code review

9a1a715

Co-authored-by: sureshanaparti <12028987+sureshanaparti@users.noreply.github.com>

Remove extra space from suggestion

92c1f76

sureshanaparti reviewed Jan 13, 2022

View reviewed changes

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41520to41600.java Outdated Show resolved Hide resolved

sureshanaparti reviewed Jan 13, 2022

View reviewed changes

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41520to41600.java Outdated Show resolved Hide resolved

Apply suggestions from code review

8803c19

Co-authored-by: sureshanaparti <12028987+sureshanaparti@users.noreply.github.com>

sureshanaparti approved these changes Jan 13, 2022

View reviewed changes

sureshanaparti added the status:needs-testing label Jan 14, 2022

DaanHoogland approved these changes Jan 14, 2022

View reviewed changes

sureshanaparti removed the status:needs-testing label Jan 18, 2022

sureshanaparti merged commit e18ff60 into apache:4.16 Jan 18, 2022

sureshanaparti mentioned this pull request Jan 18, 2022

Possible upgrade issue when rules exist #5617

Closed

nvazquez deleted the fixannotationspermissions branch February 12, 2022 02:37

Prevent upgrade failures if there are existing annotations permissions #5846

Prevent upgrade failures if there are existing annotations permissions #5846

Conversation

nvazquez commented Jan 10, 2022 • edited Loading

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

nvazquez commented Jan 10, 2022

blueorangutan commented Jan 10, 2022

blueorangutan commented Jan 10, 2022

nvazquez commented Jan 10, 2022

blueorangutan commented Jan 10, 2022

DaanHoogland left a comment

Choose a reason for hiding this comment

GutoVeronezi left a comment

Choose a reason for hiding this comment

blueorangutan commented Jan 10, 2022

GabrielBrascher left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Jan 11, 2022

nvazquez commented Jan 12, 2022 • edited Loading

DaanHoogland commented Jan 12, 2022

nvazquez commented Jan 12, 2022

blueorangutan commented Jan 12, 2022

nvazquez commented Jan 12, 2022

nvazquez commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

nvazquez commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

sureshanaparti left a comment

Choose a reason for hiding this comment

blueorangutan commented Jan 13, 2022

nvazquez commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

nvazquez commented Jan 13, 2022

blueorangutan commented Jan 13, 2022

blueorangutan commented Jan 14, 2022

DaanHoogland left a comment

Choose a reason for hiding this comment

sureshanaparti commented Jan 14, 2022

nvazquez commented Jan 14, 2022

DaanHoogland commented Jan 17, 2022

sureshanaparti commented Jan 17, 2022

DaanHoogland commented Jan 17, 2022

sureshanaparti commented Jan 18, 2022

nvazquez commented Jan 10, 2022 •

edited

Loading

nvazquez commented Jan 12, 2022 •

edited

Loading