Merge branch 'update-gitlab-experiment-to-0.6.2' into 'master'

Update gitlab-experiment to version 0.6.2

See merge request gitlab-org/gitlab!66109

Gemfile

@@ -488,7 +488,7 @@ gem 'flipper', '~> 0.21.0'
 gem 'flipper-active_record', '~> 0.21.0'
 gem 'flipper-active_support_cache_store', '~> 0.21.0'
 gem 'unleash', '~> 0.1.5'
-gem 'gitlab-experiment', '~> 0.6.1'
+gem 'gitlab-experiment', '~> 0.6.2'
 
 # Structured logging
 gem 'lograge', '~> 0.5'

Gemfile.lock

@@ -208,7 +208,7 @@ GEM
     concord (0.1.5)
       adamantium (~> 0.2.0)
       equalizer (~> 0.0.9)
-    concurrent-ruby (1.1.8)
+    concurrent-ruby (1.1.9)
     connection_pool (2.2.2)
     contracts (0.11.0)
     cork (0.3.0)
@@ -471,7 +471,7 @@ GEM
     gitlab-dangerfiles (2.2.2)
       danger (>= 8.3.1)
       danger-gitlab (>= 8.0.0)
-    gitlab-experiment (0.6.1)
+    gitlab-experiment (0.6.2)
       activesupport (>= 3.0)
       request_store (>= 1.0)
       scientist (~> 1.6, >= 1.6.0)
@@ -1486,7 +1486,7 @@ DEPENDENCIES
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.2.2)
-  gitlab-experiment (~> 0.6.1)
+  gitlab-experiment (~> 0.6.2)
   gitlab-fog-azure-rm (~> 1.1.1)
   gitlab-labkit (~> 0.20.0)
   gitlab-license (~> 2.0)

app/experiments/new_project_readme_content_experiment.rb

@@ -19,7 +19,7 @@ class NewProjectReadmeContentExperiment < ApplicationExperiment # rubocop:disabl
   end
 
   def redirect(to_url)
-    experiment_redirect_url(self, to_url)
+    experiment_redirect_url(self, url: to_url)
   end
 
   private

config/initializers/gitlab_experiment.rb

 # frozen_string_literal: true
 
 Gitlab::Experiment.configure do |config|
+  # The base experiment class that will be instantiated when using the
+  # `experiment` DSL, is our ApplicationExperiment. If a custom experiment
+  # class is resolvable by the experiment name, that will be instantiated
+  # instead -- which can then inherit from whatever else it wants to.
+  #
+  # Custom experiment classes can be defined in /app/experiments.
+  #
   config.base_class = 'ApplicationExperiment'
+
+  # Mount the engine and middleware at a gitlab friendly style path.
+  #
+  # The middleware currently focuses only on handling redirection logic, which
+  # is used for instrumenting urls in places where urls are otherwise not
+  # possible to instrument. Emails, and markdown content being among the top
+  # places where this can be useful.
+  #
   config.mount_at = '/-/experiment'
+
+  # We use a long lived redis cache to increase the performance of experiments.
+  #
+  # Experiments can implement exclusionary and segmentation logic that can be
+  # expensive, and so to better handle these cases, once a variant is assigned
+  # to a given context, it's "sticky" to that context. This cache check is one
+  # of the first things in the process of variant resolution, and so if one is
+  # cached, no further logic is executed in resolving variant assignment.
+  #
+  # This means that there's no easy way to currently move a context from one
+  # variant to another. Future tooling will make this easier, but implementing
+  # a custom cache for your experiment may be required in edge cases.
+  #
   config.cache = Gitlab::Experiment::Cache::RedisHashStore.new(
-    pool: ->(&block) { Gitlab::Redis::SharedState.with { |redis| block.call(redis) } }
+    pool: ->(&block) { Gitlab::Redis::SharedState.with(&block) }
   )
+
+  # The middleware instruments and redirects urls, but we don't want this to be
+  # exploited or used to send people from a trusted site to a nefarious one. So
+  # we validate urls before redirecting them.
+  #
+  # This behavior doesn't make perfect sense for self managed installs either,
+  # so we don't think we should redirect in those cases.
+  #
+  valid_domains = %w[about.gitlab.com docs.gitlab.com gitlab.com]
+  config.redirect_url_validator = lambda do |url|
+    Gitlab.dev_env_or_com? && (url = URI.parse(url)) && valid_domains.include?(url.host)
+  rescue URI::InvalidURIError
+    false
+  end
+
+  # Experiments are instrumented using an event based system by default. This
+  # can be overridden in your experiment by specifying a `#track` method.
+  #
+  # The basic behavior though, is to accept any details and pass them along to
+  # snowplow, with an included gitlab_experiment schema, that has various
+  # details about the experiment, like name and variant assignment.
+  #
+  # This uses the Gitlab::Tracking interface, so arbitrary event properties are
+  # permitted, and will be sent along using Gitlab::Tracking::StandardContext.
+  #
   config.tracking_behavior = lambda do |action, event_args|
     Gitlab::Tracking.event(name, action.to_s, **event_args.merge(
       context: (event_args[:context] || []) << SnowplowTracker::SelfDescribingJson.new(

spec/deprecation_toolkit_env.rb

@@ -57,6 +57,7 @@ module DeprecationToolkitEnv
     %w[
       asciidoctor-2.0.12/lib/asciidoctor/extensions.rb
       gitlab-labkit-0.20.0/lib/labkit/correlation/grpc/client_interceptor.rb
+      actionpack-6.1.3.2/lib/action_dispatch/routing/route_set.rb
     ]
   end
 

spec/experiments/application_experiment_spec.rb

@@ -209,6 +209,35 @@ RSpec.describe ApplicationExperiment, :experiment do
     end
   end
 
+  describe "#process_redirect_url" do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:url, :processed_url) do
+      'https://about.gitlab.com/'                 | 'https://about.gitlab.com/'
+      'https://gitlab.com/'                       | 'https://gitlab.com/'
+      'http://docs.gitlab.com'                    | 'http://docs.gitlab.com'
+      'https://docs.gitlab.com/some/path?foo=bar' | 'https://docs.gitlab.com/some/path?foo=bar'
+      'http://badgitlab.com'                      | nil
+      'https://gitlab.com.nefarious.net'          | nil
+      'https://unknown.gitlab.com'                | nil
+      "https://badplace.com\nhttps://gitlab.com"  | nil
+      'https://gitlabbcom'                        | nil
+      'https://gitlabbcom/'                       | nil
+    end
+
+    with_them do
+      it "returns the url or nil if invalid" do
+        allow(Gitlab).to receive(:dev_env_or_com?).and_return(true)
+        expect(subject.process_redirect_url(url)).to eq(processed_url)
+      end
+
+      it "considers all urls invalid when not on dev or com" do
+        allow(Gitlab).to receive(:dev_env_or_com?).and_return(false)
+        expect(subject.process_redirect_url(url)).to be_nil
+      end
+    end
+  end
+
   context "when resolving variants" do
     it "uses the default value as specified in the yaml" do
       expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)

spec/experiments/new_project_readme_content_experiment_spec.rb

@@ -30,7 +30,9 @@ RSpec.describe NewProjectReadmeContentExperiment, :experiment do
     end
 
     it "renders redirect URLs" do
-      expect(markdown).to include(Rails.application.routes.url_helpers.experiment_redirect_url(subject, initial_url))
+      expect(markdown).to include(
+        Rails.application.routes.url_helpers.experiment_redirect_url(subject, url: initial_url)
+      )
     end
   end
 end