Merge branch 'revert-342adf4e' into 'master'

Revert "Merge branch 'cat-default-secondary-proxying-unified-urls' into 'master'"

See merge request gitlab-org/gitlab!74979

doc/administration/geo/secondary_proxy/index.md

@@ -7,14 +7,11 @@ type: howto
 
 # Geo proxying for secondary sites **(PREMIUM SELF)**
 
-> - [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/5914) in GitLab 14.4 [with a flag](../../feature_flags.md) named `geo_secondary_proxy`. Disabled by default.
-> - [Enabled by default for unified URLs](https://gitlab.com/gitlab-org/gitlab/-/issues/325732) in GitLab 14.5.
-> - [Disabled by default for different URLs](https://gitlab.com/gitlab-org/gitlab/-/issues/325732) in GitLab 14.5 [with a flag](../../feature_flags.md) named `geo_secondary_proxy_separate_urls`.
+> [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/5914) in GitLab 14.4 [with a flag](../../feature_flags.md) named `geo_secondary_proxy`. Disabled by default.
 
 FLAG:
-On self-managed GitLab, this feature is only available by default for Geo sites using a unified URL. See below to
-[set up a unified URL for Geo sites](#set-up-a-unified-url-for-geo-sites).
-The feature is not ready for production use with separate URLs.
+On self-managed GitLab, by default this feature is not available. See below to [Set up a unified URL for Geo sites](#set-up-a-unified-url-for-geo-sites).
+The feature is not ready for production use.
 
 Use Geo proxying to:
 
@@ -68,10 +65,8 @@ a single URL used by all Geo sites, including the primary.
    In the Geo administration page of the **primary** site, edit each Geo secondary that
    is using the secondary proxying and set the `URL` field to the single URL.
    Make sure the primary site is also using this URL.
-   
-## Disable Geo proxying
 
-You can disable the secondary proxying on each Geo site, separately, by following these steps:
+### Enable secondary proxying
 
 1. SSH into each application node (serving user traffic directly) on your secondary Geo site
    and add the following environment variable:
@@ -82,7 +77,7 @@ You can disable the secondary proxying on each Geo site, separately, by followin
 
    ```ruby
    gitlab_workhorse['env'] = {
-     "GEO_SECONDARY_PROXY" => "0"
+     "GEO_SECONDARY_PROXY" => "1"
    }
    ```
 
@@ -92,34 +87,18 @@ You can disable the secondary proxying on each Geo site, separately, by followin
    gitlab-ctl reconfigure
    ```
 
+1. SSH into one node running Rails on your primary Geo site and enable the Geo secondary proxy feature flag:
+
+   ```shell
+   sudo gitlab-rails runner "Feature.enable(:geo_secondary_proxy)"
+   ```
+
 ## Enable Geo proxying with Separate URLs
 
 The ability to use proxying with separate URLs is still in development. You can follow the
 ["Geo secondary proxying with separate URLs" epic](https://gitlab.com/groups/gitlab-org/-/epics/6865)
 for progress.
 
-To try out this feature, enable the `geo_secondary_proxy_separate_urls` feature flag.
-SSH into one node running Rails on your primary Geo site and run:
-
-```shell
-sudo gitlab-rails runner "Feature.enable(:geo_secondary_proxy_separate_urls)"
-```
-
-## Limitations
-
-The asynchronous Geo replication can cause unexpected issues when secondary proxying is used, for accelerated
-data types that may be replicated to the Geo secondaries with a delay.
-
-For example, we found a potential issue where
-[Replication lag introduces read-your-own-write inconsistencies](https://gitlab.com/gitlab-org/gitlab/-/issues/345267).
-If the replication lag is high enough, this can result in Git reads receiving stale data when hitting a secondary.
-
-Non-Rails requests are not proxied, so other services may need to use a separate, non-unified URL to ensure requests
-are always sent to the primary. These services include:
-
-- GitLab Container Registry - [can be configured to use a separate domain](../../packages/container_registry.md#configure-container-registry-under-its-own-domain).
-- GitLab Pages - should always use a separate domain, as part of [the prerequisites for running GitLab Pages](../../pages/index.md#prerequisites).
-
 ## Features accelerated by secondary Geo sites
 
 Most HTTP traffic sent to a secondary Geo site can be proxied to the primary Geo site. With this architecture,

doc/administration/geo/setup/index.md

@@ -26,7 +26,6 @@ If you installed GitLab using the Omnibus packages (highly recommended):
 1. [Configure GitLab](../replication/configuration.md) to set the **primary** and **secondary** site(s).
 1. Optional: [Configure a secondary LDAP server](../../auth/ldap/index.md) for the **secondary** site(s). See [notes on LDAP](../index.md#ldap).
 1. Follow the [Using a Geo Site](../replication/usage.md) guide.
-1. [Configure Geo secondary proxying](../secondary_proxy/index.md) to use a single, unified URL for all Geo sites. This step is recommended to accelerate most read requests while transparently proxying writes to the primary Geo site.
 
 ## Post-installation documentation
 

ee/config/feature_flags/development/geo_secondary_proxy_separate_urls.yml → ee/config/feature_flags/development/geo_secondary_proxy.yml

 ---
-name: geo_secondary_proxy_separate_urls
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74329
+name: geo_secondary_proxy
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56297
 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325732
-milestone: '14.5'
+milestone: '13.11'
 type: development
 group: group::geo
 default_enabled: false

ee/lib/ee/api/geo.rb

@@ -38,9 +38,7 @@ module EE
             #
             # The cached values are invalidated when changed.
             #
-            return super unless ::Gitlab::Geo.secondary_with_primary?
-
-            if ::Gitlab::Geo.secondary_with_unified_url? || ::Feature.enabled?(:geo_secondary_proxy_separate_urls, default_enabled: :yaml)
+            if ::Feature.enabled?(:geo_secondary_proxy, default_enabled: :yaml) && ::Gitlab::Geo.secondary_with_primary?
               { geo_proxy_url: ::Gitlab::Geo.primary_node.internal_url }
             else
               super

ee/lib/gitlab/geo.rb

@@ -86,10 +86,6 @@ module Gitlab
       self.secondary? && self.primary_node_configured?
     end
 
-    def self.secondary_with_unified_url?
-      self.secondary_with_primary? && self.primary_node.url == self.current_node.url
-    end
-
     def self.license_allows?
       ::License.feature_available?(:geo)
     end

ee/spec/lib/gitlab/geo_spec.rb

@@ -120,76 +120,6 @@ RSpec.describe Gitlab::Geo, :geo, :request_store do
         expect(described_class.secondary?).to be_falsey
       end
     end
-
-    context 'when current node is a primary node' do
-      it 'returns false' do
-        expect(described_class.secondary?).to be_falsey
-      end
-    end
-  end
-
-  describe '.secondary_with_primary?' do
-    context 'when current node is a primary node' do
-      it 'returns false' do
-        expect(described_class.secondary_with_primary?).to be_falsey
-      end
-    end
-
-    context 'when current node is a secondary node' do
-      before do
-        stub_current_geo_node(secondary_node)
-      end
-
-      it 'returns true' do
-        expect(described_class.secondary_with_primary?).to be_truthy
-      end
-
-      context 'when a primary does not exist' do
-        it 'returns false' do
-          allow(::Gitlab::Geo).to receive(:primary_node_configured?).and_return(false)
-
-          expect(described_class.secondary_with_primary?).to be_falsey
-        end
-      end
-    end
-  end
-
-  describe '.secondary_with_unified_url?' do
-    context 'when current node is a primary node' do
-      it 'returns false' do
-        expect(described_class.secondary_with_unified_url?).to be_falsey
-      end
-    end
-
-    context 'when current node is a secondary node' do
-      before do
-        stub_current_geo_node(secondary_node)
-      end
-
-      context 'when a primary does not exist' do
-        it 'returns false' do
-          allow(::Gitlab::Geo).to receive(:primary_node_configured?).and_return(false)
-
-          expect(described_class.secondary_with_unified_url?).to be_falsey
-        end
-      end
-
-      context 'when the secondary node has different URLs' do
-        it 'returns false' do
-          expect(described_class.secondary_with_unified_url?).to be_falsey
-        end
-      end
-
-      context 'when the secondary node has unified URL' do
-        before do
-          stub_current_geo_node(create(:geo_node, url: primary_node.url))
-        end
-
-        it 'returns true' do
-          expect(described_class.secondary_with_unified_url?).to be_truthy
-        end
-      end
-    end
   end
 
   describe '.enabled?' do
@@ -249,6 +179,21 @@ RSpec.describe Gitlab::Geo, :geo, :request_store do
     end
   end
 
+  describe '.secondary?' do
+    context 'when current node is secondary' do
+      it 'returns true' do
+        stub_current_geo_node(secondary_node)
+        expect(described_class.secondary?).to be_truthy
+      end
+    end
+
+    context 'current node is primary' do
+      it 'returns false' do
+        expect(described_class.secondary?).to be_falsey
+      end
+    end
+  end
+
   describe '.expire_cache!' do
     it 'clears the Geo cache keys', :request_store do
       described_class::CACHE_KEYS.each do |key|

ee/spec/requests/api/geo_spec.rb

@@ -671,11 +671,9 @@ RSpec.describe API::Geo do
         end
       end
 
-      context 'when this is a secondary site with unified URL' do
-        let_it_be(:unified_url_secondary_node) { create(:geo_node, url: primary_node.url) }
-
+      context 'when this is a secondary site' do
         before do
-          stub_current_geo_node(unified_url_secondary_node)
+          stub_current_geo_node(secondary_node)
         end
 
         context 'when a primary exists' do
@@ -699,42 +697,16 @@ RSpec.describe API::Geo do
         end
       end
 
-      context 'when this is a secondary site with separate URLs' do
+      context 'when geo_secondary_proxy feature flag is disabled' do
         before do
-          stub_current_geo_node(secondary_node)
-        end
-
-        context 'when a primary does not exist' do
-          it 'returns empty data' do
-            allow(::Gitlab::Geo).to receive(:primary_node_configured?).and_return(false)
-
-            subject
-
-            expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response).to be_empty
-          end
-        end
-
-        context 'when geo_secondary_proxy_separate_urls feature flag is disabled' do
-          before do
-            stub_feature_flags(geo_secondary_proxy_separate_urls: false)
-          end
-
-          it 'returns empty data' do
-            subject
-
-            expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response).to be_empty
-          end
+          stub_feature_flags(geo_secondary_proxy: false)
         end
 
-        context 'when geo_secondary_proxy_separate_urls feature flag is enabled' do
-          it 'returns the primary internal URL' do
-            subject
+        it 'returns empty data' do
+          subject
 
-            expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response['geo_proxy_url']).to match(primary_node.internal_url)
-          end
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response).to be_empty
         end
       end
     end

spec/spec_helper.rb

@@ -478,7 +478,3 @@ Rugged::Settings['search_path_global'] = Rails.root.join('tmp/tests').to_s
 
 # Initialize FactoryDefault to use create_default helper
 TestProf::FactoryDefault.init
-
-# Exclude the Geo proxy API request from getting on_next_request Warden handlers,
-# necessary to prevent race conditions with feature tests not getting authenticated.
-::Warden.asset_paths << %r{^/api/v4/geo/proxy$}

workhorse/internal/upstream/upstream.go

@@ -65,7 +65,7 @@ func newUpstream(cfg config.Config, accessLogger *logrus.Logger, routesCallback
 		Config:       cfg,
 		accessLogger: accessLogger,
 		// Kind of a feature flag. See https://gitlab.com/groups/gitlab-org/-/epics/5914#note_564974130
-		enableGeoProxyFeature: os.Getenv("GEO_SECONDARY_PROXY") != "0",
+		enableGeoProxyFeature: os.Getenv("GEO_SECONDARY_PROXY") == "1",
 		geoProxyBackend:       &url.URL{},
 	}
 	if up.geoProxyPollSleep == nil {
@@ -207,8 +207,8 @@ func (u *upstream) findGeoProxyRoute(cleanedPath string, r *http.Request) *route
 
 func (u *upstream) pollGeoProxyAPI() {
 	for {
-		u.geoProxyPollSleep(geoProxyApiPollingInterval)
 		u.callGeoProxyAPI()
+		u.geoProxyPollSleep(geoProxyApiPollingInterval)
 	}
 }
 

workhorse/internal/upstream/upstream_test.go

@@ -310,9 +310,5 @@ func startWorkhorseServer(railsServerURL string, enableGeoProxyFeature bool) (*h
 		}
 	}
 
-	// Since the first sleep happens before any API call, this ensures
-	// we call the API at least once.
-	waitForNextApiPoll()
-
 	return ws, ws.Close, waitForNextApiPoll
 }