Commit 2a552e55 authored by Nick Thomas's avatar Nick Thomas

Handle Geo node URL parse errors intelligently

By storing the URL unmodified in the database, parsing it can happen at
validation time, rather than during data entry. This allows problems to be
reported to the end-user via the errors hash, rather than an exception.
parent 891acf9d
......@@ -8,19 +8,17 @@ class GeoNode < ActiveRecord::Base
has_many :namespaces, through: :geo_node_namespace_links
has_one :status, class_name: 'GeoNodeStatus'
default_values schema: lambda { Gitlab.config.gitlab.protocol },
host: lambda { Gitlab.config.gitlab.host },
port: lambda { Gitlab.config.gitlab.port },
relative_url_root: lambda { Gitlab.config.gitlab.relative_url_root },
default_values url: ->(record) { record.class.current_node_url },
primary: false,
clone_protocol: 'http'
accepts_nested_attributes_for :geo_node_key
validates :host, host: true, presence: true, uniqueness: { case_sensitive: false, scope: :port }
validates :url, presence: true, uniqueness: { case_sensitive: false }
validate :check_url_is_valid
validates :primary, uniqueness: { message: 'node already exists' }, if: :primary
validates :schema, inclusion: %w(http https)
validates :relative_url_root, length: { minimum: 0, allow_nil: false }
validates :access_key, presence: true
validates :encrypted_secret_access_key, presence: true
validates :clone_protocol, presence: true, inclusion: %w(ssh http)
......@@ -35,16 +33,33 @@ class GeoNode < ActiveRecord::Base
before_validation :ensure_access_keys!
scope :with_url_prefix, ->(prefix) { where('url LIKE ?', "#{prefix}%") }
attr_encrypted :secret_access_key,
key: Gitlab::Application.secrets.db_key_base,
algorithm: 'aes-256-gcm',
mode: :per_attribute_iv,
encode: true
class << self
def current_node_url
RequestStore.fetch('geo_node:current_node_url') do
cfg = Gitlab.config.gitlab
uri = URI.parse("#{cfg.protocol}://#{cfg.host}:#{cfg.port}#{cfg.relative_url_root}")
uri.path += '/' unless uri.path.end_with?('/')
uri.to_s
end
end
def current_node
GeoNode.find_by(url: current_node_url)
end
end
def current?
host == Gitlab.config.gitlab.host &&
port == Gitlab.config.gitlab.port &&
relative_url_root == Gitlab.config.gitlab.relative_url_root
self.class.current_node_url == url
end
def secondary?
......@@ -55,24 +70,23 @@ class GeoNode < ActiveRecord::Base
secondary? && clone_protocol == 'ssh'
end
def uri
if relative_url_root
relative_url = relative_url_root.starts_with?('/') ? relative_url_root : "/#{relative_url_root}"
end
def url
value = read_attribute(:url)
value += '/' if value.present? && !value.end_with?('/')
URI.parse(URI::Generic.build(scheme: schema, host: host, port: port, path: relative_url).normalize.to_s)
value
end
def url
uri.to_s
def url=(value)
value += '/' if value.present? && !value.end_with?('/')
write_attribute(:url, value)
@uri = nil
end
def url=(new_url)
new_uri = URI.parse(new_url)
self.schema = new_uri.scheme
self.host = new_uri.host
self.port = new_uri.port
self.relative_url_root = new_uri.path != '/' ? new_uri.path : ''
def uri
@uri ||= URI.parse(url) if url.present?
end
def geo_transfers_url(file_type, file_id)
......@@ -203,7 +217,7 @@ class GeoNode < ActiveRecord::Base
private
def geo_api_url(suffix)
URI.join(uri, "#{uri.path}/", "api/#{API::API.version}/geo/#{suffix}").to_s
URI.join(uri, "#{uri.path}", "api/#{API::API.version}/geo/#{suffix}").to_s
end
def ensure_access_keys!
......@@ -216,11 +230,7 @@ class GeoNode < ActiveRecord::Base
end
def url_helper_args
if relative_url_root
relative_url = relative_url_root.starts_with?('/') ? relative_url_root : "/#{relative_url_root}"
end
{ protocol: schema, host: host, port: port, script_name: relative_url }
{ protocol: uri.scheme, host: uri.host, port: uri.port, script_name: uri.path }
end
def build_dependents
......@@ -246,13 +256,19 @@ class GeoNode < ActiveRecord::Base
# Prevent locking yourself out
def check_not_adding_primary_as_secondary
if host == Gitlab.config.gitlab.host &&
port == Gitlab.config.gitlab.port &&
relative_url_root == Gitlab.config.gitlab.relative_url_root
if url == self.class.current_node_url
errors.add(:base, 'Current node must be the primary node or you will be locking yourself out')
end
end
def check_url_is_valid
if uri.present? && !%w[http https].include?(uri.scheme)
errors.add(:url, 'scheme must be http or https')
end
rescue URI::InvalidURIError
errors.add(:url, 'is invalid')
end
def update_clone_url
self.clone_url_prefix = Gitlab.config.gitlab_shell.ssh_path_prefix
end
......
class StoreGeoNodesUrlDirectly < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
class GeoNode < ActiveRecord::Base
def compute_unified_url!
uri = URI.parse("#{schema}://#{host}#{relative_url_root}")
uri.port = port if port.present?
uri.path += '/' unless uri.path.end_with?('/')
update!(url: uri.to_s)
end
def compute_split_url!
uri = URI.parse(url)
update!(
schema: uri.scheme,
host: uri.host,
port: uri.port,
relative_url_root: uri.path
)
end
end
def up
add_column :geo_nodes, :url, :string
GeoNode.find_each { |node| node.compute_unified_url! }
change_column_null(:geo_nodes, :url, false)
end
def down
GeoNode.find_each { |node| node.compute_split_url! }
remove_column :geo_nodes, :url, :string
end
end
class IndexGeoNodesUrl < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :geo_nodes, :url, unique: true
end
def down
remove_concurrent_index :geo_nodes, :url, unique: true
end
end
class RemoveGeoNodesUrlPartColumns < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
remove_column :geo_nodes, :schema, :string
remove_column :geo_nodes, :host, :string
remove_column :geo_nodes, :port, :integer
remove_column :geo_nodes, :relative_url_root, :string
end
def down
add_column :geo_nodes, :schema, :string
add_column :geo_nodes, :host, :string
add_column :geo_nodes, :port, :integer
add_column :geo_nodes, :relative_url_root, :string
add_concurrent_index :geo_nodes, :host
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171124070437) do
ActiveRecord::Schema.define(version: 20171124165823) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -959,10 +959,6 @@ ActiveRecord::Schema.define(version: 20171124070437) do
add_index "geo_node_statuses", ["geo_node_id"], name: "index_geo_node_statuses_on_geo_node_id", unique: true, using: :btree
create_table "geo_nodes", force: :cascade do |t|
t.string "schema"
t.string "host"
t.integer "port"
t.string "relative_url_root"
t.boolean "primary"
t.integer "geo_node_key_id"
t.integer "oauth_application_id"
......@@ -974,11 +970,12 @@ ActiveRecord::Schema.define(version: 20171124070437) do
t.integer "files_max_capacity", default: 10, null: false
t.integer "repos_max_capacity", default: 25, null: false
t.string "clone_protocol", default: "http", null: false
t.string "url", null: false
end
add_index "geo_nodes", ["access_key"], name: "index_geo_nodes_on_access_key", using: :btree
add_index "geo_nodes", ["host"], name: "index_geo_nodes_on_host", using: :btree
add_index "geo_nodes", ["primary"], name: "index_geo_nodes_on_primary", using: :btree
add_index "geo_nodes", ["url"], name: "index_geo_nodes_on_url", unique: true, using: :btree
create_table "geo_repositories_changed_events", id: :bigserial, force: :cascade do |t|
t.integer "geo_node_id", null: false
......
......@@ -110,7 +110,7 @@ class Admin::GeoNodesController < Admin::ApplicationController
end
def has_insecure_nodes?
GeoNode.where(schema: 'http').any?
GeoNode.with_url_prefix('http://').exists?
end
def flash_now(type, message)
......
......@@ -39,8 +39,14 @@ module EE
def redirect_allowed_to?(uri)
raise NotImplementedError unless defined?(super)
# Redirect is not only allowed to current host, but also to other Geo nodes
super || ::Gitlab::Geo.geo_node?(host: uri.host, port: uri.port)
# Redirect is not only allowed to current host, but also to other Geo
# nodes. relative_url_root *must* be ignored here as we don't know what
# is root and what is path
super || begin
truncated = uri.dup.tap { |uri| uri.path = '/' }
::GeoNode.with_url_prefix(truncated).exists?
end
end
end
end
......@@ -18,11 +18,7 @@ module Gitlab
FDW_SCHEMA = 'gitlab_secondary'.freeze
def self.current_node
self.cache_value(:geo_node_current) do
GeoNode.find_by(host: Gitlab.config.gitlab.host,
port: Gitlab.config.gitlab.port,
relative_url_root: Gitlab.config.gitlab.relative_url_root)
end
self.cache_value(:geo_node_current) { GeoNode.current_node }
end
def self.primary_node
......@@ -72,10 +68,6 @@ module Gitlab
::License.feature_available?(:geo)
end
def self.geo_node?(host:, port:)
GeoNode.where(host: host, port: port).exists?
end
def self.fdw?
self.cache_value(:geo_fdw?) do
::Geo::BaseRegistry.connection.execute(
......
......@@ -175,15 +175,7 @@ namespace :geo do
end
def set_primary_geo_node
params = {
schema: Gitlab.config.gitlab.protocol,
host: Gitlab.config.gitlab.host,
port: Gitlab.config.gitlab.port,
relative_url_root: Gitlab.config.gitlab.relative_url_root,
primary: true
}
node = GeoNode.new(params)
node = GeoNode.new(primary: true, url: GeoNode.current_node_url)
puts "Saving primary GeoNode with URL #{node.url}".color(:green)
node.save
......
......@@ -249,7 +249,7 @@ describe Admin::GeoNodesController, :postgresql do
end
context 'with a secondary node' do
let(:geo_node) { create(:geo_node, host: 'example.com', port: 80, enabled: true) }
let(:geo_node) { create(:geo_node, url: 'http://example.com') }
context 'when succeed' do
before do
......
......@@ -2,16 +2,19 @@ require 'rails_helper'
feature 'Geo clone instructions', :js do
include Devise::Test::IntegrationHelpers
include ::EE::GeoHelpers
let(:project) { create(:project, :empty_repo) }
let(:developer) { create(:user) }
background do
primary = create(:geo_node, :primary, schema: 'https', host: 'primary.domain.com', port: 443)
primary.update_attribute(:clone_url_prefix, 'git@primary.domain.com:')
allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
primary = create(:geo_node, :primary, url: 'https://primary.domain.com')
primary.update_columns(clone_url_prefix: 'git@primary.domain.com:')
secondary = create(:geo_node)
project.team << [developer, :developer]
stub_current_geo_node(secondary)
project.add_developer(developer)
sign_in(developer)
end
......
......@@ -32,7 +32,6 @@ describe EE::GitlabRoutingHelper do
context 'HTTP' do
before do
allow(helper).to receive(:default_clone_protocol).and_return('http')
primary.update!(schema: 'http')
end
context 'project' do
......@@ -51,7 +50,7 @@ describe EE::GitlabRoutingHelper do
context 'HTTPS' do
before do
allow(helper).to receive(:default_clone_protocol).and_return('https')
primary.update!(schema: 'https')
primary.update!(url: 'https://localhost:123/relative')
end
context 'project' do
......
......@@ -7,9 +7,17 @@ describe RemoveSystemHookFromGeoNodes, :migration do
before do
allow_any_instance_of(WebHookService).to receive(:execute)
node_attrs = {
schema: 'http',
host: 'localhost',
port: 3000
}
create(:system_hook)
geo_nodes.create! attributes_for(:geo_node, :primary)
geo_nodes.create! attributes_for(:geo_node, system_hook_id: create(:system_hook).id)
hook_id = create(:system_hook).id
geo_nodes.create!(node_attrs.merge(primary: true))
geo_nodes.create!(node_attrs.merge(system_hook_id: hook_id, port: 3001))
end
it 'destroy all system hooks for secondary nodes' do
......
FactoryGirl.define do
factory :geo_node do
host { Gitlab.config.gitlab.host }
sequence(:port) {|n| n}
sequence(:url) do |port|
uri = URI.parse("http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.relative_url_root}")
uri.port = port
uri.path += '/' unless uri.path.end_with?('/')
uri.to_s
end
trait :ssh do
clone_protocol 'ssh'
......@@ -10,7 +15,13 @@ FactoryGirl.define do
trait :primary do
primary true
port { Gitlab.config.gitlab.port }
url do
uri = URI.parse("http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.relative_url_root}")
uri.port = Gitlab.config.gitlab.port
uri.path += '/' unless uri.path.end_with?('/')
uri.to_s
end
end
end
end
......@@ -149,16 +149,6 @@ describe Gitlab::Geo, :geo do
end
end
describe 'geo_node?' do
it 'returns true if a node with specific host and port exists' do
expect(described_class.geo_node?(host: primary_node.host, port: primary_node.port)).to be_truthy
end
it 'returns false if specified host and port doesnt match any existing node' do
expect(described_class.geo_node?(host: 'inexistent', port: 1234)).to be_falsey
end
end
describe 'license_allows?' do
it 'returns true if license has Geo addon' do
stub_licensed_features(geo: true)
......
......@@ -4,8 +4,8 @@ describe GeoNode, type: :model do
using RSpec::Parameterized::TableSyntax
include ::EE::GeoHelpers
let(:new_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
let(:new_primary_node) { create(:geo_node, :primary, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
let(:new_node) { create(:geo_node, url: 'https://localhost:3000/gitlab') }
let(:new_primary_node) { create(:geo_node, :primary, url: 'https://localhost:3000/gitlab') }
let(:empty_node) { described_class.new }
let(:primary_node) { create(:geo_node, :primary) }
let(:node) { create(:geo_node) }
......@@ -34,10 +34,7 @@ describe GeoNode, type: :model do
let(:gitlab_host) { 'gitlabhost' }
where(:attribute, :value) do
:schema | 'http'
:host | 'gitlabhost'
:port | 80
:relative_url_root | ''
:url | Gitlab::Routing.url_helpers.root_url
:primary | false
:repos_max_capacity | 25
:files_max_capacity | 10
......@@ -45,22 +42,13 @@ describe GeoNode, type: :model do
end
with_them do
before do
allow(Gitlab.config.gitlab).to receive(:host) { gitlab_host }
end
it { expect(empty_node[attribute]).to eq(value) }
end
end
context 'prevent locking yourself out' do
it 'does not accept adding a non primary node with same details as current_node' do
node = GeoNode.new(
host: Gitlab.config.gitlab.host,
port: Gitlab.config.gitlab.port,
relative_url_root: Gitlab.config.gitlab.relative_url_root,
geo_node_key: build(:geo_node_key)
)
node = build(:geo_node, :primary, primary: false)
expect(node).not_to be_valid
expect(node.errors.full_messages.count).to eq(1)
......@@ -129,18 +117,16 @@ describe GeoNode, type: :model do
end
describe '#current?' do
subject { described_class.new }
it 'returns true when node is the current node' do
stub_current_geo_node(subject)
node = described_class.new(url: described_class.current_node_url)
expect(subject.current?).to eq true
expect(node.current?).to be_truthy
end
it 'returns false when node is not the current node' do
subject.port = Gitlab.config.gitlab.port + 1
node = described_class.new(url: 'http://another.node.com:8080/foo')
expect(subject.current?).to eq false
expect(node.current?).to be_falsy
end
end
......@@ -150,8 +136,9 @@ describe GeoNode, type: :model do
expect(new_node.uri).to be_a URI
end
it 'includes schema home port and relative_url' do
it 'includes schema, host, port and relative_url_root with a terminating /' do
expected_uri = URI.parse(dummy_url)
expected_uri.path += '/'
expect(new_node.uri).to eq(expected_uri)
end
end
......@@ -172,18 +159,18 @@ describe GeoNode, type: :model do
expect(new_node.url).to be_a String
end
it 'includes schema home port and relative_url' do
expected_url = 'https://localhost:3000/gitlab'
it 'includes schema home port and relative_url with a terminating /' do
expected_url = 'https://localhost:3000/gitlab/'
expect(new_node.url).to eq(expected_url)
end
it 'defaults to existing HTTPS and relative URL if present' do
it 'defaults to existing HTTPS and relative URL with a terminating / if present' do
stub_config_setting(port: 443)
stub_config_setting(protocol: 'https')
stub_config_setting(relative_url_root: '/gitlab')
node = GeoNode.new
expect(node.url).to eq('https://localhost/gitlab')
expect(node.url).to eq('https://localhost/gitlab/')
end
end
......@@ -195,15 +182,15 @@ describe GeoNode, type: :model do
end
it 'sets schema field based on url' do
expect(subject.schema).to eq('https')
expect(subject.uri.scheme).to eq('https')
end
it 'sets host field based on url' do
expect(subject.host).to eq('localhost')
expect(subject.uri.host).to eq('localhost')
end
it 'sets port field based on specified by url' do
expect(subject.port).to eq(3000)
expect(subject.uri.port).to eq(3000)
end
context 'when unspecified ports' do
......@@ -212,12 +199,14 @@ describe GeoNode, type: :model do
it 'sets port 80 when http and no port is specified' do
subject.url = dummy_http
expect(subject.port).to eq(80)
expect(subject.uri.port).to eq(80)
end
it 'sets port 443 when https and no port is specified' do
subject.url = dummy_https
expect(subject.port).to eq(443)
expect(subject.uri.port).to eq(443)
end
end
end
......
......@@ -17,7 +17,7 @@ describe Geo::BaseSyncService do
end
describe '#primary_ssh_path_prefix' do
let!(:primary_node) { create(:geo_node, :primary, host: 'primary-geo-node') }
let!(:primary_node) { create(:geo_node, :primary) }
it 'raises exception when clone_url_prefix is nil' do
allow_any_instance_of(GeoNode).to receive(:clone_url_prefix) { nil }
......
......@@ -3,7 +3,7 @@ require 'spec_helper'
describe Geo::RepositorySyncService do
include ::EE::GeoHelpers
set(:primary) { create(:geo_node, :primary, host: 'primary-geo-node', relative_url_root: '/gitlab') }
set(:primary) { create(:geo_node, :primary) }
set(:secondary) { create(:geo_node) }
let(:lease) { double(try_obtain: true) }
......@@ -19,7 +19,7 @@ describe Geo::RepositorySyncService do
describe '#execute' do
let(:project) { create(:project_empty_repo) }
let(:repository) { project.repository }
let(:url_to_repo) { "#{primary.url}/#{project.full_path}.git" }
let(:url_to_repo) { "#{primary.url}#{project.full_path}.git" }
before do
allow(Gitlab::ExclusiveLease).to receive(:new)
......
......@@ -3,7 +3,7 @@ require 'spec_helper'
RSpec.describe Geo::WikiSyncService do
include ::EE::GeoHelpers
set(:primary) { create(:geo_node, :primary, host: 'primary-geo-node', relative_url_root: '/gitlab') }
set(:primary) { create(:geo_node, :primary) }
set(:secondary) { create(:geo_node) }
let(:lease) { double(try_obtain: true) }
......@@ -19,7 +19,7 @@ RSpec.describe Geo::WikiSyncService do
describe '#execute' do
let(:project) { create(:project_empty_repo) }
let(:repository) { project.wiki.repository }
let(:url_to_repo) { "#{primary.url}/#{project.full_path}.wiki.git" }
let(:url_to_repo) { "#{primary.url}#{project.full_path}.wiki.git" }
before do
allow(Gitlab::ExclusiveLease).to receive(:new)
......
......@@ -20,7 +20,7 @@ describe 'geo rake tasks' do
node = GeoNode.first
expect(node.schema).to eq('https')
expect(node.uri.scheme).to eq('https')
expect(node.primary).to be_truthy
expect(node.geo_node_key).to be_nil
end
......
......@@ -4,7 +4,7 @@ describe Geo::PruneEventLogWorker, :geo do
include ::EE::GeoHelpers
subject(:worker) { described_class.new }
set(:primary) { create(:geo_node, :primary, host: 'primary-geo-node') }
set(:primary) { create(:geo_node, :primary) }
set(:secondary) { create(:geo_node) }
before do
......
......@@ -5,8 +5,9 @@ require 'spec_helper'
describe Geo::RepositorySyncWorker, :geo, :truncate do
include ::EE::GeoHelpers
let(:secondary) { create(:geo_node) }
let(:synced_group) { create(:group) }
let!(:primary) { create(:geo_node, :primary) }
let!(:secondary) { create(:geo_node) }
let!(:synced_group) { create(:group) }
let!(:project_in_synced_group) { create(:project, group: synced_group) }
let!(:unsynced_project) { create(:project) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment