Prevent replication subscription to the same region as the current region #16446
Conversation
app/models/pglogical_subscription.rb
Outdated
| @@ -229,6 +230,12 @@ def assert_valid_schemas! | |||
| end | |||
| end | |||
|
|
|||
| def assert_differnet_region! | |||
There was a problem hiding this comment.
typo. "differnet" should read "different"
There was a problem hiding this comment.
😢 I'll fix that. Thanks for finding it!
There was a problem hiding this comment.
Maybe he's diff'ing internet regions. 
| allow(sub).to receive(:remote_region_number).and_return(remote_region1) | ||
| allow(sub).to receive(:ensure_node_created).and_return(true) | ||
|
|
||
| expect { sub.save! }.not_to raise_error |
There was a problem hiding this comment.
The code change, once spelled correctly, looks good. I don't understand all of this stubbing. Maybe that's required because of magic. Some things are just hard to test.
There was a problem hiding this comment.
We can't assume pglogical is installed and the model is an ActsAsArModel so we can't really do a factory...
Some things are just hard to test.
☝️ that 😟
gtanzillo
force-pushed
the
replication-assert-different-region
branch
from
f44389f to
b01b38e
Compare
app/models/pglogical_subscription.rb
Outdated
| @@ -34,6 +34,7 @@ def self.find_by_id(to_find) | |||
|
|
|||
| def save! | |||
| assert_valid_schemas! | |||
| assert_different_region! | |||
There was a problem hiding this comment.
Personally, I think we should check the different region first. No point in checking the schema (more expensive) if the region check fails (less expensive)
There was a problem hiding this comment.
Good point, I'll change.
…gion This change protects a user from accidentally creating a replication subscription to the same region he is in which will result in deleting all the data from the current region. https://bugzilla.redhat.com/show_bug.cgi?id=1507323 Fixed typo1 change 1
…ferent region than the current region https://bugzilla.redhat.com/show_bug.cgi?id=1507323 Fixed typo2 Change 2
gtanzillo
force-pushed
the
replication-assert-different-region
branch
from
b01b38e to
192b14e
Compare
|
Checked commits gtanzillo/manageiq@f173fdc~...192b14e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
…-region Prevent replication subscription to the same region as the current region (cherry picked from commit 91d8041) https://bugzilla.redhat.com/show_bug.cgi?id=1513508
|
Gaprindashvili backport details: |
…-region Prevent replication subscription to the same region as the current region (cherry picked from commit 91d8041) https://bugzilla.redhat.com/show_bug.cgi?id=1513509
|
Fine backport details: |
…ifferent-region Prevent replication subscription to the same region as the current region (cherry picked from commit 91d8041) https://bugzilla.redhat.com/show_bug.cgi?id=1513509
This change prevents a user from shooting himself in the foot by creating a subscription to the same region as the server he's accessing which means creating a subscription to the same DB and region.
The side effect of doing that is, as part of subscription creation, we do a
MiqRegion.destroy_region(connection, remote_region_number). In this case that will destroy the current region. This will prevent that from happeninghttps://bugzilla.redhat.com/show_bug.cgi?id=1507323
/cc @jrafanie