From 09409d9dfb406911f45c867e2364e63a2620e966 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 4 May 2022 00:09:14 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .rubocop_todo.yml | 7 - .rubocop_todo/style/empty_else.yml | 60 ++++++++ .../javascripts/persistent_user_callouts.js | 1 + app/models/concerns/sha256_attribute.rb | 45 ------ app/models/concerns/sha_attribute.rb | 64 ++++++-- app/models/key.rb | 2 +- .../development/omit_epic_subscribed.yml | 8 + db/docs/approvers.yml | 4 +- db/docs/clusters_applications_cilium.yml | 2 +- db/docs/clusters_applications_prometheus.yml | 4 +- ...ty_orchestration_policy_configurations.yml | 4 +- ...ty_orchestration_policy_rule_schedules.yml | 3 +- ..._up_fix_merge_request_diff_commit_users.rb | 15 ++ db/schema_migrations/20220502015011 | 1 + doc/administration/gitaly/index.md | 14 ++ doc/administration/gitaly/praefect.md | 85 ++++++++++ doc/administration/gitaly/troubleshooting.md | 4 + doc/development/feature_flags/index.md | 11 ++ doc/user/clusters/agent/install/index.md | 2 +- lib/feature.rb | 65 ++++++-- ...ll_integrations_enable_ssl_verification.rb | 2 +- lib/gitlab/metrics/subscribers/rack_attack.rb | 28 +--- lib/gitlab/middleware/go.rb | 1 + rubocop/cop/gitlab/avoid_feature_get.rb | 4 +- spec/lib/feature_spec.rb | 46 +++++- .../metrics/subscribers/rack_attack_spec.rb | 55 +++++-- spec/models/concerns/sha256_attribute_spec.rb | 91 ----------- spec/models/concerns/sha_attribute_spec.rb | 145 ++++++++++-------- .../requests/rack_attack_shared_examples.rb | 19 ++- 29 files changed, 501 insertions(+), 291 deletions(-) create mode 100644 .rubocop_todo/style/empty_else.yml delete mode 100644 app/models/concerns/sha256_attribute.rb create mode 100644 config/feature_flags/development/omit_epic_subscribed.yml create mode 100644 db/post_migrate/20220502015011_clean_up_fix_merge_request_diff_commit_users.rb create mode 100644 db/schema_migrations/20220502015011 delete mode 100644 spec/models/concerns/sha256_attribute_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index fd114bee890..76605609d52 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -205,13 +205,6 @@ Style/BarePercentLiterals: Style/CaseLikeIf: Enabled: false -# Offense count: 55 -# Cop supports --auto-correct. -# Configuration parameters: EnforcedStyle. -# SupportedStyles: empty, nil, both -Style/EmptyElse: - Enabled: false - # Offense count: 205 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle. diff --git a/.rubocop_todo/style/empty_else.yml b/.rubocop_todo/style/empty_else.yml new file mode 100644 index 00000000000..ed840856e19 --- /dev/null +++ b/.rubocop_todo/style/empty_else.yml @@ -0,0 +1,60 @@ +--- +# Cop supports --auto-correct. +Style/EmptyElse: + # Offense count: 59 + # Temporarily disabled due to too many offenses + Enabled: false + Exclude: + - 'app/controllers/concerns/issuable_collections_action.rb' + - 'app/controllers/concerns/sends_blob.rb' + - 'app/controllers/google_api/authorizations_controller.rb' + - 'app/controllers/projects_controller.rb' + - 'app/finders/group_finder.rb' + - 'app/finders/merge_requests_finder/params.rb' + - 'app/finders/snippets_finder.rb' + - 'app/graphql/mutations/concerns/mutations/spam_protection.rb' + - 'app/graphql/resolvers/group_milestones_resolver.rb' + - 'app/graphql/types/ci/detailed_status_type.rb' + - 'app/graphql/types/packages/package_file_type.rb' + - 'app/graphql/types/packages/package_type.rb' + - 'app/helpers/submodule_helper.rb' + - 'app/models/commit.rb' + - 'app/models/legacy_diff_discussion.rb' + - 'app/models/note.rb' + - 'app/models/performance_monitoring/prometheus_dashboard.rb' + - 'app/models/repository.rb' + - 'app/models/resource_state_event.rb' + - 'app/models/resource_timebox_event.rb' + - 'app/services/award_emojis/add_service.rb' + - 'app/services/merge_requests/update_service.rb' + - 'app/workers/post_receive.rb' + - 'config/initializers/doorkeeper_openid_connect.rb' + - 'ee/app/controllers/admin/audit_logs_controller.rb' + - 'ee/app/controllers/ee/groups_controller.rb' + - 'ee/app/helpers/ee/kerberos_spnego_helper.rb' + - 'ee/app/helpers/ee/trial_helper.rb' + - 'ee/app/models/ee/audit_event.rb' + - 'ee/app/services/ee/users/update_service.rb' + - 'ee/app/services/epics/tree_reorder_service.rb' + - 'ee/app/services/gitlab_subscriptions/check_future_renewal_service.rb' + - 'ee/app/services/projects/update_mirror_service.rb' + - 'ee/app/workers/audit_events/audit_event_streaming_worker.rb' + - 'ee/app/workers/gitlab_subscriptions/notify_seats_exceeded_worker.rb' + - 'ee/db/fixtures/development/20_vulnerabilities.rb' + - 'ee/lib/elastic/latest/note_instance_proxy.rb' + - 'ee/lib/gitlab/geo/oauth/logout_token.rb' + - 'lib/api/subscriptions.rb' + - 'lib/gitlab/auth/o_auth/provider.rb' + - 'lib/gitlab/database/load_balancing/sidekiq_server_middleware.rb' + - 'lib/gitlab/database/sha256_attribute.rb' + - 'lib/gitlab/diff/parser.rb' + - 'lib/gitlab/git.rb' + - 'lib/gitlab/git/ref.rb' + - 'lib/gitlab/git/tag.rb' + - 'lib/gitlab/pagination/keyset/paginator.rb' + - 'lib/gitlab/sidekiq_daemon/monitor.rb' + - 'lib/gitlab/x509/signature.rb' + - 'lib/peek/views/external_http.rb' + - 'qa/qa/support/loglinking.rb' + - 'spec/support/helpers/api_internal_base_helpers.rb' + - 'spec/support/shared_examples/requests/api/snippets_shared_examples.rb' diff --git a/app/assets/javascripts/persistent_user_callouts.js b/app/assets/javascripts/persistent_user_callouts.js index dee832c01d5..100ffc0664b 100644 --- a/app/assets/javascripts/persistent_user_callouts.js +++ b/app/assets/javascripts/persistent_user_callouts.js @@ -13,6 +13,7 @@ const PERSISTENT_USER_CALLOUTS = [ '.js-approaching-seats-count-threshold', '.js-storage-enforcement-banner', '.js-user-over-limit-free-plan-alert', + '.js-minute-limit-banner', ]; const initCallouts = () => { diff --git a/app/models/concerns/sha256_attribute.rb b/app/models/concerns/sha256_attribute.rb deleted file mode 100644 index 3c906642b1a..00000000000 --- a/app/models/concerns/sha256_attribute.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -module Sha256Attribute - extend ActiveSupport::Concern - - class_methods do - def sha256_attribute(name) - return if ENV['STATIC_VERIFICATION'] - - validate_binary_column_exists!(name) unless Rails.env.production? - - attribute(name, Gitlab::Database::Sha256Attribute.new) - end - - # This only gets executed in non-production environments as an additional check to ensure - # the column is the correct type. In production it should behave like any other attribute. - # See https://gitlab.com/gitlab-org/gitlab/merge_requests/5502 for more discussion - def validate_binary_column_exists!(name) - return unless database_exists? - - unless table_exists? - warn "WARNING: sha256_attribute #{name.inspect} is invalid since the table doesn't exist - you may need to run database migrations" - return - end - - column = columns.find { |c| c.name == name.to_s } - - unless column - warn "WARNING: sha256_attribute #{name.inspect} is invalid since the column doesn't exist - you may need to run database migrations" - return - end - - unless column.type == :binary - raise ArgumentError, "sha256_attribute #{name.inspect} is invalid since the column type is not :binary" - end - rescue StandardError => error - Gitlab::AppLogger.error "Sha256Attribute initialization: #{error.message}" - raise - end - - def database_exists? - database.exists? - end - end -end diff --git a/app/models/concerns/sha_attribute.rb b/app/models/concerns/sha_attribute.rb index e49f4d03bda..701d2fda5c5 100644 --- a/app/models/concerns/sha_attribute.rb +++ b/app/models/concerns/sha_attribute.rb @@ -3,39 +3,71 @@ module ShaAttribute extend ActiveSupport::Concern - # Needed for the database method - include DatabaseReflection + class ShaAttributeTypeMismatchError < StandardError + def initialize(column_name, column_type) + @column_name = column_name + @column_type = column_type + end + + def message + "sha_attribute :#{@column_name} should be a :binary column but it is :#{@column_type}" + end + end + + class Sha256AttributeTypeMismatchError < ShaAttributeTypeMismatchError + def message + "sha256_attribute :#{@column_name} should be a :binary column but it is :#{@column_type}" + end + end class_methods do def sha_attribute(name) return if ENV['STATIC_VERIFICATION'] - validate_binary_column_exists!(name) if Rails.env.development? || Rails.env.test? + sha_attribute_fields << name attribute(name, Gitlab::Database::ShaAttribute.new) end + def sha_attribute_fields + @sha_attribute_fields ||= [] + end + + def sha256_attribute(name) + return if ENV['STATIC_VERIFICATION'] + + sha256_attribute_fields << name + + attribute(name, Gitlab::Database::Sha256Attribute.new) + end + + def sha256_attribute_fields + @sha256_attribute_fields ||= [] + end + # This only gets executed in non-production environments as an additional check to ensure # the column is the correct type. In production it should behave like any other attribute. # See https://gitlab.com/gitlab-org/gitlab/merge_requests/5502 for more discussion - def validate_binary_column_exists!(name) - return unless database_exists? - return unless table_exists? + def load_schema! + super - column = columns.find { |c| c.name == name.to_s } + return if Rails.env.production? - return unless column + sha_attribute_fields.each do |field| + column = columns_hash[field.to_s] - unless column.type == :binary - raise ArgumentError, "sha_attribute #{name.inspect} is invalid since the column type is not :binary" + if column && column.type != :binary + raise ShaAttributeTypeMismatchError.new(column.name, column.type) + end end - rescue StandardError => error - Gitlab::AppLogger.error "ShaAttribute initialization: #{error.message}" - raise - end - def database_exists? - database.exists? + sha256_attribute_fields.each do |field| + column = columns_hash[field.to_s] + + if column && column.type != :binary + raise Sha256AttributeTypeMismatchError.new(column.name, column.type) + end + end end end end diff --git a/app/models/key.rb b/app/models/key.rb index 07d5b1eea3a..5e61ecf73f5 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -5,7 +5,7 @@ require 'digest/md5' class Key < ApplicationRecord include AfterCommitQueue include Sortable - include Sha256Attribute + include ShaAttribute include Expirable include FromUnion diff --git a/config/feature_flags/development/omit_epic_subscribed.yml b/config/feature_flags/development/omit_epic_subscribed.yml new file mode 100644 index 00000000000..13205b9c1dc --- /dev/null +++ b/config/feature_flags/development/omit_epic_subscribed.yml @@ -0,0 +1,8 @@ +--- +name: omit_epic_subscribed +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86016 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/360663 +milestone: '15.0' +type: development +group: group::product planning +default_enabled: false diff --git a/db/docs/approvers.yml b/db/docs/approvers.yml index 01f7ac1f4e9..86fc663be93 100644 --- a/db/docs/approvers.yml +++ b/db/docs/approvers.yml @@ -3,7 +3,7 @@ table_name: approvers classes: - Approver feature_categories: -- security_orchestration -description: TODO +- code_review +description: Approvers of given merge request introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/commit/3cc78d89984d9c9df8372c52b7bba38e6226f9f2 milestone: '7.13' diff --git a/db/docs/clusters_applications_cilium.yml b/db/docs/clusters_applications_cilium.yml index 97428bcc665..1fa71a93911 100644 --- a/db/docs/clusters_applications_cilium.yml +++ b/db/docs/clusters_applications_cilium.yml @@ -4,6 +4,6 @@ classes: - Clusters::Applications::Cilium feature_categories: - container_network_security -description: TODO +description: Information about installed instance of Cilium in the cluster introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34601 milestone: '13.2' diff --git a/db/docs/clusters_applications_prometheus.yml b/db/docs/clusters_applications_prometheus.yml index 5118d286ffc..394ab63f81f 100644 --- a/db/docs/clusters_applications_prometheus.yml +++ b/db/docs/clusters_applications_prometheus.yml @@ -3,7 +3,7 @@ table_name: clusters_applications_prometheus classes: - Clusters::Applications::Prometheus feature_categories: -- container_network_security -description: TODO +- kubernetes_management +description: Information about installed instance of Prometheus in the cluster introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/commit/0d4548026f3060ca0a8f7aa8d8fc89838bc66130 milestone: '10.4' diff --git a/db/docs/security_orchestration_policy_configurations.yml b/db/docs/security_orchestration_policy_configurations.yml index 0505aad2317..0f91d180dc3 100644 --- a/db/docs/security_orchestration_policy_configurations.yml +++ b/db/docs/security_orchestration_policy_configurations.yml @@ -4,6 +4,8 @@ classes: - Security::OrchestrationPolicyConfiguration feature_categories: - security_orchestration -description: TODO +description: | + Relates a Project/Namespace and Security Orchestration Policy Project, where Security + Policies are stored in the repository as a YAML file. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53743 milestone: '13.9' diff --git a/db/docs/security_orchestration_policy_rule_schedules.yml b/db/docs/security_orchestration_policy_rule_schedules.yml index a4fea57adaf..160e8657f7c 100644 --- a/db/docs/security_orchestration_policy_rule_schedules.yml +++ b/db/docs/security_orchestration_policy_rule_schedules.yml @@ -4,6 +4,7 @@ classes: - Security::OrchestrationPolicyRuleSchedule feature_categories: - security_orchestration -description: TODO +description: | + Security policies scheduled to run based on cadence defined in the policy introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59842 milestone: '13.12' diff --git a/db/post_migrate/20220502015011_clean_up_fix_merge_request_diff_commit_users.rb b/db/post_migrate/20220502015011_clean_up_fix_merge_request_diff_commit_users.rb new file mode 100644 index 00000000000..a3e59b38975 --- /dev/null +++ b/db/post_migrate/20220502015011_clean_up_fix_merge_request_diff_commit_users.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CleanUpFixMergeRequestDiffCommitUsers < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + MIGRATION_CLASS = 'FixMergeRequestDiffCommitUsers' + + def up + finalize_background_migration(MIGRATION_CLASS) + end + + def down + # no-op + end +end diff --git a/db/schema_migrations/20220502015011 b/db/schema_migrations/20220502015011 new file mode 100644 index 00000000000..dff75369b56 --- /dev/null +++ b/db/schema_migrations/20220502015011 @@ -0,0 +1 @@ +fbc4aa4bc958a5b3d9b184d459e1e540a5f83f01c5a8206d9546ccb28817c143 \ No newline at end of file diff --git a/doc/administration/gitaly/index.md b/doc/administration/gitaly/index.md index e0358dd5f54..b02416349aa 100644 --- a/doc/administration/gitaly/index.md +++ b/doc/administration/gitaly/index.md @@ -550,6 +550,20 @@ To monitor the number of repositories that have no healthy, up-to-date replicas: - `gitaly_praefect_unavailable_repositories` +To monitor [repository verification](praefect.md#repository-verification), use the following Prometheus metrics: + +- `gitaly_praefect_verification_queue_depth`, the total number of replicas pending verification. This + metric is scraped from the database and is only available when Prometheus is scraping the database metrics. +- `gitaly_praefect_verification_jobs_dequeued_total`, the number of verification jobs picked up by the + worker. +- `gitaly_praefect_verification_jobs_completed_total`, the number of verification jobs completed by the + worker. The `result` label indicates the end result of the jobs: + - `valid` indicates the expected replica existed on the storage. + - `invalid` indicates the replica expected to exist did not exist on the storage. + - `error` indicates the job failed and has to be retried. +- `gitaly_praefect_stale_verification_leases_released_total`, the number of stale verification leases + released. + You can also monitor the [Praefect logs](../logs.md#praefect-logs). #### Database metrics `/db_metrics` endpoint diff --git a/doc/administration/gitaly/praefect.md b/doc/administration/gitaly/praefect.md index e6f5b1b5128..b3f4e511c50 100644 --- a/doc/administration/gitaly/praefect.md +++ b/doc/administration/gitaly/praefect.md @@ -1209,6 +1209,91 @@ You can configure: If `default_replication_factor` is unset, the repositories are always replicated on every node defined in `virtual_storages`. If a new node is introduced to the virtual storage, both new and existing repositories are replicated to the node automatically. +## Repository verification + +> [Introduced](https://gitlab.com/gitlab-org/gitaly/-/issues/4080) in GitLab 15.0. + +Praefect stores metadata about the repositories in a database. If the repositories are modified on disk +without going through Praefect, the metadata can become inaccurate. Because the metadata is used for replication +and routing decisions, any inaccuracies may cause problems. Praefect contains a background worker that +periodically verifies the metadata against the actual state on the disks. The worker: + +1. Picks up a batch of replicas to verify on healthy storages. The replicas are either unverified or have exceeded + the configured verification interval. Replicas that have never been verified are prioritized, followed by + the other replicas ordered by longest time since the last successful verification. +1. Checks whether the replicas exist on their respective storages. If the: + - Replica exists, update its last successful verification time. + - Replica doesn't exist, remove its metadata record. + - Check failed, the replica is picked up for verification again when the next worker dequeues more work. + +The worker acquires an exclusive verification lease on each of the replicas it is about to verify. This avoids multiple +workers from verifying the same replica concurrently. The worker releases the leases when it has completed its check. +Praefect contains a background goroutine that releases stale leases every 10 seconds when workers are terminated for +some reason without releasing the lease. + +The worker logs each of the metadata removals prior to executing them. For example: + +```json +{ + "level": "info", + "msg": "removing metadata records of non-existent replicas", + "replicas": { + "default": { + "@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b.git": [ + "praefect-internal-0" + ] + } + } +} +``` + +### Enable verification worker + +The worker is disabled by default. It can be enabled by configuring the verification interval. The interval +accepts any valid [Go duration string](https://pkg.go.dev/time#ParseDuration). + +To enable the worker and verify replicas every 7 days: + +```ruby +praefect['background_verification_verification_interval'] = '168h' +``` + +Values of 0 and below disable the background verifier. + +```ruby +praefect['background_verification_verification_interval'] = '0' +``` + +### Prioritize verification manually + +You can prioritize verification of some replicas ahead of their next scheduled verification time. +This might be needed after a disk failure, for example, when the administrator knows that the disk contents may have +changed. Praefect would eventually verify the replicas again, but users may encounter errors in the meantime. + +To manually prioritize reverification of some replicas, use the `praefect verify` subcommand. The subcommand marks +replicas as unverified. Unverified replicas are prioritized by the background verification worker. The verification +worker must be enabled for the replicas to be verified. + +Prioritize verifying the replicas of a specific repository: + +```shell +sudo /opt/gitlab/embedded/bin/praefect -config /var/opt/gitlab/praefect/config.toml verify -repository-id= +``` + +Prioritize verifying all replicas stored on a virtual storage: + +```shell +sudo /opt/gitlab/embedded/bin/praefect -config /var/opt/gitlab/praefect/config.toml verify -virtual-storage= +``` + +Prioritize verifying all replicas stored on a storage: + +```shell +sudo /opt/gitlab/embedded/bin/praefect -config /var/opt/gitlab/praefect/config.toml verify -virtual-storage= -storage= +``` + +The output includes the number of replicas that were marked unverified. + ## Automatic failover and primary election strategies Praefect regularly checks the health of each Gitaly node. This is used to automatically fail over diff --git a/doc/administration/gitaly/troubleshooting.md b/doc/administration/gitaly/troubleshooting.md index 1be0bf23ed5..cefddad1b62 100644 --- a/doc/administration/gitaly/troubleshooting.md +++ b/doc/administration/gitaly/troubleshooting.md @@ -513,16 +513,19 @@ Replicas: Generation: 1, fully up to date Healthy: true Valid Primary: true + Verified At: 2021-04-01 10:04:20 +0000 UTC - Storage: "gitaly-2" Assigned: true Generation: 0, behind by 1 changes Healthy: true Valid Primary: false + Verified At: unverified - Storage: "gitaly-3" Assigned: true Generation: replica not yet created Healthy: false Valid Primary: false + Verified At: unverified ``` #### Available metadata @@ -548,6 +551,7 @@ For each replica, the following metadata is available: | `Generation` | Latest confirmed generation of the replica. It indicates:

- The replica is fully up to date if the generation matches the repository's generation.
- The replica is outdated if the replica's generation is less than the repository's generation.
- `replica not yet created` if the replica does not yet exist at all on the storage. | | `Healthy` | Indicates whether the Gitaly node that is hosting this replica is considered healthy by the consensus of Praefect nodes. | | `Valid Primary` | Indicates whether the replica is fit to serve as the primary node. If the repository's primary is not a valid primary, a failover occurs on the next write to the repository if there is another replica that is a valid primary. A replica is a valid primary if:

- It is stored on a healthy Gitaly node.
- It is fully up to date.
- It is not targeted by a pending deletion job from decreasing replication factor.
- It is assigned. | +| `Verified At` | Indicates last successful verification of the replica by the [verification worker](praefect.md#repository-verification). If the replica has not yet been verified, `unverified` is displayed in place of the last successful verification time. Introduced in GitLab 15.0. | ### Check that repositories are in sync diff --git a/doc/development/feature_flags/index.md b/doc/development/feature_flags/index.md index 4b417b26381..33010c06b1e 100644 --- a/doc/development/feature_flags/index.md +++ b/doc/development/feature_flags/index.md @@ -333,6 +333,17 @@ class MyClass end ``` +#### Recursion detection + +When there are many feature flags, it is not always obvious where they are +called. Avoid cycles where the evaluation of one feature flag requires the +evaluation of other feature flags. If this causes a cycle, it will be broken +and the default value will be returned. + +To enable this recursion detection to work correctly, always access feature values through +`Feature::enabled?`, and avoid the low-level use of `Feature::get`. When this +happens, we track a `Feature::RecursionError` exception to the error tracker. + ### Frontend When using a feature flag for UI elements, make sure to _also_ use a feature diff --git a/doc/user/clusters/agent/install/index.md b/doc/user/clusters/agent/install/index.md index 86093813e43..49d5afc56b1 100644 --- a/doc/user/clusters/agent/install/index.md +++ b/doc/user/clusters/agent/install/index.md @@ -20,7 +20,7 @@ Before you can install the agent in your cluster, you need: - [Google Kubernetes Engine (GKE)](https://cloud.google.com/kubernetes-engine/docs/quickstart) - [Amazon Elastic Kubernetes Service (EKS)](https://docs.aws.amazon.com/eks/latest/userguide/getting-started.html) - [Digital Ocean](https://docs.digitalocean.com/products/kubernetes/quickstart/) -- On self-managed GitLab instances, a GitLab administrator must set up the [agent server](../../../../administration/clusters/kas.md). +- On self-managed GitLab instances, a GitLab administrator must set up the [agent server](../../../../administration/clusters/kas.md). Then it will be available by default at `wss://gitlab.example.com/-/kubernetes-agent/`. On GitLab.com, the agent server is available at `wss://kas.gitlab.com`. ## Installation steps diff --git a/lib/feature.rb b/lib/feature.rb index bcedb556e90..8a911f9210d 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -42,8 +42,10 @@ class Feature flipper.features.to_a end + RecursionError = Class.new(RuntimeError) + def get(key) - flipper.feature(key) + with_feature(key, &:itself) end def persisted_names @@ -82,13 +84,12 @@ class Feature # If `default_enabled: :yaml` we fetch the value from the YAML definition instead. default_enabled = Feature::Definition.default_enabled?(key) if default_enabled == :yaml - # During setup the database does not exist yet. So we haven't stored a value - # for the feature yet and return the default. - return default_enabled unless ApplicationRecord.database.exists? + feature_value = with_feature(key) do |feature| + feature_value = current_feature_value(feature, thing, default_enabled: default_enabled) + end - feature = get(key) - - feature_value = current_feature_value(feature, thing, default_enabled: default_enabled) + # If not yielded, then either recursion is happening, or the database does not exist yet, so use default_enabled. + feature_value = default_enabled if feature_value.nil? # If we don't filter out this flag here we will enter an infinite loop log_feature_flag_state(key, feature_value) if log_feature_flag_states?(key) @@ -103,39 +104,39 @@ class Feature def enable(key, thing = true) log(key: key, action: __method__, thing: thing) - get(key).enable(thing) + with_feature(key) { _1.enable(thing) } end def disable(key, thing = false) log(key: key, action: __method__, thing: thing) - get(key).disable(thing) + with_feature(key) { _1.disable(thing) } end def enable_percentage_of_time(key, percentage) log(key: key, action: __method__, percentage: percentage) - get(key).enable_percentage_of_time(percentage) + with_feature(key) { _1.enable_percentage_of_time(percentage) } end def disable_percentage_of_time(key) log(key: key, action: __method__) - get(key).disable_percentage_of_time + with_feature(key, &:disable_percentage_of_time) end def enable_percentage_of_actors(key, percentage) log(key: key, action: __method__, percentage: percentage) - get(key).enable_percentage_of_actors(percentage) + with_feature(key) { _1.enable_percentage_of_actors(percentage) } end def disable_percentage_of_actors(key) log(key: key, action: __method__) - get(key).disable_percentage_of_actors + with_feature(key, &:disable_percentage_of_actors) end def remove(key) return unless persisted_name?(key) log(key: key, action: __method__) - get(key).remove + with_feature(key, &:remove) end def reset @@ -187,6 +188,42 @@ class Feature feature.enabled?(thing) end + # NOTE: it is not safe to call `Flipper::Feature#enabled?` outside the block + def with_feature(key) + feature = unsafe_get(key) + yield feature if feature.present? + ensure + pop_recursion_stack + end + + def unsafe_get(key) + # During setup the database does not exist yet. So we haven't stored a value + # for the feature yet and return the default. + return unless ApplicationRecord.database.exists? + + flag_stack = ::Thread.current[:feature_flag_recursion_check] || [] + Thread.current[:feature_flag_recursion_check] = flag_stack + + # Prevent more than 10 levels of recursion. This limit was chosen as a fairly + # low limit while allowing some nesting of flag evaluation. We have not seen + # this limit hit in production. + if flag_stack.size > 10 + Gitlab::ErrorTracking.track_exception(RecursionError.new('deep recursion'), stack: flag_stack) + return + elsif flag_stack.include?(key) + Gitlab::ErrorTracking.track_exception(RecursionError.new('self recursion'), stack: flag_stack) + return + end + + flag_stack.push(key) + flipper.feature(key) + end + + def pop_recursion_stack + flag_stack = Thread.current[:feature_flag_recursion_check] + flag_stack.pop if flag_stack + end + def flipper if Gitlab::SafeRequestStore.active? Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance(memoize: true) diff --git a/lib/gitlab/background_migration/backfill_integrations_enable_ssl_verification.rb b/lib/gitlab/background_migration/backfill_integrations_enable_ssl_verification.rb index f2248803588..de52629522b 100644 --- a/lib/gitlab/background_migration/backfill_integrations_enable_ssl_verification.rb +++ b/lib/gitlab/background_migration/backfill_integrations_enable_ssl_verification.rb @@ -27,7 +27,7 @@ module Gitlab }.freeze # Define the `Integration` model - class Integration < ActiveRecord::Base + class Integration < ::ApplicationRecord include IgnorableColumns self.table_name = :integrations diff --git a/lib/gitlab/metrics/subscribers/rack_attack.rb b/lib/gitlab/metrics/subscribers/rack_attack.rb index 70dcc6fad90..e6cf14a6c8c 100644 --- a/lib/gitlab/metrics/subscribers/rack_attack.rb +++ b/lib/gitlab/metrics/subscribers/rack_attack.rb @@ -15,22 +15,6 @@ module Gitlab INSTRUMENTATION_STORE_KEY = :rack_attack_instrumentation - THROTTLES_WITH_USER_INFORMATION = [ - :throttle_authenticated_api, - :throttle_authenticated_web, - :throttle_authenticated_protected_paths_api, - :throttle_authenticated_protected_paths_web, - :throttle_authenticated_packages_api, - :throttle_authenticated_git_lfs, - :throttle_authenticated_files_api, - :throttle_authenticated_deprecated_api - ].freeze - - PAYLOAD_KEYS = [ - :rack_attack_redis_count, - :rack_attack_redis_duration_s - ].freeze - def self.payload Gitlab::SafeRequestStore[INSTRUMENTATION_STORE_KEY] ||= { rack_attack_redis_count: 0, @@ -49,20 +33,20 @@ module Gitlab end def throttle(event) - log_into_auth_logger(event) + log_into_auth_logger(event, status: 429) end def blocklist(event) - log_into_auth_logger(event) + log_into_auth_logger(event, status: 403) end def track(event) - log_into_auth_logger(event) + log_into_auth_logger(event, status: nil) end private - def log_into_auth_logger(event) + def log_into_auth_logger(event, status:) req = event.payload[:request] rack_attack_info = { message: 'Rack_Attack', @@ -73,6 +57,10 @@ module Gitlab matched: req.env['rack.attack.matched'] } + if status + rack_attack_info[:status] = status + end + discriminator = req.env['rack.attack.match_discriminator'].to_s discriminator_id = discriminator.split(':').last diff --git a/lib/gitlab/middleware/go.rb b/lib/gitlab/middleware/go.rb index bfa4e4cf5f8..dcbb4557377 100644 --- a/lib/gitlab/middleware/go.rb +++ b/lib/gitlab/middleware/go.rb @@ -21,6 +21,7 @@ module Gitlab rescue Gitlab::Auth::IpBlacklisted Gitlab::AuthLogger.error( message: 'Rack_Attack', + status: 403, env: :blocklist, remote_ip: request.ip, request_method: request.request_method, diff --git a/rubocop/cop/gitlab/avoid_feature_get.rb b/rubocop/cop/gitlab/avoid_feature_get.rb index e36e0b020c0..1bce89268d9 100644 --- a/rubocop/cop/gitlab/avoid_feature_get.rb +++ b/rubocop/cop/gitlab/avoid_feature_get.rb @@ -5,8 +5,8 @@ module RuboCop module Gitlab # Cop that blacklists the use of `Feature.get`. class AvoidFeatureGet < RuboCop::Cop::Cop - MSG = 'Use `Feature.enable/disable` methods instead of `Feature.get`. ' \ - 'See doc/development/testing_guide/best_practices.md#feature-flags-in-tests for more information.' + MSG = 'Use `stub_feature_flags` method instead of `Feature.get`. ' \ + 'See doc/development/feature_flags/index.md#feature-flags-in-tests for more information.' def_node_matcher :feature_get?, <<~PATTERN (send (const nil? :Feature) :get ...) diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 62cda78e8b9..54d95ff9e05 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Feature, stub_feature_flags: false do expect_any_instance_of(Flipper::DSL).to receive(:feature).with(key) .and_return(feature) - expect(described_class.get(key)).to be(feature) + expect(described_class.get(key)).to eq(feature) end end @@ -159,6 +159,48 @@ RSpec.describe Feature, stub_feature_flags: false do allow(Feature).to receive(:log_feature_flag_states?).and_return(false) end + context 'when self-recursive' do + before do + allow(Feature).to receive(:with_feature).and_wrap_original do |original, name, &block| + original.call(name) do |ff| + Feature.enabled?(name) + block.call(ff) + end + end + end + + it 'returns the default value' do + expect(described_class.enabled?(:ff, default_enabled: true)).to eq true + end + + it 'detects self recursion' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with(have_attributes(message: 'self recursion'), { stack: [:ff] }) + + described_class.enabled?(:ff) + end + end + + context 'when deeply recursive' do + before do + allow(Feature).to receive(:with_feature).and_wrap_original do |original, name, &block| + original.call(name) do |ff| + Feature.enabled?(:"deeper_#{name}") + block.call(ff) + end + end + end + + it 'detects deep recursion' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with(have_attributes(message: 'deep recursion'), stack: have_attributes(size: be > 10)) + + described_class.enabled?(:ff) + end + end + it 'returns false for undefined feature' do expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey end @@ -401,7 +443,7 @@ RSpec.describe Feature, stub_feature_flags: false do end it 'checks the persisted status and returns false' do - expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original + expect(described_class).to receive(:with_feature).with(:non_existent_flag).and_call_original expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false) end diff --git a/spec/lib/gitlab/metrics/subscribers/rack_attack_spec.rb b/spec/lib/gitlab/metrics/subscribers/rack_attack_spec.rb index fda4b94bd78..9f939d0d7d6 100644 --- a/spec/lib/gitlab/metrics/subscribers/rack_attack_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/rack_attack_spec.rb @@ -77,8 +77,8 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do end it 'logs request information' do - expect(Gitlab::AuthLogger).to receive(:error).with( - include( + expect(Gitlab::AuthLogger).to receive(:error) do |arguments| + expect(arguments).to include( message: 'Rack_Attack', env: match_type, remote_ip: '1.2.3.4', @@ -86,7 +86,14 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do path: '/api/v4/internal/authorized_keys', matched: 'throttle_unauthenticated' ) - ) + + if expected_status + expect(arguments).to include(status: expected_status) + else + expect(arguments).not_to have_key(:status) + end + end + subscriber.send(match_type, event) end end @@ -111,8 +118,8 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do end it 'logs request information and user id' do - expect(Gitlab::AuthLogger).to receive(:error).with( - include( + expect(Gitlab::AuthLogger).to receive(:error) do |arguments| + expect(arguments).to include( message: 'Rack_Attack', env: match_type, remote_ip: '1.2.3.4', @@ -121,7 +128,14 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do matched: 'throttle_authenticated_api', user_id: non_existing_record_id ) - ) + + if expected_status + expect(arguments).to include(status: expected_status) + else + expect(arguments).not_to have_key(:status) + end + end + subscriber.send(match_type, event) end end @@ -145,8 +159,8 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do end it 'logs request information and user meta' do - expect(Gitlab::AuthLogger).to receive(:error).with( - include( + expect(Gitlab::AuthLogger).to receive(:error) do |arguments| + expect(arguments).to include( message: 'Rack_Attack', env: match_type, remote_ip: '1.2.3.4', @@ -156,7 +170,14 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do user_id: user.id, 'meta.user' => user.username ) - ) + + if expected_status + expect(arguments).to include(status: expected_status) + else + expect(arguments).not_to have_key(:status) + end + end + subscriber.send(match_type, event) end end @@ -182,8 +203,8 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do end it 'logs request information and user meta' do - expect(Gitlab::AuthLogger).to receive(:error).with( - include( + expect(Gitlab::AuthLogger).to receive(:error) do |arguments| + expect(arguments).to include( message: 'Rack_Attack', env: match_type, remote_ip: '1.2.3.4', @@ -192,7 +213,14 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do matched: 'throttle_authenticated_api', deploy_token_id: deploy_token.id ) - ) + + if expected_status + expect(arguments).to include(status: expected_status) + else + expect(arguments).not_to have_key(:status) + end + end + subscriber.send(match_type, event) end end @@ -202,6 +230,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do describe '#throttle' do let(:match_type) { :throttle } + let(:expected_status) { 429 } let(:event_name) { 'throttle.rack_attack' } it_behaves_like 'log into auth logger' @@ -209,6 +238,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do describe '#blocklist' do let(:match_type) { :blocklist } + let(:expected_status) { 403 } let(:event_name) { 'blocklist.rack_attack' } it_behaves_like 'log into auth logger' @@ -216,6 +246,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do describe '#track' do let(:match_type) { :track } + let(:expected_status) { nil } let(:event_name) { 'track.rack_attack' } it_behaves_like 'log into auth logger' diff --git a/spec/models/concerns/sha256_attribute_spec.rb b/spec/models/concerns/sha256_attribute_spec.rb deleted file mode 100644 index 02947325bf4..00000000000 --- a/spec/models/concerns/sha256_attribute_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Sha256Attribute do - let(:model) { Class.new(ApplicationRecord) { include Sha256Attribute } } - - before do - columns = [ - double(:column, name: 'name', type: :text), - double(:column, name: 'sha256', type: :binary) - ] - - allow(model).to receive(:columns).and_return(columns) - end - - describe '#sha_attribute' do - context 'when in non-production' do - before do - stub_rails_env('development') - end - - context 'when the table exists' do - before do - allow(model).to receive(:table_exists?).and_return(true) - end - - it 'defines a SHA attribute for a binary column' do - expect(model).to receive(:attribute) - .with(:sha256, an_instance_of(Gitlab::Database::Sha256Attribute)) - - model.sha256_attribute(:sha256) - end - - it 'raises ArgumentError when the column type is not :binary' do - expect { model.sha256_attribute(:name) }.to raise_error(ArgumentError) - end - end - - context 'when the table does not exist' do - it 'allows the attribute to be added and issues a warning' do - allow(model).to receive(:table_exists?).and_return(false) - - expect(model).not_to receive(:columns) - expect(model).to receive(:attribute) - expect(model).to receive(:warn) - - model.sha256_attribute(:name) - end - end - - context 'when the column does not exist' do - it 'allows the attribute to be added and issues a warning' do - allow(model).to receive(:table_exists?).and_return(true) - - expect(model).to receive(:columns) - expect(model).to receive(:attribute) - expect(model).to receive(:warn) - - model.sha256_attribute(:no_name) - end - end - - context 'when other execeptions are raised' do - it 'logs and re-rasises the error' do - allow(model).to receive(:table_exists?).and_raise(ActiveRecord::NoDatabaseError.new('does not exist')) - - expect(model).not_to receive(:columns) - expect(model).not_to receive(:attribute) - expect(Gitlab::AppLogger).to receive(:error) - - expect { model.sha256_attribute(:name) }.to raise_error(ActiveRecord::NoDatabaseError) - end - end - end - - context 'when in production' do - before do - stub_rails_env('production') - end - - it 'defines a SHA attribute' do - expect(model).not_to receive(:table_exists?) - expect(model).not_to receive(:columns) - expect(model).to receive(:attribute).with(:sha256, an_instance_of(Gitlab::Database::Sha256Attribute)) - - model.sha256_attribute(:sha256) - end - end - end -end diff --git a/spec/models/concerns/sha_attribute_spec.rb b/spec/models/concerns/sha_attribute_spec.rb index 1bcf3dc8b61..790e6936803 100644 --- a/spec/models/concerns/sha_attribute_spec.rb +++ b/spec/models/concerns/sha_attribute_spec.rb @@ -3,86 +3,101 @@ require 'spec_helper' RSpec.describe ShaAttribute do - let(:model) { Class.new(ActiveRecord::Base) { include ShaAttribute } } + let(:model) do + Class.new(ActiveRecord::Base) do + include ShaAttribute - before do - columns = [ - double(:column, name: 'name', type: :text), - double(:column, name: 'sha1', type: :binary) - ] - - allow(model).to receive(:columns).and_return(columns) + self.table_name = 'merge_requests' + end end - describe '#sha_attribute' do - context 'when in development' do - before do - stub_rails_env('development') - end + let(:binary_column) { :merge_ref_sha } + let(:text_column) { :target_branch } - context 'when the table exists' do - before do - allow(model).to receive(:table_exists?).and_return(true) - end + describe '.sha_attribute' do + it 'defines a SHA attribute with Gitlab::Database::ShaAttribute type' do + expect(model).to receive(:attribute) + .with(binary_column, an_instance_of(Gitlab::Database::ShaAttribute)) + .and_call_original - it 'defines a SHA attribute for a binary column' do - expect(model).to receive(:attribute) - .with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute)) + model.sha_attribute(binary_column) + end + end - model.sha_attribute(:sha1) - end + describe '.sha256_attribute' do + it 'defines a SHA256 attribute with Gitlab::Database::ShaAttribute type' do + expect(model).to receive(:attribute) + .with(binary_column, an_instance_of(Gitlab::Database::Sha256Attribute)) + .and_call_original - it 'raises ArgumentError when the column type is not :binary' do - expect { model.sha_attribute(:name) }.to raise_error(ArgumentError) - end - end + model.sha256_attribute(binary_column) + end + end - context 'when the table does not exist' do - it 'allows the attribute to be added' do - allow(model).to receive(:table_exists?).and_return(false) + describe '.load_schema!' do + # load_schema! is not a documented class method, so use a documented method + # that we know will call load_schema! + def load_schema! + expect(model).to receive(:load_schema!).and_call_original - expect(model).not_to receive(:columns) - expect(model).to receive(:attribute) - - model.sha_attribute(:name) - end - end - - context 'when the column does not exist' do - it 'allows the attribute to be added' do - allow(model).to receive(:table_exists?).and_return(true) - - expect(model).to receive(:columns) - expect(model).to receive(:attribute) - - model.sha_attribute(:no_name) - end - end - - context 'when other execeptions are raised' do - it 'logs and re-rasises the error' do - allow(model).to receive(:table_exists?).and_raise(ActiveRecord::NoDatabaseError.new('does not exist')) - - expect(model).not_to receive(:columns) - expect(model).not_to receive(:attribute) - expect(Gitlab::AppLogger).to receive(:error) - - expect { model.sha_attribute(:name) }.to raise_error(ActiveRecord::NoDatabaseError) - end - end + model.new end - context 'when in production' do + using RSpec::Parameterized::TableSyntax + + where(:column_name, :environment, :expected_error) do + ref(:binary_column) | 'development' | :no_error + ref(:binary_column) | 'production' | :no_error + ref(:text_column) | 'development' | :sha_mismatch_error + ref(:text_column) | 'production' | :no_error + :__non_existent_column | 'development' | :no_error + :__non_existent_column | 'production' | :no_error + end + + let(:sha_mismatch_error) do + [ + described_class::ShaAttributeTypeMismatchError, + /#{column_name}.* should be a :binary column/ + ] + end + + with_them do before do - stub_rails_env('production') + stub_rails_env(environment) end - it 'defines a SHA attribute' do - expect(model).not_to receive(:table_exists?) - expect(model).not_to receive(:columns) - expect(model).to receive(:attribute).with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute)) + context 'with sha_attribute' do + before do + model.sha_attribute(column_name) + end - model.sha_attribute(:sha1) + it 'validates column type' do + if expected_error == :no_error + expect { load_schema! }.not_to raise_error + elsif expected_error == :sha_mismatch_error + expect { load_schema! }.to raise_error( + described_class::ShaAttributeTypeMismatchError, + /sha_attribute.*#{column_name}.* should be a :binary column/ + ) + end + end + end + + context 'with sha256_attribute' do + before do + model.sha256_attribute(column_name) + end + + it 'validates column type' do + if expected_error == :no_error + expect { load_schema! }.not_to raise_error + elsif expected_error == :sha_mismatch_error + expect { load_schema! }.to raise_error( + described_class::Sha256AttributeTypeMismatchError, + /sha256_attribute.*#{column_name}.* should be a :binary column/ + ) + end + end end end end diff --git a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb index 68cb91d7414..d4417b23a5f 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb @@ -149,6 +149,7 @@ RSpec.shared_examples 'rate-limited token requests' do arguments = a_hash_including({ message: 'Rack_Attack', + status: 429, env: :throttle, remote_ip: '127.0.0.1', request_method: request_method, @@ -314,6 +315,7 @@ RSpec.shared_examples 'rate-limited web authenticated requests' do arguments = a_hash_including({ message: 'Rack_Attack', + status: 429, env: :throttle, remote_ip: '127.0.0.1', request_method: request_method, @@ -391,14 +393,16 @@ RSpec.shared_examples 'tracking when dry-run mode is set' do end it 'logs RackAttack info into structured logs' do - arguments = a_hash_including({ - message: 'Rack_Attack', - env: :track, - remote_ip: '127.0.0.1', - matched: throttle_name - }) + expect(Gitlab::AuthLogger).to receive(:error) do |arguments| + expect(arguments).to include( + message: 'Rack_Attack', + env: :track, + remote_ip: '127.0.0.1', + matched: throttle_name + ) - expect(Gitlab::AuthLogger).to receive(:error).with(arguments) + expect(arguments).not_to have_key(:status) + end (1 + requests_per_period).times do do_request @@ -576,6 +580,7 @@ RSpec.shared_examples 'rate-limited unauthenticated requests' do arguments = a_hash_including({ message: 'Rack_Attack', + status: 429, env: :throttle, remote_ip: '127.0.0.1', request_method: 'GET',