diff options
33 files changed, 465 insertions, 136 deletions
diff --git a/app/assets/javascripts/blob/components/constants.js b/app/assets/javascripts/blob/components/constants.js index 93dceacabdd..0137bd38d28 100644 --- a/app/assets/javascripts/blob/components/constants.js +++ b/app/assets/javascripts/blob/components/constants.js @@ -25,7 +25,7 @@ export const BLOB_RENDER_ERRORS = { TOO_LARGE: { id: 'too_large', text: sprintf(__('it is larger than %{limit}'), { - limit: numberToHumanSize(104857600), // 100MB in bytes + limit: numberToHumanSize(10485760), // 10MB in bytes }), }, EXTERNAL: { diff --git a/app/models/active_session.rb b/app/models/active_session.rb index a23190cc8b3..be07c221f32 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -91,8 +91,11 @@ class ActiveSession key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) } redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id)) - redis.del(key_names) - redis.del(rack_session_keys(session_ids)) + + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.del(key_names) + redis.del(rack_session_keys(session_ids)) + end end def self.cleanup(user) @@ -136,8 +139,10 @@ class ActiveSession session_keys = rack_session_keys(session_ids) session_keys.each_slice(SESSION_BATCH_SIZE).flat_map do |session_keys_batch| - redis.mget(session_keys_batch).compact.map do |raw_session| - load_raw_session(raw_session) + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.mget(session_keys_batch).compact.map do |raw_session| + load_raw_session(raw_session) + end end end end @@ -178,7 +183,9 @@ class ActiveSession entry_keys = session_ids.map { |session_id| key_name(user_id, session_id) } - redis.mget(entry_keys) + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.mget(entry_keys) + end end def self.active_session_entries(session_ids, user_id, redis) diff --git a/app/models/ci/build_trace_chunks/redis.rb b/app/models/ci/build_trace_chunks/redis.rb index 813eaf5d839..c3864f78b01 100644 --- a/app/models/ci/build_trace_chunks/redis.rb +++ b/app/models/ci/build_trace_chunks/redis.rb @@ -35,7 +35,10 @@ module Ci keys = keys.map { |key| key_raw(*key) } Gitlab::Redis::SharedState.with do |redis| - redis.del(keys) + # https://gitlab.com/gitlab-org/gitlab/-/issues/224171 + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.del(keys) + end end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index e80eb1b6b69..d9f6139beb6 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -779,14 +779,6 @@ :weight: 2 :idempotent: :tags: [] -- :name: notifications:new_release - :feature_category: :release_orchestration - :has_external_dependencies: - :urgency: :low - :resource_boundary: :unknown - :weight: 2 - :idempotent: - :tags: [] - :name: object_pool:object_pool_create :feature_category: :gitaly :has_external_dependencies: diff --git a/app/workers/new_release_worker.rb b/app/workers/new_release_worker.rb deleted file mode 100644 index fa4703d10f2..00000000000 --- a/app/workers/new_release_worker.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -# TODO: Worker can be removed in 13.2: -# https://gitlab.com/gitlab-org/gitlab/-/issues/218231 -class NewReleaseWorker # rubocop:disable Scalability/IdempotentWorker - include ApplicationWorker - - queue_namespace :notifications - feature_category :release_orchestration - weight 2 - - def perform(release_id) - release = Release.preloaded.find_by_id(release_id) - return unless release - - NotificationService.new.send_new_release_notifications(release) - end -end diff --git a/changelogs/unreleased/223773-correxct-too-large-limit.yml b/changelogs/unreleased/223773-correxct-too-large-limit.yml new file mode 100644 index 00000000000..0c09b93b55c --- /dev/null +++ b/changelogs/unreleased/223773-correxct-too-large-limit.yml @@ -0,0 +1,5 @@ +--- +title: Fixed size limit for too large snippets +merge_request: 35076 +author: +type: fixed diff --git a/changelogs/unreleased/cilium-cluster-application-migration.yml b/changelogs/unreleased/cilium-cluster-application-migration.yml new file mode 100644 index 00000000000..baadbdbdd3a --- /dev/null +++ b/changelogs/unreleased/cilium-cluster-application-migration.yml @@ -0,0 +1,5 @@ +--- +title: Add clusters_applications_cilium DB table +merge_request: 34601 +author: +type: added diff --git a/changelogs/unreleased/rmv-worker-code.yml b/changelogs/unreleased/rmv-worker-code.yml new file mode 100644 index 00000000000..316c099e570 --- /dev/null +++ b/changelogs/unreleased/rmv-worker-code.yml @@ -0,0 +1,5 @@ +--- +title: Remove the unused worker code and its queue +merge_request: 32595 +author: Ravishankar +type: deprecated diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index edeff06c63c..a8ce1946244 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -166,8 +166,6 @@ - 2 - - new_note - 2 -- - notifications - - 2 - - object_pool - 1 - - object_storage diff --git a/db/migrate/20200615234047_create_clusters_applications_cilium.rb b/db/migrate/20200615234047_create_clusters_applications_cilium.rb new file mode 100644 index 00000000000..9f77ee71164 --- /dev/null +++ b/db/migrate/20200615234047_create_clusters_applications_cilium.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateClustersApplicationsCilium < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :clusters_applications_cilium do |t| + t.references :cluster, null: false, index: { unique: true }, foreign_key: { on_delete: :cascade } + t.timestamps_with_timezone null: false + t.integer :status, null: false + t.text :status_reason # rubocop:disable Migration/AddLimitToTextColumns + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 7f1dda6375a..0a77d788bab 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1793,6 +1793,24 @@ CREATE SEQUENCE public.clusters_applications_cert_managers_id_seq ALTER SEQUENCE public.clusters_applications_cert_managers_id_seq OWNED BY public.clusters_applications_cert_managers.id; +CREATE TABLE public.clusters_applications_cilium ( + id bigint NOT NULL, + cluster_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + status integer NOT NULL, + status_reason text +); + +CREATE SEQUENCE public.clusters_applications_cilium_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE public.clusters_applications_cilium_id_seq OWNED BY public.clusters_applications_cilium.id; + CREATE TABLE public.clusters_applications_crossplane ( id integer NOT NULL, created_at timestamp with time zone NOT NULL, @@ -7659,6 +7677,8 @@ ALTER TABLE ONLY public.clusters ALTER COLUMN id SET DEFAULT nextval('public.clu ALTER TABLE ONLY public.clusters_applications_cert_managers ALTER COLUMN id SET DEFAULT nextval('public.clusters_applications_cert_managers_id_seq'::regclass); +ALTER TABLE ONLY public.clusters_applications_cilium ALTER COLUMN id SET DEFAULT nextval('public.clusters_applications_cilium_id_seq'::regclass); + ALTER TABLE ONLY public.clusters_applications_crossplane ALTER COLUMN id SET DEFAULT nextval('public.clusters_applications_crossplane_id_seq'::regclass); ALTER TABLE ONLY public.clusters_applications_elastic_stacks ALTER COLUMN id SET DEFAULT nextval('public.clusters_applications_elastic_stacks_id_seq'::regclass); @@ -8389,6 +8409,9 @@ ALTER TABLE ONLY public.cluster_providers_gcp ALTER TABLE ONLY public.clusters_applications_cert_managers ADD CONSTRAINT clusters_applications_cert_managers_pkey PRIMARY KEY (id); +ALTER TABLE ONLY public.clusters_applications_cilium + ADD CONSTRAINT clusters_applications_cilium_pkey PRIMARY KEY (id); + ALTER TABLE ONLY public.clusters_applications_crossplane ADD CONSTRAINT clusters_applications_crossplane_pkey PRIMARY KEY (id); @@ -9712,6 +9735,8 @@ CREATE UNIQUE INDEX index_cluster_providers_gcp_on_cluster_id ON public.cluster_ CREATE UNIQUE INDEX index_clusters_applications_cert_managers_on_cluster_id ON public.clusters_applications_cert_managers USING btree (cluster_id); +CREATE UNIQUE INDEX index_clusters_applications_cilium_on_cluster_id ON public.clusters_applications_cilium USING btree (cluster_id); + CREATE UNIQUE INDEX index_clusters_applications_crossplane_on_cluster_id ON public.clusters_applications_crossplane USING btree (cluster_id); CREATE UNIQUE INDEX index_clusters_applications_elastic_stacks_on_cluster_id ON public.clusters_applications_elastic_stacks USING btree (cluster_id); @@ -12323,6 +12348,9 @@ ALTER TABLE ONLY public.issue_user_mentions ALTER TABLE ONLY public.merge_request_assignees ADD CONSTRAINT fk_rails_579d375628 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.clusters_applications_cilium + ADD CONSTRAINT fk_rails_59dc12eea6 FOREIGN KEY (cluster_id) REFERENCES public.clusters(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.analytics_cycle_analytics_group_stages ADD CONSTRAINT fk_rails_5a22f40223 FOREIGN KEY (start_event_label_id) REFERENCES public.labels(id) ON DELETE CASCADE; @@ -14105,6 +14133,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200615123055 20200615193524 20200615232735 +20200615234047 20200616145031 20200617000757 20200617001001 diff --git a/doc/development/integrations/secure_partner_integration.md b/doc/development/integrations/secure_partner_integration.md index 22e1f8bf769..19a497641f9 100644 --- a/doc/development/integrations/secure_partner_integration.md +++ b/doc/development/integrations/secure_partner_integration.md @@ -11,6 +11,15 @@ with [onboarding as a partner](https://about.gitlab.com/partners/integrate/). The steps below are a high-level view of what needs to be done to complete an integration as well as linking to more detailed resources for how to do so. +## Integration Tiers + +GitLab's security offerings are designed for GitLab Gold and GitLab Ultimate users, and the +[DevSecOps](https://about.gitlab.com/handbook/use-cases/#4-devsecops-shift-left-security) +use case. All the features are in those tiers. This includes the APIs and standard reporting +framework needed to provide a consistent experience for users to easily bring their preferred +security tools into GitLab. We ask that our integration partners focus their work on those license +tiers so that we can provide the most value to our mutual customers. + ## What is the GitLab Developer Workflow? This workflow is how GitLab users interact with our product and expect it to diff --git a/doc/development/redis.md b/doc/development/redis.md index 6782ea96448..693b9e1ad0d 100644 --- a/doc/development/redis.md +++ b/doc/development/redis.md @@ -33,6 +33,8 @@ stop being consulted if the project is renamed. If the contents of the key are invalidated by a name change, it is better to include a hook that will expire the entry, instead of relying on the key changing. +### Multi-key commands + We don't use [Redis Cluster](https://redis.io/topics/cluster-tutorial) at the moment, but may wish to in the future: [#118820](https://gitlab.com/gitlab-org/gitlab/-/issues/118820). @@ -41,3 +43,8 @@ operations that require several keys to be held on the same Redis server - for instance, diffing two sets held in Redis - the keys should ensure that by enclosing the changeable parts in curly braces, such as, `project:{1}:set_a` and `project:{1}:set_b`. + +Currently, we validate this in the development and test environments +with the [`RedisClusterValidator`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/instrumentation/redis_cluster_validator.rb), +which is enabled for the `cache` and `shared_state` +[Redis instances](https://docs.gitlab.com/omnibus/settings/redis.html#running-with-multiple-redis-instances).. diff --git a/doc/user/project/integrations/prometheus.md b/doc/user/project/integrations/prometheus.md index 58257d1bfd8..d50197e7f64 100644 --- a/doc/user/project/integrations/prometheus.md +++ b/doc/user/project/integrations/prometheus.md @@ -416,6 +416,8 @@ Read the documentation on [links](#add-related-links-to-custom-dashboards). | `priority` | number | optional, defaults to order in file | Order to appear on the dashboard. Higher number means higher priority, which will be higher on the page. Numbers do not need to be consecutive. | | `panels` | array | required | The panels which should be in the panel group. | +Panels in a panel group are laid out in rows consisting of two panels per row. An exception to this rule are single panels on a row: these panels will take the full width of their containing row. + ##### **Panel (`panels`) properties** | Property | Type | Required | Description | diff --git a/lib/gitlab/analytics/unique_visits.rb b/lib/gitlab/analytics/unique_visits.rb index 3ed76c5c7f5..ba0447990b1 100644 --- a/lib/gitlab/analytics/unique_visits.rb +++ b/lib/gitlab/analytics/unique_visits.rb @@ -43,7 +43,9 @@ module Gitlab keys = TARGET_IDS.map { |target_id| key(target_id, week_of) } Gitlab::Redis::SharedState.with do |redis| - redis.pfcount(*keys) + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.pfcount(*keys) + end end end diff --git a/lib/gitlab/discussions_diff/highlight_cache.rb b/lib/gitlab/discussions_diff/highlight_cache.rb index 75d5a5df74b..4bec6467c1a 100644 --- a/lib/gitlab/discussions_diff/highlight_cache.rb +++ b/lib/gitlab/discussions_diff/highlight_cache.rb @@ -36,7 +36,9 @@ module Gitlab content = Redis::Cache.with do |redis| - redis.mget(keys) + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.mget(keys) + end end content.map! do |lines| @@ -58,7 +60,11 @@ module Gitlab keys = raw_keys.map { |id| cache_key_for(id) } - Redis::Cache.with { |redis| redis.del(keys) } + Redis::Cache.with do |redis| + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.del(keys) + end + end end def cache_key_for(raw_key) diff --git a/lib/gitlab/gitaly_client/cleanup_service.rb b/lib/gitlab/gitaly_client/cleanup_service.rb index e2293d3121a..8836d08a807 100644 --- a/lib/gitlab/gitaly_client/cleanup_service.rb +++ b/lib/gitlab/gitaly_client/cleanup_service.rb @@ -13,15 +13,15 @@ module Gitlab end def apply_bfg_object_map_stream(io, &blk) - responses = GitalyClient.call( + GitalyClient.streaming_call( storage, :cleanup_service, :apply_bfg_object_map_stream, build_object_map_enum(io), timeout: GitalyClient.long_timeout - ) - - responses.each(&blk) + ) do |response| + response.each(&blk) + end end private diff --git a/lib/gitlab/gl_repository.rb b/lib/gitlab/gl_repository.rb index abd4e847a50..7346de13626 100644 --- a/lib/gitlab/gl_repository.rb +++ b/lib/gitlab/gl_repository.rb @@ -43,10 +43,10 @@ module Gitlab end def self.parse(gl_repository) - result = ::Gitlab::GlRepository::Identifier.new(gl_repository) + identifier = ::Gitlab::GlRepository::Identifier.parse(gl_repository) - repo_type = result.repo_type - container = result.fetch_container! + repo_type = identifier.repo_type + container = identifier.container [container, repo_type.project_for(container), repo_type] end diff --git a/lib/gitlab/gl_repository/identifier.rb b/lib/gitlab/gl_repository/identifier.rb index dc3e7931696..57350b1edb0 100644 --- a/lib/gitlab/gl_repository/identifier.rb +++ b/lib/gitlab/gl_repository/identifier.rb @@ -3,71 +3,83 @@ module Gitlab class GlRepository class Identifier - attr_reader :gl_repository, :repo_type + include Gitlab::Utils::StrongMemoize - def initialize(gl_repository) - @gl_repository = gl_repository - @segments = gl_repository.split('-') + InvalidIdentifier = Class.new(ArgumentError) - raise_error if segments.size > 3 + def self.parse(gl_repository) + segments = gl_repository&.split('-') - @repo_type = find_repo_type - @container_id = find_container_id - @container_class = find_container_class - end + # gl_repository can either have 2 or 3 segments: + # + # TODO: convert all 2-segment format to 3-segment: + # https://gitlab.com/gitlab-org/gitlab/-/issues/219192 + identifier = case segments&.size + when 2 + TwoPartIdentifier.new(*segments) + when 3 + ThreePartIdentifier.new(*segments) + end + + return identifier if identifier&.valid? - def fetch_container! - container_class.find_by_id(container_id) + raise InvalidIdentifier, %Q(Invalid GL Repository "#{gl_repository}") end - private + # The older 2-segment format, where the container is implied. + # eg. project-1, wiki-1 + class TwoPartIdentifier < Identifier + def initialize(repo_type_name, container_id_str) + @container_id_str = container_id_str + @repo_type_name = repo_type_name + end - attr_reader :segments, :container_class, :container_id + private - def find_repo_type - type_name = three_segments_format? ? segments.last : segments.first - type = Gitlab::GlRepository.types[type_name] + def container_class + repo_type.container_class + end + end - raise_error unless type + # The newer 3-segment format, where the container is explicit + # eg. group-1-wiki, project-1-wiki + class ThreePartIdentifier < Identifier + def initialize(container_type, container_id_str, repo_type_name) + @container_id_str = container_id_str + @container_type = container_type + @repo_type_name = repo_type_name + end - type - end + private - def find_container_class - if three_segments_format? - case segments[0] + def container_class + case @container_type when 'project' Project when 'group' Group - else - raise_error end - else - repo_type.container_class end end - def find_container_id - id = Integer(segments[1], 10, exception: false) - - raise_error unless id + def repo_type + strong_memoize(:repo_type) { Gitlab::GlRepository.types[repo_type_name] } + end - id + def container + strong_memoize(:container) { container_class.find_by_id(container_id) } end - # gl_repository can either have 2 or 3 segments: - # "wiki-1" is the older 2-segment format, where container is implied. - # "group-1-wiki" is the newer 3-segment format, including container information. - # - # TODO: convert all 2-segment format to 3-segment: - # https://gitlab.com/gitlab-org/gitlab/-/issues/219192 - def three_segments_format? - segments.size == 3 + def valid? + repo_type.present? && container_class.present? && container_id&.positive? end - def raise_error - raise ArgumentError, "Invalid GL Repository \"#{gl_repository}\"" + private + + attr_reader :container_id_str, :repo_type_name + + def container_id + strong_memoize(:container_id) { Integer(container_id_str, 10, exception: false) } end end end diff --git a/lib/gitlab/import_export/json/streaming_serializer.rb b/lib/gitlab/import_export/json/streaming_serializer.rb index 1c2f9b6cc17..05b7679e0ff 100644 --- a/lib/gitlab/import_export/json/streaming_serializer.rb +++ b/lib/gitlab/import_export/json/streaming_serializer.rb @@ -69,8 +69,16 @@ module Gitlab key_preloads = preloads&.dig(key) records = records.preload(key_preloads) if key_preloads - records.find_each(batch_size: batch_size) do |record| - items << Raw.new(record.to_json(options)) + records.in_batches(of: batch_size) do |batch| # rubocop:disable Cop/InBatches + # order each batch by its primary key to ensure + # consistent and predictable ordering of each exported relation + # as additional `WHERE` clauses can impact the order in which data is being + # returned by database when no `ORDER` is specified + batch = batch.reorder(batch.klass.primary_key) + + batch.each do |record| + items << Raw.new(record.to_json(options)) + end end end diff --git a/lib/gitlab/instrumentation/redis.rb b/lib/gitlab/instrumentation/redis.rb index 82b4701872f..4a85a313fd7 100644 --- a/lib/gitlab/instrumentation/redis.rb +++ b/lib/gitlab/instrumentation/redis.rb @@ -5,9 +5,9 @@ module Gitlab # Aggregates Redis measurements from different request storage sources. class Redis ActionCable = Class.new(RedisBase) - Cache = Class.new(RedisBase) + Cache = Class.new(RedisBase).enable_redis_cluster_validation Queues = Class.new(RedisBase) - SharedState = Class.new(RedisBase) + SharedState = Class.new(RedisBase).enable_redis_cluster_validation STORAGES = [ActionCable, Cache, Queues, SharedState].freeze diff --git a/lib/gitlab/instrumentation/redis_base.rb b/lib/gitlab/instrumentation/redis_base.rb index 012543e1645..a96ca1f4dea 100644 --- a/lib/gitlab/instrumentation/redis_base.rb +++ b/lib/gitlab/instrumentation/redis_base.rb @@ -71,6 +71,16 @@ module Gitlab query_time.round(::Gitlab::InstrumentationHelper::DURATION_PRECISION) end + def redis_cluster_validate!(command) + RedisClusterValidator.validate!(command) if @redis_cluster_validation + end + + def enable_redis_cluster_validation + @redis_cluster_validation = true + + self + end + private def request_count_key diff --git a/lib/gitlab/instrumentation/redis_cluster_validator.rb b/lib/gitlab/instrumentation/redis_cluster_validator.rb new file mode 100644 index 00000000000..6800e5667f6 --- /dev/null +++ b/lib/gitlab/instrumentation/redis_cluster_validator.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'rails' +require 'redis' + +module Gitlab + module Instrumentation + module RedisClusterValidator + # Generate with: + # + # Gitlab::Redis::Cache + # .with { |redis| redis.call('COMMAND') } + # .select { |command| command[3] != command[4] } + # .map { |command| [command[0].upcase, { first: command[3], last: command[4], step: command[5] }] } + # .sort_by(&:first) + # .to_h + # + MULTI_KEY_COMMANDS = { + "BITOP" => { first: 2, last: -1, step: 1 }, + "BLPOP" => { first: 1, last: -2, step: 1 }, + "BRPOP" => { first: 1, last: -2, step: 1 }, + "BRPOPLPUSH" => { first: 1, last: 2, step: 1 }, + "BZPOPMAX" => { first: 1, last: -2, step: 1 }, + "BZPOPMIN" => { first: 1, last: -2, step: 1 }, + "DEL" => { first: 1, last: -1, step: 1 }, + "EXISTS" => { first: 1, last: -1, step: 1 }, + "MGET" => { first: 1, last: -1, step: 1 }, + "MSET" => { first: 1, last: -1, step: 2 }, + "MSETNX" => { first: 1, last: -1, step: 2 }, + "PFCOUNT" => { first: 1, last: -1, step: 1 }, + "PFMERGE" => { first: 1, last: -1, step: 1 }, + "RENAME" => { first: 1, last: 2, step: 1 }, + "RENAMENX" => { first: 1, last: 2, step: 1 }, + "RPOPLPUSH" => { first: 1, last: 2, step: 1 }, + "SDIFF" => { first: 1, last: -1, step: 1 }, + "SDIFFSTORE" => { first: 1, last: -1, step: 1 }, + "SINTER" => { first: 1, last: -1, step: 1 }, + "SINTERSTORE" => { first: 1, last: -1, step: 1 }, + "SMOVE" => { first: 1, last: 2, step: 1 }, + "SUNION" => { first: 1, last: -1, step: 1 }, + "SUNIONSTORE" => { first: 1, last: -1, step: 1 }, + "UNLINK" => { first: 1, last: -1, step: 1 }, + "WATCH" => { first: 1, last: -1, step: 1 } + }.freeze + + CrossSlotError = Class.new(StandardError) + + class << self + def validate!(command) + return unless Rails.env.development? || Rails.env.test? + return if allow_cross_slot_commands? + + command_name = command.first.to_s.upcase + argument_positions = MULTI_KEY_COMMANDS[command_name] + + return unless argument_positions + + arguments = command.flatten[argument_positions[:first]..argument_positions[:last]] + + key_slots = arguments.each_slice(argument_positions[:step]).map do |args| + key_slot(args.first) + end + + unless key_slots.uniq.length == 1 + raise CrossSlotError.new("Redis command #{command_name} arguments hash to different slots. See https://docs.gitlab.com/ee/development/redis.html#multi-key-commands") + end + end + + # Keep track of the call stack to allow nested calls to work. + def allow_cross_slot_commands + Thread.current[:allow_cross_slot_commands] ||= 0 + Thread.current[:allow_cross_slot_commands] += 1 + + yield + ensure + Thread.current[:allow_cross_slot_commands] -= 1 + end + + private + + def allow_cross_slot_commands? + Thread.current[:allow_cross_slot_commands].to_i > 0 + end + + def key_slot(key) + ::Redis::Cluster::KeySlotConverter.convert(extract_hash_tag(key)) + end + + # This is almost identical to Redis::Cluster::Command#extract_hash_tag, + # except that it returns the original string if no hash tag is found. + # + def extract_hash_tag(key) + s = key.index('{') + + return key unless s + + e = key.index('}', s + 1) + + return key unless e + + key[s + 1..e - 1] + end + end + end + end +end diff --git a/lib/gitlab/instrumentation/redis_interceptor.rb b/lib/gitlab/instrumentation/redis_interceptor.rb index a36aade59c3..725e9939ad9 100644 --- a/lib/gitlab/instrumentation/redis_interceptor.rb +++ b/lib/gitlab/instrumentation/redis_interceptor.rb @@ -7,6 +7,9 @@ module Gitlab module RedisInterceptor def call(*args, &block) start = Time.now + + instrumentation_class.redis_cluster_validate!(args.first) + super(*args, &block) ensure duration = (Time.now - start) diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb index e891b805879..6ba9ee26634 100644 --- a/lib/gitlab/set_cache.rb +++ b/lib/gitlab/set_cache.rb @@ -20,7 +20,10 @@ module Gitlab with do |redis| keys = keys.map { |key| cache_key(key) } - unlink_or_delete(redis, keys) + + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + unlink_or_delete(redis, keys) + end end end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index c380eb293b5..6af91d473a6 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -18,7 +18,9 @@ namespace :cache do count: REDIS_CLEAR_BATCH_SIZE ) - redis.del(*keys) if keys.any? + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.del(*keys) if keys.any? + end break if cursor == REDIS_SCAN_START_STOP end diff --git a/spec/frontend/blob/components/blob_content_error_spec.js b/spec/frontend/blob/components/blob_content_error_spec.js index 6eb5cfb71aa..508b1ed7e68 100644 --- a/spec/frontend/blob/components/blob_content_error_spec.js +++ b/spec/frontend/blob/components/blob_content_error_spec.js @@ -24,9 +24,9 @@ describe('Blob Content Error component', () => { describe('collapsed and too large blobs', () => { it.each` - error | reason | options - ${BLOB_RENDER_ERRORS.REASONS.COLLAPSED} | ${'it is larger than 1.00 MiB'} | ${[BLOB_RENDER_ERRORS.OPTIONS.LOAD.text, BLOB_RENDER_ERRORS.OPTIONS.DOWNLOAD.text]} - ${BLOB_RENDER_ERRORS.REASONS.TOO_LARGE} | ${'it is larger than 100.00 MiB'} | ${[BLOB_RENDER_ERRORS.OPTIONS.DOWNLOAD.text]} + error | reason | options + ${BLOB_RENDER_ERRORS.REASONS.COLLAPSED} | ${'it is larger than 1.00 MiB'} | ${[BLOB_RENDER_ERRORS.OPTIONS.LOAD.text, BLOB_RENDER_ERRORS.OPTIONS.DOWNLOAD.text]} + ${BLOB_RENDER_ERRORS.REASONS.TOO_LARGE} | ${'it is larger than 10.00 MiB'} | ${[BLOB_RENDER_ERRORS.OPTIONS.DOWNLOAD.text]} `('renders correct reason for $error.id', ({ error, reason, options }) => { createComponent({ viewerError: error.id, diff --git a/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb b/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb index 3381c69ea0d..014e54fb3b1 100644 --- a/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb @@ -22,5 +22,11 @@ RSpec.describe Gitlab::GitalyClient::CleanupService do client.apply_bfg_object_map_stream(StringIO.new) end + + it 'is wrapped as a streaming call' do + expect(Gitlab::GitalyClient).to receive(:streaming_call).with(anything, :cleanup_service, :apply_bfg_object_map_stream, anything, anything) + + client.apply_bfg_object_map_stream(StringIO.new) + end end end diff --git a/spec/lib/gitlab/gl_repository/identifier_spec.rb b/spec/lib/gitlab/gl_repository/identifier_spec.rb index d21e29f0898..e95aaaa6690 100644 --- a/spec/lib/gitlab/gl_repository/identifier_spec.rb +++ b/spec/lib/gitlab/gl_repository/identifier_spec.rb @@ -14,6 +14,21 @@ RSpec.describe Gitlab::GlRepository::Identifier do let(:expected_container) { project } let(:expected_type) { Gitlab::GlRepository::PROJECT } end + + pending 'https://gitlab.com/gitlab-org/gitlab/-/issues/219192' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project.id } + let(:identifier) { "project-#{record_id}-code" } + let(:expected_container) { project } + let(:expected_type) { Gitlab::GlRepository::PROJECT } + end + end + + it_behaves_like 'parsing gl_repository identifier' do + let(:identifier) { "project-1000000" } + let(:expected_container) { nil } + let(:expected_type) { Gitlab::GlRepository::PROJECT } + end end describe 'wiki' do @@ -23,6 +38,13 @@ RSpec.describe Gitlab::GlRepository::Identifier do let(:expected_container) { project } let(:expected_type) { Gitlab::GlRepository::WIKI } end + + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project.id } + let(:identifier) { "project-#{record_id}-wiki" } + let(:expected_container) { project } + let(:expected_type) { Gitlab::GlRepository::WIKI } + end end describe 'snippet' do @@ -54,29 +76,30 @@ RSpec.describe Gitlab::GlRepository::Identifier do end end - describe 'incorrect format' do - def expect_error_raised_for(identifier) - expect { described_class.new(identifier) }.to raise_error(ArgumentError) - end - - it 'raises error for incorrect id' do - expect_error_raised_for('wiki-noid') + context 'when the format is incorrect' do + where(:identifier) do + [ + 'wiki-noid', + 'foo-2', + 'project-0', + '2-project', + 'snippet-2-wiki', + 'project-wibble-wiki', + 'wiki-1-project', + 'snippet', + 'project-1-wiki-bar' + ] end - it 'raises error for incorrect type' do - expect_error_raised_for('foo-2') - end - - it 'raises error for incorrect three-segment container' do - expect_error_raised_for('snippet-2-wiki') - end - - it 'raises error for one segment' do - expect_error_raised_for('snippet') + with_them do + it 'raises InvalidIdentifier' do + expect { described_class.parse(identifier) }.to raise_error(described_class::InvalidIdentifier) + end end - it 'raises error for more than three segments' do - expect_error_raised_for('project-1-wiki-bar') + it 'raises InvalidIdentifier on project-1-project' do + pending 'https://gitlab.com/gitlab-org/gitlab/-/issues/219192' + expect { described_class.parse('project-1-project') }.to raise_error(described_class::InvalidIdentifier) end end end diff --git a/spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb b/spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb index 0d015f0693c..eb6b07ce02f 100644 --- a/spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb +++ b/spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb @@ -61,6 +61,20 @@ RSpec.describe Gitlab::ImportExport::JSON::StreamingSerializer do subject.execute end + + context 'relation ordering' do + before do + create_list(:issue, 5, project: exportable) + end + + it 'orders exported issues by primary key' do + expected_issues = exportable.issues.reorder(:id).map(&:to_json) + + expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, expected_issues) + + subject.execute + end + end end context 'with single relation' do diff --git a/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb new file mode 100644 index 00000000000..6aa23118fcc --- /dev/null +++ b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'support/helpers/rails_helpers' +require 'rspec-parameterized' + +describe Gitlab::Instrumentation::RedisClusterValidator do + include RailsHelpers + + describe '.validate!' do + using RSpec::Parameterized::TableSyntax + + context 'Rails environments' do + where(:env, :should_raise) do + 'production' | false + 'staging' | false + 'development' | true + 'test' | true + end + + with_them do + it do + stub_rails_env(env) + + args = [:mget, 'foo', 'bar'] + + if should_raise + expect { described_class.validate!(args) } + .to raise_error(described_class::CrossSlotError) + else + expect { described_class.validate!(args) }.not_to raise_error + end + end + end + end + + where(:command, :arguments, :should_raise) do + :rename | %w(foo bar) | true + :RENAME | %w(foo bar) | true + 'rename' | %w(foo bar) | true + 'RENAME' | %w(foo bar) | true + :rename | %w(iaa ahy) | false # 'iaa' and 'ahy' hash to the same slot + :rename | %w({foo}:1 {foo}:2) | false + :rename | %w(foo foo bar) | false # This is not a valid command but should not raise here + :mget | %w(foo bar) | true + :mget | %w(foo foo bar) | true + :mget | %w(foo foo) | false + :blpop | %w(foo bar 1) | true + :blpop | %w(foo foo 1) | false + :mset | %w(foo a bar a) | true + :mset | %w(foo a foo a) | false + :del | %w(foo bar) | true + :del | [%w(foo bar)] | true # Arguments can be a nested array + :del | %w(foo foo) | false + :hset | %w(foo bar) | false # Not a multi-key command + end + + with_them do + it do + args = [command] + arguments + + if should_raise + expect { described_class.validate!(args) } + .to raise_error(described_class::CrossSlotError) + else + expect { described_class.validate!(args) }.not_to raise_error + end + end + end + end + + describe '.allow_cross_slot_commands' do + it 'does not raise for invalid arguments' do + expect do + described_class.allow_cross_slot_commands do + described_class.validate!([:mget, 'foo', 'bar']) + end + end.not_to raise_error + end + + it 'allows nested invocation' do + expect do + described_class.allow_cross_slot_commands do + described_class.allow_cross_slot_commands do + described_class.validate!([:mget, 'foo', 'bar']) + end + + described_class.validate!([:mget, 'foo', 'bar']) + end + end.not_to raise_error + end + end +end diff --git a/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb index 97f4341340d..28137530038 100644 --- a/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true RSpec.shared_examples 'parsing gl_repository identifier' do - subject { described_class.new(identifier) } + subject { described_class.parse(identifier) } it 'returns correct information' do - aggregate_failures do - expect(subject.repo_type).to eq(expected_type) - expect(subject.fetch_container!).to eq(expected_container) - end + expect(subject).to have_attributes( + repo_type: expected_type, + container: expected_container + ) end end diff --git a/spec/workers/new_release_worker_spec.rb b/spec/workers/new_release_worker_spec.rb deleted file mode 100644 index 1c9953c0154..00000000000 --- a/spec/workers/new_release_worker_spec.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -# TODO: Worker can be removed in 13.2: -# https://gitlab.com/gitlab-org/gitlab/-/issues/218231 -require 'spec_helper' - -RSpec.describe NewReleaseWorker do - let(:release) { create(:release) } - - it 'sends a new release notification' do - expect_next_instance_of(NotificationService) do |instance| - expect(instance).to receive(:send_new_release_notifications).with(release) - end - - described_class.new.perform(release.id) - end -end |