Add latest changes from gitlab-org/gitlab@master

app/controllers/customers_dot/proxy_controller.rb

@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-module CustomersDot
-  class ProxyController < ApplicationController
-    skip_before_action :authenticate_user!
-    skip_before_action :verify_authenticity_token
-
-    feature_category :purchase
-
-    BASE_URL = Gitlab::SubscriptionPortal::SUBSCRIPTIONS_URL
-
-    def graphql
-      response = Gitlab::HTTP.post("#{BASE_URL}/graphql",
-        body: request.raw_post,
-        headers: { 'Content-Type' => 'application/json' }
-      )
-
-      render json: response.body, status: response.code
-    end
-  end
-end

app/models/application_setting.rb

@@ -456,6 +456,9 @@ class ApplicationSetting < ApplicationRecord
   validates :ci_jwt_signing_key,
             rsa_key: true, allow_nil: true
 
+  validates :customers_dot_jwt_signing_key,
+            rsa_key: true, allow_nil: true
+
   validates :rate_limiting_response_text,
             length: { maximum: 255, message: _('is too long (maximum is %{count} characters)') },
             allow_blank: true
@@ -559,6 +562,7 @@ class ApplicationSetting < ApplicationRecord
   attr_encrypted :slack_app_secret, encryption_options_base_32_aes_256_gcm
   attr_encrypted :slack_app_verification_token, encryption_options_base_32_aes_256_gcm
   attr_encrypted :ci_jwt_signing_key, encryption_options_base_32_aes_256_gcm
+  attr_encrypted :customers_dot_jwt_signing_key, encryption_options_base_32_aes_256_gcm
   attr_encrypted :secret_detection_token_revocation_token, encryption_options_base_32_aes_256_gcm
   attr_encrypted :cloud_license_auth_token, encryption_options_base_32_aes_256_gcm
   attr_encrypted :external_pipeline_validation_service_token, encryption_options_base_32_aes_256_gcm

app/models/namespace/traversal_hierarchy.rb

@@ -36,7 +36,7 @@ class Namespace
             SET traversal_ids = cte.traversal_ids
             FROM (#{recursive_traversal_ids}) as cte
             WHERE namespaces.id = cte.id
-              AND namespaces.traversal_ids <> cte.traversal_ids
+              AND namespaces.traversal_ids::bigint[] <> cte.traversal_ids
             """
       Namespace.transaction do
         @root.lock!
@@ -51,7 +51,7 @@ class Namespace
     def incorrect_traversal_ids
       Namespace
         .joins("INNER JOIN (#{recursive_traversal_ids}) as cte ON namespaces.id = cte.id")
-        .where('namespaces.traversal_ids <> cte.traversal_ids')
+        .where('namespaces.traversal_ids::bigint[] <> cte.traversal_ids')
     end
 
     private
@@ -66,9 +66,9 @@ class Namespace
 
       <<~SQL
       WITH RECURSIVE cte(id, traversal_ids, cycle) AS (
-        VALUES(#{root_id}, ARRAY[#{root_id}], false)
+        VALUES(#{root_id}::bigint, ARRAY[#{root_id}]::bigint[], false)
       UNION ALL
-        SELECT n.id, cte.traversal_ids || n.id, n.id = ANY(cte.traversal_ids)
+        SELECT n.id, cte.traversal_ids || n.id::bigint, n.id = ANY(cte.traversal_ids)
         FROM namespaces n, cte
         WHERE n.parent_id = cte.id AND NOT cycle
       )

db/migrate/20210630222522_add_customers_dot_jwt_signing_key_to_application_settings.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AddCustomersDotJwtSigningKeyToApplicationSettings < ActiveRecord::Migration[6.1]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :encrypted_customers_dot_jwt_signing_key, :binary
+    add_column :application_settings, :encrypted_customers_dot_jwt_signing_key_iv, :binary
+  end
+end

db/migrate/20210630224625_generate_customers_dot_jwt_signing_key.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+class GenerateCustomersDotJwtSigningKey < ActiveRecord::Migration[6.1]
+  DOWNTIME = false
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+
+    attr_encrypted :customers_dot_jwt_signing_key, {
+      mode: :per_attribute_iv,
+      key: Gitlab::Utils.ensure_utf8_size(Rails.application.secrets.db_key_base, bytes: 32.bytes),
+      algorithm: 'aes-256-gcm',
+      encode: true
+    }
+  end
+
+  def up
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.find_each do |application_setting|
+      application_setting.update(customers_dot_jwt_signing_key: OpenSSL::PKey::RSA.new(2048).to_pem)
+    end
+  end
+
+  def down
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.find_each do |application_setting|
+      application_setting.update_columns(encrypted_customers_dot_jwt_signing_key: nil, encrypted_customers_dot_jwt_signing_key_iv: nil)
+    end
+  end
+end

db/schema_migrations/20210630222522

@@ -0,0 +1 @@
+6cd7654e53bb3dd75118dd399473c98e9953cbb28eaed7a4e3a232de38ca72d1

db/schema_migrations/20210630224625

@@ -0,0 +1 @@
+570edf634eba17e5c7d388fdf7103acb857e477374763205535e280f72050f71

db/structure.sql

@@ -9606,6 +9606,8 @@ CREATE TABLE application_settings (
     encrypted_mailgun_signing_key_iv bytea,
     mailgun_events_enabled boolean DEFAULT false NOT NULL,
     usage_ping_features_enabled boolean DEFAULT false NOT NULL,
+    encrypted_customers_dot_jwt_signing_key bytea,
+    encrypted_customers_dot_jwt_signing_key_iv bytea,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)),
     CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),

doc/development/database/multiple_databases.md

@@ -99,3 +99,149 @@ and their tables must be placed in two directories for now:
 - `db/ci_migrate`
 
 We aim to keep the schema for both tables the same across both databases.
+
+### Removing joins between `ci_*` and non `ci_*` tables
+
+We are planning on moving all the `ci_*` tables to a separate database so
+referencing `ci_*` tables with other tables will not be possible. This means,
+that using any kind of `JOIN` in SQL queries will not work. We have identified
+already many such examples that need to be fixed in
+<https://gitlab.com/groups/gitlab-org/-/epics/6289> .
+
+The following are some real examples that have resulted from this and these
+patterns may apply to future cases.
+
+#### Remove the code
+
+The simplest solution we've seen several times now has been an existing scope
+that is unused. This is the easiest example to fix. So the first step is to
+investigate if the code is unused and then simply remove it. These are some
+real examples:
+
+- <https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67162>
+- <https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66714>
+- <https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66503>
+
+There may be more examples where the code is used, but we can evaluate
+if we need it or if the feature should behave this way.
+Before complicating things by adding new columns and tables,
+consider if you can simplify the solution and still meet the requirements.
+One case being evaluated involves changing how certain `UsageData` is
+calculated to remove a join query in
+<https://gitlab.com/gitlab-org/gitlab/-/issues/336170>. This is a good candidate
+to evaluate, because `UsageData` is not critical to users and it may be possible
+to get a similarly useful metric with a simpler approach. Alternatively we may
+find that nobody is using these metrics, so we can remove them.
+
+#### De-normalize some foreign key to the table
+
+De-normalization refers to adding redundant precomputed (duplicated) data to
+a table to simplify certain queries or to improve performance. In this
+case, it can be useful when you are doing a join that involves three tables, where
+you are joining through some intermediate table.
+
+Generally when modeling a database schema, a "normalized" structure is
+preferred because of the following reasons:
+
+- Duplicate data uses extra storage.
+- Duplicate data needs to be kept in sync.
+
+Sometimes normalized data is less performant so de-normalization has been a
+common technique GitLab has used to improve the performance of database queries
+for a while. The above problems are mitigated when the following conditions are
+met:
+
+1. There isn't much data (for example, it's just an integer column).
+1. The data does not update often (for example, the `project_id` column is almost
+   never updated for most tables).
+
+One example we found was the `security_scans` table. This table has a foreign
+key `security_scans.build_id` which allows you to join to the build. Therefore
+you could join to the project like so:
+
+```sql
+select projects.* from security_scans
+inner join ci_builds on security_scans.build_id = ci_builds.id
+inner join projects on ci_builds.project_id = projects.id
+```
+
+The problem with this query is that `ci_builds` is in a different database
+from the other two tables.
+
+The solution in this case is to add the `project_id` column to
+`security_scans`. This doesn't use much extra storage, and due to the way
+these features work, it's never updated (a build never moves projects).
+
+This simplified the query to:
+
+```sql
+select projects.* from security_scans
+inner join projects on security_scans.project_id = projects.id
+```
+
+This also improves performance because you don't need to join through an extra
+table.
+
+You can see this approach implemented in
+<https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66963> . This MR also
+de-normalizes `pipeline_id` to fix a similar query.
+
+#### De-normalize into an extra table
+
+Sometimes the previous de-normalization (adding an extra column) doesn't work for
+your specific case. This may be due to the fact that your data is not 1:1, or
+because the table you're adding to is already too wide (for example, the `projects`
+table shouldn't have more columns added).
+
+In this case you may decide to just store the extra data in a separate table.
+
+One example where this approach is being used was to implement the
+`Project.with_code_coverage` scope. This scope was essentially used to narrow
+down a list of projects to only those that have at one point in time used code
+coverage features. This query (simplified) was:
+
+```sql
+select projects.* from projects
+inner join ci_daily_build_group_report_results on ci_daily_build_group_report_results.project_id = projects.id
+where ((data->'coverage') is not null)
+and ci_daily_build_group_report_results.default_branch = true
+group by projects.id
+```
+
+This work is still in progress but the current plan is to introduce a new table
+called `projects_with_ci_feature_usage` which has 2 columns `project_id` and
+`ci_feature`. This table would be written to the first time a project creates a
+`ci_daily_build_group_report_results` for code coverage. Therefore the new
+query would be:
+
+```sql
+select projects.* from projects
+inner join projects_with_ci_feature_usage on projects_with_ci_feature_usage.project_id = projects.id
+where projects_with_ci_feature_usage.ci_feature = 'code_coverage'
+```
+
+The above example uses as a text column for simplicity but we should probably
+use an [enum](../creating_enums.md) to save space.
+
+The downside of this new design is that this may need to be
+updated (removed if the `ci_daily_build_group_report_results` is deleted).
+Depending on your domain, however, this may not be necessary because deletes are
+edge cases or impossible, or because the user impact of seeing the project on the
+list page may not be problematic. It's also possible to implement the
+logic to delete these rows if or whenever necessary in your domain.
+
+Finally, this de-normalization and new query also improves performance because
+it does less joins and needs less filtering.
+
+#### Summary of cross-join removal patterns
+
+A quick checklist for fixing a specific join query would be:
+
+1. Is the code even used? If not just remove it
+1. If the code is used, then is this feature even used or can we implement the
+   feature in a simpler way and still meet the requirements. Always prefer the
+   simplest option.
+1. Can we remove the join if we de-normalize the data you are joining to by
+   adding a new column
+1. Can we remove the join by adding a new table in the correct database that
+   replicates the minimum data needed to do the join

spec/migrations/generate_customers_dot_jwt_signing_key_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe GenerateCustomersDotJwtSigningKey do
+  let(:application_settings) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'application_settings'
+
+      attr_encrypted :customers_dot_jwt_signing_key, {
+        mode: :per_attribute_iv,
+        key: Gitlab::Utils.ensure_utf8_size(Rails.application.secrets.db_key_base, bytes: 32.bytes),
+        algorithm: 'aes-256-gcm',
+        encode: true
+      }
+    end
+  end
+
+  it 'generates JWT signing key' do
+    application_settings.create!
+
+    reversible_migration do |migration|
+      migration.before -> {
+        settings = application_settings.first
+
+        expect(settings.customers_dot_jwt_signing_key).to be_nil
+        expect(settings.encrypted_customers_dot_jwt_signing_key).to be_nil
+        expect(settings.encrypted_customers_dot_jwt_signing_key_iv).to be_nil
+      }
+
+      migration.after -> {
+        settings = application_settings.first
+
+        expect(settings.encrypted_customers_dot_jwt_signing_key).to be_present
+        expect(settings.encrypted_customers_dot_jwt_signing_key_iv).to be_present
+        expect { OpenSSL::PKey::RSA.new(settings.customers_dot_jwt_signing_key) }.not_to raise_error
+      }
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -834,6 +834,23 @@ RSpec.describe ApplicationSetting do
         end
       end
 
+      describe '#customers_dot_jwt_signing_key' do
+        it { is_expected.not_to allow_value('').for(:customers_dot_jwt_signing_key) }
+        it { is_expected.not_to allow_value('invalid RSA key').for(:customers_dot_jwt_signing_key) }
+        it { is_expected.to allow_value(nil).for(:customers_dot_jwt_signing_key) }
+        it { is_expected.to allow_value(OpenSSL::PKey::RSA.new(1024).to_pem).for(:customers_dot_jwt_signing_key) }
+
+        it 'is encrypted' do
+          subject.customers_dot_jwt_signing_key = OpenSSL::PKey::RSA.new(1024).to_pem
+
+          aggregate_failures do
+            expect(subject.encrypted_customers_dot_jwt_signing_key).to be_present
+            expect(subject.encrypted_customers_dot_jwt_signing_key_iv).to be_present
+            expect(subject.encrypted_customers_dot_jwt_signing_key).not_to eq(subject.customers_dot_jwt_signing_key)
+          end
+        end
+      end
+
       describe '#cloud_license_auth_token' do
         it { is_expected.to allow_value(nil).for(:cloud_license_auth_token) }
 

spec/requests/customers_dot/proxy_controller_spec.rb

@@ -1,37 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe CustomersDot::ProxyController, type: :request do
-  describe 'POST graphql' do
-    let_it_be(:customers_dot) { "#{Gitlab::SubscriptionPortal::SUBSCRIPTIONS_URL}/graphql" }
-
-    it 'forwards request body to customers dot' do
-      request_params = '{ "foo" => "bar" }'
-
-      stub_request(:post, customers_dot)
-
-      post customers_dot_proxy_graphql_path, params: request_params
-
-      expect(WebMock).to have_requested(:post, customers_dot).with(body: request_params)
-    end
-
-    it 'responds with customers dot status' do
-      stub_request(:post, customers_dot).to_return(status: 500)
-
-      post customers_dot_proxy_graphql_path
-
-      expect(response).to have_gitlab_http_status(:internal_server_error)
-    end
-
-    it 'responds with customers dot response body' do
-      customers_dot_response = 'foo'
-
-      stub_request(:post, customers_dot).to_return(body: customers_dot_response)
-
-      post customers_dot_proxy_graphql_path
-
-      expect(response.body).to eq(customers_dot_response)
-    end
-  end
-end