diff options
41 files changed, 355 insertions, 312 deletions
diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml index 20903904144..1ae169a437a 100644 --- a/.rubocop_todo/rspec/feature_category.yml +++ b/.rubocop_todo/rspec/feature_category.yml @@ -2933,8 +2933,6 @@ RSpec/FeatureCategory: - 'spec/lib/gitlab/ci/config/entry/retry_spec.rb' - 'spec/lib/gitlab/ci/config/entry/root_spec.rb' - 'spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb' - - 'spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb' - - 'spec/lib/gitlab/ci/config/entry/rules_spec.rb' - 'spec/lib/gitlab/ci/config/entry/service_spec.rb' - 'spec/lib/gitlab/ci/config/entry/services_spec.rb' - 'spec/lib/gitlab/ci/config/entry/stage_spec.rb' diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fca3bb3460f..d7b005d03b5 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -26,7 +26,6 @@ class ApplicationController < BaseActionController include CheckRateLimit include RequestPayloadLogger - before_action :limit_session_time, if: -> { !current_user } before_action :authenticate_user!, except: [:route_not_found] before_action :enforce_terms!, if: :should_enforce_terms? before_action :check_password_expiration, if: :html_request? @@ -51,7 +50,6 @@ class ApplicationController < BaseActionController around_action :set_current_admin after_action :set_page_title_header, if: :json_request? - after_action :ensure_authenticated_session_time, if: -> { current_user } protect_from_forgery with: :exception, prepend: true diff --git a/app/graphql/types/commit_signatures/verification_status_enum.rb b/app/graphql/types/commit_signatures/verification_status_enum.rb index 4a3032ad244..d0d8f6670c3 100644 --- a/app/graphql/types/commit_signatures/verification_status_enum.rb +++ b/app/graphql/types/commit_signatures/verification_status_enum.rb @@ -8,7 +8,7 @@ module Types graphql_name 'VerificationStatus' description 'Verification status of a GPG, X.509 or SSH signature for a commit.' - ::Enums::CommitSignature.verification_statuses.each do |status, _| + ::Enums::CommitSignature.verification_statuses.each_key do |status| value status.to_s.upcase, value: status.to_s, description: "#{status} verification status." end end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index cf5cc92587f..e93f95beb4f 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -14,32 +14,6 @@ module SessionsHelper flash[:alert] == t(:unconfirmed, scope: [:devise, :failure]) end - # By default, all sessions are given the same expiration time configured in - # the session store (e.g. 1 week). However, unauthenticated users can - # generate a lot of sessions, primarily for CSRF verification. It makes - # sense to reduce the TTL for unauthenticated to something much lower than - # the default (e.g. 1 hour) to limit Redis memory. In addition, Rails - # creates a new session after login, so the short TTL doesn't even need to - # be extended. - def limit_session_time - set_session_time(Settings.gitlab['unauthenticated_session_expire_delay']) - end - - def ensure_authenticated_session_time - set_session_time(nil) - end - - def set_session_time(expiry_s) - # Rack sets this header, but not all tests may have it: https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L251-L259 - return unless request.env['rack.session.options'] - - # This works because Rack uses these options every time a request is handled, and redis-store - # uses the Rack setting first: - # 1. https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342 - # 2. https://github.com/redis-store/redis-store/blob/3acfa95f4eb6260c714fdb00a3d84be8eedc13b2/lib/redis/store/ttl.rb#L32 - request.env['rack.session.options'][:expire_after] = expiry_s - end - def obfuscated_email(email) # Moved to Gitlab::Utils::Email in 15.9 Gitlab::Utils::Email.obfuscated_email(email) diff --git a/app/serializers/diffs_metadata_entity.rb b/app/serializers/diffs_metadata_entity.rb index e55f31a8376..b840f3acb88 100644 --- a/app/serializers/diffs_metadata_entity.rb +++ b/app/serializers/diffs_metadata_entity.rb @@ -15,6 +15,10 @@ class DiffsMetadataEntity < DiffsEntity presenter(options[:merge_request]).conflict_resolution_path end + # #cannot_be_merged? is generally indicative of conflicts, and is set via + # MergeRequests::MergeabilityCheckService. However, it can also indicate + # that either #has_no_commits? or #branch_missing? are true. + # expose :has_conflicts do |_, options| options[:merge_request].cannot_be_merged? end diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb index 575a6bfe95a..4e6f117e9fb 100644 --- a/app/services/merge_requests/conflicts/list_service.rb +++ b/app/services/merge_requests/conflicts/list_service.rb @@ -15,6 +15,10 @@ module MergeRequests def can_be_resolved_in_ui? return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui) + # #cannot_be_merged? is generally indicative of conflicts, and is set via + # MergeRequests::MergeabilityCheckService. However, it can also indicate + # that either #has_no_commits? or #branch_missing? are true. + # return @conflicts_can_be_resolved_in_ui = false unless merge_request.cannot_be_merged? return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs? return @conflicts_can_be_resolved_in_ui = false if merge_request.branch_missing? diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 733ad94240a..7f410d7bf7b 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -31,12 +31,16 @@ cookie_key = if Rails.env.development? store = Gitlab::Redis::Sessions.store(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE) -Gitlab::Application.config.session_store( - :redis_store, # Using the cookie_store would enable session replay attacks. - redis_store: store, - key: cookie_key, - secure: Gitlab.config.gitlab.https, - httponly: true, - expires_in: Settings.gitlab['session_expire_delay'] * 60, - path: Rails.application.config.relative_url_root.presence || '/' -) +Rails.application.configure do + config.session_store( + :redis_store, # Using the cookie_store would enable session replay attacks. + redis_store: store, + key: cookie_key, + secure: Gitlab.config.gitlab.https, + httponly: true, + expires_in: Settings.gitlab['session_expire_delay'] * 60, + path: Rails.application.config.relative_url_root.presence || '/' + ) + + config.middleware.insert_after ActionDispatch::Session::RedisStore, Gitlab::Middleware::UnauthenticatedSessionExpiry +end diff --git a/doc/architecture/blueprints/cloud_connector/index.md b/doc/architecture/blueprints/cloud_connector/index.md index 9aef8bc7a98..50e233a6089 100644 --- a/doc/architecture/blueprints/cloud_connector/index.md +++ b/doc/architecture/blueprints/cloud_connector/index.md @@ -170,16 +170,16 @@ It will have the following responsibilities: We suggest to use one of the following language stacks: 1. **Go.** There is substantial organizational knowledge in writing and running -Go systems at GitLab, and it is a great systems language that gives us efficient ways to handle requests where -they merely need to be forwarded (request proxying) and a powerful concurrency mechanism through goroutines. This makes the -service easier to scale and cheaper to run than Ruby or Python, which scale largely at the process level due to their use -of Global Interpreter Locks, and use inefficient memory models especially as regards byte stream handling and manipulation. -A drawback of Go is that resource requirements such as memory use are less predictable because Go is a garbage collected language. + Go systems at GitLab, and it is a great systems language that gives us efficient ways to handle requests where + they merely need to be forwarded (request proxying) and a powerful concurrency mechanism through goroutines. This makes the + service easier to scale and cheaper to run than Ruby or Python, which scale largely at the process level due to their use + of Global Interpreter Locks, and use inefficient memory models especially as regards byte stream handling and manipulation. + A drawback of Go is that resource requirements such as memory use are less predictable because Go is a garbage collected language. 1. **Rust.** We are starting to build up knowledge in Rust at GitLab. Like Go, it is a great systems language that is -also starting to see wider adoption in the Ruby ecosystem to write CRuby extensions. A major benefit is more predictable -resource consumption because it is not garbage collected and allows for finer control of memory use. -It is also very fast; we found that the Rust implementation for `prometheus-client-mmap` outperformed the original -extension written in C. + also starting to see wider adoption in the Ruby ecosystem to write CRuby extensions. A major benefit is more predictable + resource consumption because it is not garbage collected and allows for finer control of memory use. + It is also very fast; we found that the Rust implementation for `prometheus-client-mmap` outperformed the original + extension written in C. ## Alternative solutions diff --git a/doc/architecture/blueprints/gitaly_adaptive_concurrency_limit/index.md b/doc/architecture/blueprints/gitaly_adaptive_concurrency_limit/index.md index f3335a0935e..7f451b4f92b 100644 --- a/doc/architecture/blueprints/gitaly_adaptive_concurrency_limit/index.md +++ b/doc/architecture/blueprints/gitaly_adaptive_concurrency_limit/index.md @@ -43,14 +43,14 @@ configurations, especially the value of the concurrency limit, are static. There are some drawbacks to this: - It's tedious to maintain a sane value for the concurrency limit. Looking at -this [production configuration](https://gitlab.com/gitlab-com/gl-infra/chef-repo/-/blob/db11ef95859e42d656bb116c817402635e946a32/roles/gprd-base-stor-gitaly-common.json), -each limit is heavily calibrated based on clues from different sources. When the -overall scene changes, we need to tweak them again. + this [production configuration](https://gitlab.com/gitlab-com/gl-infra/chef-repo/-/blob/db11ef95859e42d656bb116c817402635e946a32/roles/gprd-base-stor-gitaly-common.json), + each limit is heavily calibrated based on clues from different sources. When the + overall scene changes, we need to tweak them again. - Static limits are not good for all usage patterns. It's not feasible to pick a -fit-them-all value. If the limit is too low, big users will be affected. If the -value is too loose, the protection effect is lost. + fit-them-all value. If the limit is too low, big users will be affected. If the + value is too loose, the protection effect is lost. - A request may be rejected even though the server is idle as the rate is not -necessarily an indicator of the load induced on the server. + necessarily an indicator of the load induced on the server. To overcome all of those drawbacks while keeping the benefits of concurrency limiting, one promising solution is to make the concurrency limit adaptive to @@ -78,18 +78,18 @@ occurs. There are various criteria for determining whether Gitaly is in trouble. In this proposal, we focus on two things: - Lack of resources, particularly memory and CPU, which are essential for -handling Git processes. + handling Git processes. - Serious latency degradation. The proposed solution is heavily inspired by many materials about this subject shared by folks from other companies in the industry, especially the following: - TCP Congestion Control ([RFC-2581](https://www.rfc-editor.org/rfc/rfc2581), [RFC-5681](https://www.rfc-editor.org/rfc/rfc5681), -[RFC-9293](https://www.rfc-editor.org/rfc/rfc9293.html#name-tcp-congestion-control), [Computer Networks: A Systems Approach](https://book.systemsapproach.org/congestion/tcpcc.html)). + [RFC-9293](https://www.rfc-editor.org/rfc/rfc9293.html#name-tcp-congestion-control), [Computer Networks: A Systems Approach](https://book.systemsapproach.org/congestion/tcpcc.html)). - Netflix adaptive concurrency limit ([blog post](https://tech.olx.com/load-shedding-with-nginx-using-adaptive-concurrency-control-part-1-e59c7da6a6df) -and [implementation](https://github.com/Netflix/concurrency-limits)) + and [implementation](https://github.com/Netflix/concurrency-limits)) - Envoy Adaptive Concurrency -([doc](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/adaptive_concurrency_filter#config-http-filters-adaptive-concurrency)) + ([doc](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/adaptive_concurrency_filter#config-http-filters-adaptive-concurrency)) We cannot blindly apply a solution without careful consideration and expect it to function flawlessly. The suggested approach considers Gitaly's specific @@ -116,12 +116,11 @@ process functioning but quickly reducing it when an issue occurs. During initialization, we configure the following parameters: - `initialLimit`: Concurrency limit to start with. This value is essentially -equal to the current static concurrency limit. + equal to the current static concurrency limit. - `maxLimit`: Maximum concurrency limit. - `minLimit`: Minimum concurrency limit so that the process is considered as -functioning. If it's equal to 0, it rejects all upcoming requests. -- `backoffFactor`: how fast the limit decreases when a backoff event occurs (`0 -< backoff < 1`, default to `0.75`) + functioning. If it's equal to 0, it rejects all upcoming requests. +- `backoffFactor`: how fast the limit decreases when a backoff event occurs (`0 < backoff < 1`, default to `0.75`) When the Gitaly process starts, it sets `limit = initialLimit`, in which `limit` is the maximum in-flight requests allowed at a time. @@ -130,9 +129,9 @@ Periodically, maybe once per 15 seconds, the value of the `limit` is re-calibrated: - `limit = limit + 1` if there is no backoff event since the last -calibration. The new limit cannot exceed `maxLimit`. + calibration. The new limit cannot exceed `maxLimit`. - `limit = limit * backoffFactor` otherwise. The new limit cannot be lower than -`minLimit`. + `minLimit`. When a process can no longer handle requests or will not be able to handle them soon, it is referred to as a back-off event. Ideally, we would love to see the @@ -151,16 +150,16 @@ The concurrency limit restricts the total number of in-flight requests (IFR) at a time. - When `IFR < limit`, Gitaly handles new requests without waiting. After an -increment, Gitaly immediately handles the subsequent request in the queue, if -any. + increment, Gitaly immediately handles the subsequent request in the queue, if + any. - When `IFR = limit`, it means the limit is reached. Subsequent requests are -queued, waiting for their turn. If the queue length reaches a configured limit, -Gitaly rejects new requests immediately. When a request stays in the queue long -enough, it is also automatically dropped by Gitaly. + queued, waiting for their turn. If the queue length reaches a configured limit, + Gitaly rejects new requests immediately. When a request stays in the queue long + enough, it is also automatically dropped by Gitaly. - When `IRF > limit`, it's appropriately a consequence of backoff events. It -means Gitaly handles more requests than the newly appointed limits. In addition -to queueing upcoming requests similarly to the above case, Gitaly may start -load-shedding in-flight requests if this situation is not resolved long enough. + means Gitaly handles more requests than the newly appointed limits. In addition + to queueing upcoming requests similarly to the above case, Gitaly may start + load-shedding in-flight requests if this situation is not resolved long enough. At several points in time we have discussed whether we want to change queueing semantics. Right now we admit queued processes from the head of the queue @@ -181,16 +180,16 @@ Each system has its own set of signals, and in the case of Gitaly, there are two aspects to consider: - Lack of resources, particularly memory and CPU, which are essential for -handling Git processes like `git-pack-objects(1)`. When these resources are limited -or depleted, it doesn't make sense for Gitaly to accept more requests. Doing so -would worsen the saturation, and Gitaly addresses this issue by applying cgroups -extensively. The following section outlines how accounting can be carried out -using cgroup. + handling Git processes like `git-pack-objects(1)`. When these resources are limited + or depleted, it doesn't make sense for Gitaly to accept more requests. Doing so + would worsen the saturation, and Gitaly addresses this issue by applying cgroups + extensively. The following section outlines how accounting can be carried out + using cgroup. - Serious latency degradation. Gitaly offers various RPCs for different purposes -besides serving Git data that is hard to reason about latencies. A significant -overall latency decline is an indication that Gitaly should not accept more -requests. Another section below describes how to assert latency degradation -reasonably. + besides serving Git data that is hard to reason about latencies. A significant + overall latency decline is an indication that Gitaly should not accept more + requests. Another section below describes how to assert latency degradation + reasonably. Apart from the above signals, we can consider adding more signals in the future to make the system smarter. Some examples are Go garbage collector statistics, diff --git a/doc/architecture/blueprints/runway/index.md b/doc/architecture/blueprints/runway/index.md index becb7914feb..af7f466cdc9 100644 --- a/doc/architecture/blueprints/runway/index.md +++ b/doc/architecture/blueprints/runway/index.md @@ -169,7 +169,7 @@ In order for runway to function, there are two JSON/YAML documents in use. They 1. The Runway Inventory Model. This covers what service projects are currently onboarded into Runway. It's located [here](https://gitlab.com/gitlab-com/gl-infra/platform/runway/provisioner/-/blob/main/inventory.json?ref_type=heads). The schema used to validate the docuemnt is located [here](https://gitlab.com/gitlab-com/gl-infra/platform/runway/runwayctl/-/blob/main/schemas/service-inventory/v1.0.0-beta/inventory.schema.json?ref_type=heads). There is no backwards compatibility guarenteed to changes to this document schema. This is because it's only used internally by the Runway team, and there is only a single document actually being used by Runway to provision/deprovision Runway services. 1. The runway Service Model. This is used by Runway users to pass through configuration needed to Runway in order to deploy their service. It's located inside their Service project, at `.runway/runway.yml`. [An example is here](https://gitlab.com/gitlab-org/modelops/applied-ml/code-suggestions/ai-assist/-/blob/main/.runway/runway.yml?ref_type=heads). The schema used to validate the document is located [here](https://gitlab.com/gitlab-com/gl-infra/platform/runway/runwayctl/-/blob/main/schemas/service-manifest/v1.0.0-beta/manifest.schema.json?ref_type=heads). We aim to continue to make improvements and changes to the model, but all changes to the model within the same `kind/apiVersion` must be backwards compatible. In order to -make breaking changes, a new `apiVersion` of the schema will be released. The overall goal is to copy the [Kubernetes model for making API changes](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md). + make breaking changes, a new `apiVersion` of the schema will be released. The overall goal is to copy the [Kubernetes model for making API changes](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md). There are also [GitLab CI templates](https://gitlab.com/gitlab-com/gl-infra/platform/runway/ci-tasks) used by Runway users in order to automate deployments via Runway through GitLab CI. Users will be encouraged to use tools such as [Renovate bot](https://gitlab.com/gitlab-com/gl-infra/common-ci-tasks/-/blob/main/renovate-bot.md) in order to make sure the CI templates and version of Runway they are using is up to date. The Runway team will support all released versions of Runway, with the exception of when a security issue is identified. When this happens, Runway users will be expected to update to a version of Runway that contains a fix for the issue as soon as possible (once notification is received). diff --git a/doc/development/activitypub/actors/index.md b/doc/development/activitypub/actors/index.md index 6d82e79b9a0..e0367d71871 100644 --- a/doc/development/activitypub/actors/index.md +++ b/doc/development/activitypub/actors/index.md @@ -72,7 +72,7 @@ render json: ActivityPub::ReleasesActorSerializer.new.represent(project, opts) ``` - `outbox` is the endpoint where to find the activities feed for this -actor. + actor. - `inbox` is where to POST to subscribe to the feed. Not yet implemented, so pass `nil`. ## Outbox page diff --git a/doc/development/database_review.md b/doc/development/database_review.md index ec4c4f9f2c5..f4fd81f85ec 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -113,7 +113,7 @@ the following preparations into account. #### Preparation when adding migrations - Ensure `db/structure.sql` is updated as [documented](migration_style_guide.md#schema-changes), and additionally ensure that the relevant version files under -`db/schema_migrations` were added or removed. + `db/schema_migrations` were added or removed. - Ensure that the Database Dictionary is updated as [documented](database/database_dictionary.md). - Make migrations reversible by using the `change` method or include a `down` method when using `up`. - Include either a rollback procedure or describe how to rollback changes. diff --git a/doc/development/lfs.md b/doc/development/lfs.md index 289c258fafe..2fb7d20cabc 100644 --- a/doc/development/lfs.md +++ b/doc/development/lfs.md @@ -180,32 +180,32 @@ sequenceDiagram 1. The user requests the project archive from the UI. 1. Workhorse forwards this request to Rails. 1. If the user is authorized to download the archive, Rails replies with -an HTTP header of `Gitlab-Workhorse-Send-Data` with a base64-encoded -JSON payload prefaced with `git-archive`. This payload includes the -`SendArchiveRequest` binary message, which is encoded again in base64. + an HTTP header of `Gitlab-Workhorse-Send-Data` with a base64-encoded + JSON payload prefaced with `git-archive`. This payload includes the + `SendArchiveRequest` binary message, which is encoded again in base64. 1. Workhorse decodes the `Gitlab-Workhorse-Send-Data` payload. If the -archive already exists in the archive cache, Workhorse sends that -file. Otherwise, Workhorse sends the `SendArchiveRequest` to the -appropriate Gitaly server. + archive already exists in the archive cache, Workhorse sends that + file. Otherwise, Workhorse sends the `SendArchiveRequest` to the + appropriate Gitaly server. 1. The Gitaly server calls `git archive <ref>` to begin generating -the Git archive on-the-fly. If the `include_lfs_blobs` flag is enabled, -Gitaly enables a custom LFS smudge filter via the `-c -filter.lfs.smudge=/path/to/gitaly-lfs-smudge` Git option. + the Git archive on-the-fly. If the `include_lfs_blobs` flag is enabled, + Gitaly enables a custom LFS smudge filter via the `-c + filter.lfs.smudge=/path/to/gitaly-lfs-smudge` Git option. 1. When `git` identifies a possible LFS pointer using the -`.gitattributes` file, `git` calls `gitaly-lfs-smudge` and provides the -LFS pointer via the standard input. Gitaly provides `GL_PROJECT_PATH` -and `GL_INTERNAL_CONFIG` as environment variables to enable lookup of -the LFS object. + `.gitattributes` file, `git` calls `gitaly-lfs-smudge` and provides the + LFS pointer via the standard input. Gitaly provides `GL_PROJECT_PATH` + and `GL_INTERNAL_CONFIG` as environment variables to enable lookup of + the LFS object. 1. If a valid LFS pointer is decoded, `gitaly-lfs-smudge` makes an -internal API call to Workhorse to download the LFS object from GitLab. + internal API call to Workhorse to download the LFS object from GitLab. 1. Workhorse forwards this request to Rails. If the LFS object exists -and is associated with the project, Rails sends `ArchivePath` either -with a path where the LFS object resides (for local disk) or a -pre-signed URL (when object storage is enabled) via the -`Gitlab-Workhorse-Send-Data` HTTP header with a payload prefaced with -`send-url`. + and is associated with the project, Rails sends `ArchivePath` either + with a path where the LFS object resides (for local disk) or a + pre-signed URL (when object storage is enabled) via the + `Gitlab-Workhorse-Send-Data` HTTP header with a payload prefaced with + `send-url`. 1. Workhorse retrieves the file and send it to the `gitaly-lfs-smudge` -process, which writes the contents to the standard output. + process, which writes the contents to the standard output. 1. `git` reads this output and sends it back to the Gitaly process. 1. Gitaly sends the data back to Rails. 1. The archive data is sent back to the client. diff --git a/doc/development/multi_version_compatibility.md b/doc/development/multi_version_compatibility.md index 40a30aa4926..716e42655d0 100644 --- a/doc/development/multi_version_compatibility.md +++ b/doc/development/multi_version_compatibility.md @@ -234,7 +234,7 @@ And these deployments align perfectly with application changes. 1. At the beginning we have `Version N` on `Schema A`. 1. Then we have a _long_ transition period with both `Version N` and `Version N+1` on `Schema B`. 1. When we only have `Version N+1` on `Schema B` the schema changes again. -1. Finally we have `Version N+1` on `Schema C`. +1. Finally we have `Version N+1` on `Schema C`. With all those details in mind, let's imagine we need to replace a query, and this query has an index to support it. @@ -310,10 +310,10 @@ variable `CI_NODE_TOTAL` being an integer failed. This was caused because after 1. New code: Sidekiq created a new pipeline and new build. `build.options[:parallel]` is a `Hash`. 1. Old code: Runners requested a job from an API node that is running the previous version. 1. As a result, the [new code](https://gitlab.com/gitlab-org/gitlab/-/blob/42b82a9a3ac5a96f9152aad6cbc583c42b9fb082/app/models/concerns/ci/contextable.rb#L104) -was not run on the API server. The runner's request failed because the -older API server tried return the `CI_NODE_TOTAL` CI/CD variable, but -instead of sending an integer value (for example, 9), it sent a serialized -`Hash` value (`{:number=>9, :total=>9}`). + was not run on the API server. The runner's request failed because the + older API server tried return the `CI_NODE_TOTAL` CI/CD variable, but + instead of sending an integer value (for example, 9), it sent a serialized + `Hash` value (`{:number=>9, :total=>9}`). If you look at the [deployment pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/deployer/-/pipelines/202212), you see all nodes were updated in parallel: @@ -322,11 +322,11 @@ you see all nodes were updated in parallel: However, even though the updated started around the same time, the completion time varied significantly: -|Node type|Duration (min)| -|---------|--------------| -|API |54 | -|Sidekiq |21 | -|K8S |8 | +| Node type | Duration (min) | +|-----------|----------------| +| API | 54 | +| Sidekiq | 21 | +| K8S | 8 | Builds that used the `parallel` keyword and depended on `CI_NODE_TOTAL` and `CI_NODE_INDEX` would fail during the time after Sidekiq was diff --git a/doc/development/reactive_caching.md b/doc/development/reactive_caching.md index 00110d21dc0..dfe1e2d1b05 100644 --- a/doc/development/reactive_caching.md +++ b/doc/development/reactive_caching.md @@ -255,14 +255,14 @@ self.reactive_cache_hard_limit = 5.megabytes #### `self.reactive_cache_work_type` - This is the type of work performed by the `calculate_reactive_cache` method. Based on this attribute, -it's able to pick the right worker to process the caching job. Make sure to -set it as `:external_dependency` if the work performs any external request -(for example, Kubernetes, Sentry); otherwise set it to `:no_dependency`. + it's able to pick the right worker to process the caching job. Make sure to + set it as `:external_dependency` if the work performs any external request + (for example, Kubernetes, Sentry); otherwise set it to `:no_dependency`. #### `self.reactive_cache_worker_finder` - This is the method used by the background worker to find or generate the object on -which `calculate_reactive_cache` can be called. + which `calculate_reactive_cache` can be called. - By default it uses the model primary key to find the object: ```ruby diff --git a/doc/development/ruby_upgrade.md b/doc/development/ruby_upgrade.md index 110bc6076b0..d1dd6d793d4 100644 --- a/doc/development/ruby_upgrade.md +++ b/doc/development/ruby_upgrade.md @@ -50,31 +50,31 @@ To help you estimate the scope of future upgrades, see the efforts required for Before any upgrade, consider all audiences and targets, ordered by how immediately they are affected by Ruby upgrades: 1. **Developers.** We have many contributors to GitLab and related projects both inside and outside the company. Changing files such as `.ruby-version` affects everyone using tooling that interprets these files. -The developers are affected as soon as they pull from the repository containing the merged changes. + The developers are affected as soon as they pull from the repository containing the merged changes. 1. **GitLab CI/CD.** We heavily lean on CI/CD for code integration and testing. CI/CD jobs do not interpret files such as `.ruby-version`. -Instead, they use the Ruby installed in the Docker container they execute in, which is defined in `.gitlab-ci.yml`. -The container images used in these jobs are maintained in the [`gitlab-build-images`](https://gitlab.com/gitlab-org/gitlab-build-images) repository. -When we merge an update to an image, CI/CD jobs are affected as soon as the [image is built](https://gitlab.com/gitlab-org/gitlab-build-images/#pushing-a-rebuild-image). + Instead, they use the Ruby installed in the Docker container they execute in, which is defined in `.gitlab-ci.yml`. + The container images used in these jobs are maintained in the [`gitlab-build-images`](https://gitlab.com/gitlab-org/gitlab-build-images) repository. + When we merge an update to an image, CI/CD jobs are affected as soon as the [image is built](https://gitlab.com/gitlab-org/gitlab-build-images/#pushing-a-rebuild-image). 1. **GitLab SaaS**. GitLab.com is deployed from customized Helm charts that use Docker images from [Cloud Native GitLab (CNG)](https://gitlab.com/gitlab-org/build/CNG). -Just like CI/CD, `.ruby-version` is meaningless in this environment. Instead, those Docker images must be patched to upgrade Ruby. -GitLab SaaS is affected with the next deployment. + Just like CI/CD, `.ruby-version` is meaningless in this environment. Instead, those Docker images must be patched to upgrade Ruby. + GitLab SaaS is affected with the next deployment. 1. **Self-managed GitLab.** Customers installing GitLab via [Omnibus](https://gitlab.com/gitlab-org/omnibus-gitlab) use none of the above. -Instead, their Ruby version is defined by the [Ruby software bundle](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/config/software/ruby.rb) in Omnibus. -Self-managed customers are affected as soon as they upgrade to the release containing this change. + Instead, their Ruby version is defined by the [Ruby software bundle](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/config/software/ruby.rb) in Omnibus. + Self-managed customers are affected as soon as they upgrade to the release containing this change. ## Ruby upgrade approach Timing all steps in a Ruby upgrade correctly is critical. As a general guideline, consider the following: - For smaller upgrades where production behavior is unlikely to change, aim to keep the version gap between -repositories and production minimal. Coordinate with stakeholders to merge all changes closely together -(within a day or two) to avoid drift. In this scenario the likely order is to upgrade developer tooling and -environments first, production second. + repositories and production minimal. Coordinate with stakeholders to merge all changes closely together + (within a day or two) to avoid drift. In this scenario the likely order is to upgrade developer tooling and + environments first, production second. - For larger changes, the risk of going to production with a new Ruby is significant. In this case, try to get into a -position where all known incompatibilities with the new Ruby version are already fixed, then work -with production engineers to deploy the new Ruby to a subset of the GitLab production fleet. In this scenario -the likely order is to update production first, developer tooling and environments second. This makes rollbacks -easier in case of critical regressions in production. + position where all known incompatibilities with the new Ruby version are already fixed, then work + with production engineers to deploy the new Ruby to a subset of the GitLab production fleet. In this scenario + the likely order is to update production first, developer tooling and environments second. This makes rollbacks + easier in case of critical regressions in production. Either way, we found that from past experience the following approach works well, with some steps likely only necessary for minor and major upgrades. Note that some of these steps can happen in parallel or may have their @@ -110,17 +110,17 @@ for a smoother transition by supporting both old and new Ruby versions for a per There are two places that require changes: 1. **[GitLab Build Images](https://gitlab.com/gitlab-org/gitlab-build-images).** These are Docker images -we use for runners and other Docker-based pre-production environments. The kind of change necessary -depends on the scope. + we use for runners and other Docker-based pre-production environments. The kind of change necessary + depends on the scope. - For [patch level updates](https://gitlab.com/gitlab-org/gitlab-build-images/-/merge_requests/418), it should suffice to increment the patch level of `RUBY_VERSION`. -All projects building against the same minor release automatically download the new patch release. + All projects building against the same minor release automatically download the new patch release. - For [major and minor updates](https://gitlab.com/gitlab-org/gitlab-build-images/-/merge_requests/320), create a new set of Docker images that can be used side-by-side with existing images during the upgrade process. **Important:** Make sure to copy over all Ruby patch files -in the `/patches` directory to a new folder matching the Ruby version you upgrade to, or they aren't applied. + in the `/patches` directory to a new folder matching the Ruby version you upgrade to, or they aren't applied. 1. **[GitLab Development Kit (GDK)](https://gitlab.com/gitlab-org/gitlab-development-kit).** -Update GDK to add the new Ruby as an additional option for -developers to choose from. This typically only requires it to be appended to `.tool-versions` so `asdf` -users will benefit from this. Other users will have to install it manually -([example](https://gitlab.com/gitlab-org/gitlab-development-kit/-/merge_requests/2136).) + Update GDK to add the new Ruby as an additional option for + developers to choose from. This typically only requires it to be appended to `.tool-versions` so `asdf` + users will benefit from this. Other users will have to install it manually + ([example](https://gitlab.com/gitlab-org/gitlab-development-kit/-/merge_requests/2136).) For larger version upgrades, consider working with [Quality Engineering](https://about.gitlab.com/handbook/engineering/quality/) to identify and set up a test plan. @@ -265,16 +265,16 @@ For GitLab SaaS, GitLab team members can inspect these log events in Kibana During the upgrade process, consider the following recommendations: - **Front-load as many changes as possible.** Especially for minor and major releases, it is likely that application -code will break or change. Any changes that are backward compatible should be merged into the main branch and -released independently ahead of the Ruby version upgrade. This ensures that we move in small increments and -get feedback from production environments early. + code will break or change. Any changes that are backward compatible should be merged into the main branch and + released independently ahead of the Ruby version upgrade. This ensures that we move in small increments and + get feedback from production environments early. - **Create an experimental branch for larger updates.** We generally try to avoid long-running topic branches, -but for purposes of feedback and experimentation, it can be useful to have such a branch to get regular -feedback from CI/CD when running a newer Ruby. This can be helpful when first assessing what problems -we might run into, as [this MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50640) demonstrates. -These experimental branches are not intended to be merged; they can be closed once all required changes have been broken out -and merged back independently. + but for purposes of feedback and experimentation, it can be useful to have such a branch to get regular + feedback from CI/CD when running a newer Ruby. This can be helpful when first assessing what problems + we might run into, as [this MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50640) demonstrates. + These experimental branches are not intended to be merged; they can be closed once all required changes have been broken out + and merged back independently. - **Give yourself enough time to fix problems ahead of a milestone release.** GitLab moves fast. -As a Ruby upgrade requires many MRs to be sent and reviewed, make sure all changes are merged at least a week -before release day. This gives us extra time to act if something breaks. If in doubt, it is better to -postpone the upgrade to the following month, as we [prioritize availability over velocity](https://about.gitlab.com/handbook/engineering/development/principles/#prioritizing-technical-decisions). + As a Ruby upgrade requires many MRs to be sent and reviewed, make sure all changes are merged at least a week + before release day. This gives us extra time to act if something breaks. If in doubt, it is better to + postpone the upgrade to the following month, as we [prioritize availability over velocity](https://about.gitlab.com/handbook/engineering/development/principles/#prioritizing-technical-decisions). diff --git a/doc/policy/maintenance.md b/doc/policy/maintenance.md index 8e4541a58a7..0790cf07ed1 100644 --- a/doc/policy/maintenance.md +++ b/doc/policy/maintenance.md @@ -72,14 +72,14 @@ GitLab. These two policies are in place because: 1. GitLab has Community and Enterprise distributions, doubling the amount of work -necessary to test/release the software. + necessary to test/release the software. 1. Backporting to more than one release creates a high development, quality assurance, -and support cost. + and support cost. 1. Supporting parallel version discourages incremental upgrades which over time accumulate in -complexity and create upgrade challenges for all users. GitLab has a dedicated team ensuring that -incremental upgrades (and installations) are as simple as possible. + complexity and create upgrade challenges for all users. GitLab has a dedicated team ensuring that + incremental upgrades (and installations) are as simple as possible. 1. The number of changes created in the GitLab application is high, which contributes to backporting complexity to older releases. In several cases, backporting has to go through the same -review process a new change goes through. + review process a new change goes through. 1. Ensuring that tests pass on the older release is a considerable challenge in some cases, and as such is very time-consuming. Including new features in a patch release is not possible as that would break [Semantic Versioning](https://semver.org/). diff --git a/lib/api/entities/merge_request_basic.rb b/lib/api/entities/merge_request_basic.rb index 56519e2bf08..39bb54bfc5a 100644 --- a/lib/api/entities/merge_request_basic.rb +++ b/lib/api/entities/merge_request_basic.rb @@ -97,6 +97,11 @@ module API expose :squash expose :squash_on_merge?, as: :squash_on_merge expose :task_completion_status + + # #cannot_be_merged? is generally indicative of conflicts, and is set via + # MergeRequests::MergeabilityCheckService. However, it can also indicate + # that either #has_no_commits? or #branch_missing? are true. + # expose :cannot_be_merged?, as: :has_conflicts expose :mergeable_discussions_state?, as: :blocking_discussions_resolved diff --git a/lib/gitlab/base_doorkeeper_controller.rb b/lib/gitlab/base_doorkeeper_controller.rb index 91994c2fa95..54cc63ab1a6 100644 --- a/lib/gitlab/base_doorkeeper_controller.rb +++ b/lib/gitlab/base_doorkeeper_controller.rb @@ -8,8 +8,6 @@ module Gitlab include EnforcesTwoFactorAuthentication include SessionsHelper - before_action :limit_session_time, if: -> { !current_user } - helper_method :can? end end diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index 0b322fd433c..d19140851f5 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -58,7 +58,8 @@ module Gitlab description: 'List of evaluable Rules to determine job inclusion.', inherit: false, metadata: { - allowed_when: %w[on_success on_failure always never manual delayed].freeze + allowed_when: %w[on_success on_failure always never manual delayed].freeze, + allowed_keys: %i[if changes exists when start_in allow_failure variables needs].freeze } entry :variables, ::Gitlab::Ci::Config::Entry::Variables, diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 1e7f6056a65..f5b5323c532 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -4,14 +4,16 @@ module Gitlab module Ci class Config module Entry + # A rule is a condition that is evaluated before a job is executed. + # Until we find a better solution in https://gitlab.com/gitlab-org/gitlab/-/issues/436473, + # these two metadata parameters need to be passed to `Entry::Rules`: + # - `allowed_when`: a list of allowed values for the `when` keyword. + # - `allowed_keys`: a list of allowed keys for each rule. class Rules::Rule < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Configurable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables needs].freeze - ALLOWED_WHEN = %w[on_success on_failure always never manual delayed].freeze - attributes :if, :exists, :when, :start_in, :allow_failure entry :changes, Entry::Rules::Rule::Changes, @@ -28,7 +30,6 @@ module Gitlab validations do validates :config, presence: true validates :config, type: { with: Hash } - validates :config, allowed_keys: ALLOWED_KEYS validates :config, disallowed_keys: %i[start_in], unless: :specifies_delay? validates :start_in, presence: true, if: :specifies_delay? validates :start_in, duration: { limit: '1 week' }, if: :specifies_delay? @@ -36,15 +37,22 @@ module Gitlab with_options allow_nil: true do validates :if, expression: true validates :exists, array_of_strings: true, length: { maximum: 50 } - validates :when, allowed_values: { in: ALLOWED_WHEN } validates :allow_failure, boolean: true end validate do + # This validation replaces the old `validates :when, allowed_values: { in: ALLOWED_WHEN }` validation. + # In https://gitlab.com/gitlab-org/gitlab/-/issues/436473, we'll remove this custom validation. validates_with Gitlab::Config::Entry::Validators::AllowedValuesValidator, attributes: %i[when], allow_nil: true, in: opt(:allowed_when) + + # This validation replaces the old `validates :config, allowed_keys: ALLOWED_KEYS` validation. + # In https://gitlab.com/gitlab-org/gitlab/-/issues/436473, we'll remove this custom validation. + validates_with Gitlab::Config::Entry::Validators::AllowedKeysValidator, + attributes: %i[config], + in: opt(:allowed_keys) end end diff --git a/lib/gitlab/ci/config/entry/workflow.rb b/lib/gitlab/ci/config/entry/workflow.rb index 5b81c74fe4d..7aafd9ba78e 100644 --- a/lib/gitlab/ci/config/entry/workflow.rb +++ b/lib/gitlab/ci/config/entry/workflow.rb @@ -19,9 +19,14 @@ module Gitlab validates :name, allow_nil: true, length: { minimum: 1, maximum: 255 } end + # `start_in`, `allow_failure`, and `needs` should not be allowed but we can't break this behavior now. + # More information: https://gitlab.com/gitlab-org/gitlab/-/issues/436473 entry :rules, Entry::Rules, description: 'List of evaluable Rules to determine Pipeline status.', - metadata: { allowed_when: %w[always never] } + metadata: { + allowed_when: %w[always never].freeze, + allowed_keys: %i[if changes exists when start_in allow_failure variables needs].freeze + } entry :auto_cancel, Entry::AutoCancel, description: 'Auto-cancel configuration for this pipeline.' diff --git a/lib/gitlab/middleware/unauthenticated_session_expiry.rb b/lib/gitlab/middleware/unauthenticated_session_expiry.rb new file mode 100644 index 00000000000..f240a6b23bd --- /dev/null +++ b/lib/gitlab/middleware/unauthenticated_session_expiry.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Gitlab + module Middleware + # By default, all sessions are given the same expiration time configured in + # the session store (e.g. 1 week). However, unauthenticated users can + # generate a lot of sessions, primarily for CSRF verification. It makes + # sense to reduce the TTL for unauthenticated to something much lower than + # the default (e.g. 2 hours) to limit Redis memory. In addition, Rails + # creates a new session after login, so the short TTL doesn't even need to + # be extended. + class UnauthenticatedSessionExpiry + def initialize(app) + @app = app + end + + def call(env) + result = @app.call(env) + + warden = env['warden'] + + unless warden && warden.user + # This works because Rack uses these options every time a request is handled, and redis-store + # uses the Rack setting first: + # 1. https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342 + # 2. https://github.com/redis-store/redis-store/blob/3acfa95f4eb6260c714fdb00a3d84be8eedc13b2/lib/redis/store/ttl.rb#L32 + env['rack.session.options'][:expire_after] = Settings.gitlab['unauthenticated_session_expire_delay'] + end + + result + end + end + end +end diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb index 21dfcd3227d..4276091251a 100644 --- a/lib/gitlab/quick_actions/merge_request_actions.rb +++ b/lib/gitlab/quick_actions/merge_request_actions.rb @@ -6,6 +6,10 @@ module Gitlab extend ActiveSupport::Concern include Gitlab::QuickActions::Dsl + REBASE_FAILURE_UNMERGEABLE = 'This merge request is currently in an unmergeable state, and cannot be rebased.' + REBASE_FAILURE_PROTECTED_BRANCH = 'This merge request branch is protected from force push.' + REBASE_FAILURE_REBASE_IN_PROGRESS = 'A rebase is already in progress.' + included do # MergeRequest only quick actions definitions desc do @@ -46,6 +50,10 @@ module Gitlab @updates[:merge] = params[:merge_request_diff_head_sha] end + ######################################################################## + # + # /rebase + # types MergeRequest desc do _('Rebase source branch') @@ -66,17 +74,17 @@ module Gitlab end command :rebase do unless quick_action_target.permits_force_push? - @execution_message[:rebase] = _('This merge request branch is protected from force push.') + @execution_message[:rebase] = _(REBASE_FAILURE_PROTECTED_BRANCH) next end if quick_action_target.cannot_be_merged? - @execution_message[:rebase] = _('This merge request cannot be rebased while there are conflicts.') + @execution_message[:rebase] = _(REBASE_FAILURE_UNMERGEABLE) next end if quick_action_target.rebase_in_progress? - @execution_message[:rebase] = _('A rebase is already in progress.') + @execution_message[:rebase] = _(REBASE_FAILURE_REBASE_IN_PROGRESS) next end diff --git a/lib/gitlab/ssh/commit.rb b/lib/gitlab/ssh/commit.rb index 7d7cc529b1a..4ea64e117ac 100644 --- a/lib/gitlab/ssh/commit.rb +++ b/lib/gitlab/ssh/commit.rb @@ -10,7 +10,7 @@ module Gitlab end def attributes - signature = ::Gitlab::Ssh::Signature.new(signature_text, signed_text, signer, @commit.committer_email) + signature = ::Gitlab::Ssh::Signature.new(signature_text, signed_text, signer, @commit) { commit_sha: @commit.sha, diff --git a/lib/gitlab/ssh/signature.rb b/lib/gitlab/ssh/signature.rb index 53e11c98b1c..5f4b6cdcb8d 100644 --- a/lib/gitlab/ssh/signature.rb +++ b/lib/gitlab/ssh/signature.rb @@ -11,11 +11,12 @@ module Gitlab GIT_NAMESPACE = 'git' - def initialize(signature_text, signed_text, signer, committer_email) + def initialize(signature_text, signed_text, signer, commit) @signature_text = signature_text @signed_text = signed_text @signer = signer - @committer_email = committer_email + @commit = commit + @committer_email = commit.committer_email end def verification_status @@ -23,15 +24,8 @@ module Gitlab next :unverified unless all_attributes_present? next :verified_system if verified_by_gitlab? next :unverified unless valid_signature_blob? - next :unknown_key unless signed_by_key - next :other_user unless committer - next :other_user unless signed_by_key.user == committer - - if signed_by_user_email_verified? - :verified - else - :unverified - end + + calculate_verification_status end end @@ -47,7 +41,7 @@ module Gitlab strong_memoize(:key_fingerprint) do public_key = signature&.public_key - next if public_key.is_a?(SSHData::Certificate) + next public_key.public_key.fingerprint if public_key.is_a?(SSHData::Certificate) public_key.fingerprint end @@ -55,10 +49,12 @@ module Gitlab private + attr_reader :commit, :committer_email + def all_attributes_present? # Signing an empty string is valid, but signature_text and committer_email # must be non-empty. - @signed_text && @signature_text.present? && @committer_email.present? + @signed_text && @signature_text.present? && committer_email.present? end # Verifies the signature using the public key embedded in the blob. @@ -72,14 +68,23 @@ module Gitlab signature.verify(@signed_text) end - def committer + def calculate_verification_status + return :unknown_key unless signed_by_key + return :other_user unless committer? + return :unverified unless signed_by_user_email_verified? + + :verified + end + + def committer? # Lookup by email because users can push verified commits that were made # by someone else. For example: Doing a rebase. - strong_memoize(:committer) { User.find_by_any_email(@committer_email) } + committer = User.find_by_any_email(committer_email) + committer && signed_by_key.user == committer end def signed_by_user_email_verified? - signed_by_key.user.verified_emails.include?(@committer_email) + signed_by_key.user.verified_emails.include?(committer_email) end def signature @@ -101,3 +106,5 @@ module Gitlab end end end + +Gitlab::Ssh::Signature.prepend_mod diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ec0c1400d85..7f269728ced 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1900,9 +1900,6 @@ msgstr "" msgid "A ready-to-go template for use with iOS Swift apps" msgstr "" -msgid "A rebase is already in progress." -msgstr "" - msgid "A release with a date in the future is labeled as an %{linkStart}Upcoming Release%{linkEnd}." msgstr "" @@ -50184,12 +50181,6 @@ msgstr "" msgid "This means you can not push code until you create an empty repository or import existing one." msgstr "" -msgid "This merge request branch is protected from force push." -msgstr "" - -msgid "This merge request cannot be rebased while there are conflicts." -msgstr "" - msgid "This merge request does not have accessibility reports" msgstr "" diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index d920fbc3c32..715ded875fe 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -103,32 +103,6 @@ RSpec.describe ApplicationController, feature_category: :shared do end end - describe 'session expiration' do - controller(described_class) do - def index - render html: 'authenticated' - end - end - - context 'authenticated user' do - it 'does not set the expire_after option' do - sign_in(create(:user)) - - get :index - - expect(request.env['rack.session.options'][:expire_after]).to be_nil - end - end - - context 'unauthenticated user' do - it 'sets the expire_after option' do - get :index - - expect(request.env['rack.session.options'][:expire_after]).to eq(Settings.gitlab['unauthenticated_session_expire_delay']) - end - end - end - describe 'response format' do controller(described_class) do def index diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index cfb512afc91..ce21c278a53 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -216,24 +216,6 @@ RSpec.describe Oauth::AuthorizationsController do end end - context 'when the user is not signed in' do - before do - sign_out(user) - end - - it 'sets a lower session expiry and redirects to the sign in page' do - subject - - expect(request.env['rack.session.options'][:expire_after]).to eq( - Settings.gitlab['unauthenticated_session_expire_delay'] - ) - - expect(request.session['user_return_to']).to eq("/oauth/authorize?#{params.to_query}") - expect(response).to have_gitlab_http_status(:found) - expect(response).to redirect_to(new_user_session_path) - end - end - context 'when the user is admin' do context 'when disable_admin_oauth_scopes is set' do before do diff --git a/spec/initializers/session_store_spec.rb b/spec/initializers/session_store_spec.rb index a94ce327a92..c9333d022dd 100644 --- a/spec/initializers/session_store_spec.rb +++ b/spec/initializers/session_store_spec.rb @@ -5,6 +5,10 @@ require 'spec_helper' RSpec.describe 'Session initializer for GitLab' do subject { Gitlab::Application.config } + before do + allow(subject).to receive_message_chain(:middleware, :insert_after) + end + let(:load_session_store) do load Rails.root.join('config/initializers/session_store.rb') end diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb index 3531d6e9f1a..23b1046deb7 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' require 'gitlab_chronic_duration' -RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do +RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule, feature_category: :pipeline_composition do let(:factory) do Gitlab::Config::Entry::Factory.new(described_class) .metadata(metadata) @@ -11,7 +11,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do end let(:metadata) do - { allowed_when: %w[on_success on_failure always never manual delayed] } + { + allowed_when: %w[on_success on_failure always never manual delayed], + allowed_keys: %i[if changes exists when start_in allow_failure variables needs] + } end let(:entry) { factory.create! } @@ -296,35 +299,8 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do end end - context 'with a string passed in metadata but not allowed in the class' do - let(:metadata) { { allowed_when: %w[explode] } } - - let(:config) do - { if: '$THIS == "that"', when: 'explode' } - end - - it { is_expected.to be_a(described_class) } - it { is_expected.not_to be_valid } - - it 'returns an error about invalid when:' do - expect(subject.errors).to include(/when unknown value: explode/) - end - - context 'when composed' do - before do - subject.compose! - end - - it { is_expected.not_to be_valid } - - it 'returns an error about invalid when:' do - expect(subject.errors).to include(/when unknown value: explode/) - end - end - end - - context 'with a string allowed in the class but not passed in metadata' do - let(:metadata) { { allowed_when: %w[always never] } } + context 'with an invalid when' do + let(:metadata) { { allowed_when: %w[always never], allowed_keys: %i[if when] } } let(:config) do { if: '$THIS == "that"', when: 'on_success' } diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index b0871f2345e..37219c743cc 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -3,14 +3,16 @@ require 'fast_spec_helper' require_dependency 'active_model' -RSpec.describe Gitlab::Ci::Config::Entry::Rules do +RSpec.describe Gitlab::Ci::Config::Entry::Rules, feature_category: :pipeline_composition do let(:factory) do Gitlab::Config::Entry::Factory.new(described_class) .metadata(metadata) .value(config) end - let(:metadata) { { allowed_when: %w[always never] } } + let(:metadata) do + { allowed_when: %w[always never], allowed_keys: %i[if when] } + end subject(:entry) { factory.create! } diff --git a/spec/lib/gitlab/ci/config/entry/workflow_spec.rb b/spec/lib/gitlab/ci/config/entry/workflow_spec.rb index d3ce3ffe641..01ba72ff042 100644 --- a/spec/lib/gitlab/ci/config/entry/workflow_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/workflow_spec.rb @@ -6,6 +6,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Workflow, feature_category: :pipeline_ subject(:config) { described_class.new(workflow_hash) } describe 'validations' do + before do + config.compose! + end + context 'when work config value is a string' do let(:workflow_hash) { 'build' } @@ -27,6 +31,28 @@ RSpec.describe Gitlab::Ci::Config::Entry::Workflow, feature_category: :pipeline_ end context 'when work config value is a hash' do + context 'with an invalid key' do + let(:workflow_hash) { { trash: [{ if: '$VAR' }] } } + + describe '#valid?' do + it 'is invalid' do + expect(config).not_to be_valid + end + + it 'attaches an error specifying the unknown key' do + expect(config.errors).to include('workflow config contains unknown keys: trash') + end + end + + describe '#value' do + it 'returns the invalid configuration' do + expect(config.value).to eq(workflow_hash) + end + end + end + end + + context 'when config has rules' do let(:workflow_hash) { { rules: [{ if: '$VAR' }] } } describe '#valid?' do @@ -45,8 +71,8 @@ RSpec.describe Gitlab::Ci::Config::Entry::Workflow, feature_category: :pipeline_ end end - context 'with an invalid key' do - let(:workflow_hash) { { trash: [{ if: '$VAR' }] } } + context 'when rules has an invalid key' do + let(:workflow_hash) { { rules: [{ if: '$VAR', trash: 'something' }] } } describe '#valid?' do it 'is invalid' do @@ -54,7 +80,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Workflow, feature_category: :pipeline_ end it 'attaches an error specifying the unknown key' do - expect(config.errors).to include('workflow config contains unknown keys: trash') + expect(config.errors).to include('rules:rule config contains unknown keys: trash') end end diff --git a/spec/lib/gitlab/ssh/commit_spec.rb b/spec/lib/gitlab/ssh/commit_spec.rb index 3b53ed9d1db..0d2621a16c0 100644 --- a/spec/lib/gitlab/ssh/commit_spec.rb +++ b/spec/lib/gitlab/ssh/commit_spec.rb @@ -28,7 +28,7 @@ RSpec.describe Gitlab::Ssh::Commit, feature_category: :source_code_management do }) allow(Gitlab::Ssh::Signature).to receive(:new) - .with(signature_text, signed_text, signer, commit.committer_email) + .with(signature_text, signed_text, signer, commit) .and_return(verifier) end diff --git a/spec/lib/gitlab/ssh/signature_spec.rb b/spec/lib/gitlab/ssh/signature_spec.rb index 6a4b72c2cb5..608fa9d71d0 100644 --- a/spec/lib/gitlab/ssh/signature_spec.rb +++ b/spec/lib/gitlab/ssh/signature_spec.rb @@ -8,7 +8,9 @@ RSpec.describe Gitlab::Ssh::Signature, feature_category: :source_code_management let_it_be(:public_key_text) { 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHZ8NHEnCIpC4mnot+BRxv6L+fq+TnN1CgsRrHWLmfwb' } let_it_be_with_reload(:user) { create(:user, email: committer_email) } let_it_be_with_reload(:key) { create(:key, usage_type: :signing, key: public_key_text, user: user) } + let_it_be_with_reload(:project) { create(:project, :repository, :in_group) } + let(:commit) { project.commit } let(:signed_text) { 'This message was signed by an ssh key' } let(:signer) { :SIGNER_USER } @@ -24,12 +26,16 @@ RSpec.describe Gitlab::Ssh::Signature, feature_category: :source_code_management SIG end + before do + allow(commit).to receive(:committer_email).and_return(committer_email) + end + subject(:signature) do described_class.new( signature_text, signed_text, signer, - committer_email + commit ) end @@ -289,36 +295,23 @@ RSpec.describe Gitlab::Ssh::Signature, feature_category: :source_code_management # ssh-keygen -Y sign -n git -f id_test-cert.pub message.txt <<~SIG -----BEGIN SSH SIGNATURE----- - U1NIU0lHAAAAAQAABGIAAAAgc3NoLWVkMjU1MTktY2VydC12MDFAb3BlbnNzaC5jb20AAA - Aga68FsjVAge+7I5h/qC8luu7iK+5QfrVlDnKRVy1d7zUAAAAgYAsBVqgfGrvGdSPjqY0H + U1NIU0lHAAAAAQAAAb0AAAAgc3NoLWVkMjU1MTktY2VydC12MDFAb3BlbnNzaC5jb20AAA + AgWbXlnjWbxTzOlRPcnSMlQQnnJTCsEv2y2ij5o7yVbcUAAAAgYAsBVqgfGrvGdSPjqY0H t8yljpOS4VumZHnAh+wCvdEAAAAAAAAAAAAAAAEAAAARYWRtaW5AZXhhbXBsZS5jb20AAA - AAAAAAAGV8UoAAAAAAZX2kUQAAAAAAAACCAAAAFXBlcm1pdC1YMTEtZm9yd2FyZGluZwAA + AAAAAAAGV9kqgAAAAAZX7kiwAAAAAAAACCAAAAFXBlcm1pdC1YMTEtZm9yd2FyZGluZwAA AAAAAAAXcGVybWl0LWFnZW50LWZvcndhcmRpbmcAAAAAAAAAFnBlcm1pdC1wb3J0LWZvcn dhcmRpbmcAAAAAAAAACnBlcm1pdC1wdHkAAAAAAAAADnBlcm1pdC11c2VyLXJjAAAAAAAA - AAAAAAGXAAAAB3NzaC1yc2EAAAADAQABAAABgQDWpZvEFL60+ijqjg/UGEWnjnHsxzEDZe - L00prZ7XdZE9yQXb2eI5TmPP/NRXHL4gRuaVBvwllOEeZRI7TJtMCVGQhw8ORVc7sYb6Pp - Y4j1AI35SsdM9SrNLucPtR8k46ab4vT0cuT/jx8fEppF7bjJ86NOfVUxGv5mhYv21iIX6L - XRUpBlLOGlxtU83PgP8Z5f4T6WUkcLM+Uh22msk15EAnWPF3FQbTH1VA88dHJG76gnpaD2 - 0FJffhaLYl9UJAzDBZqjYeqjNuVN3+BOzi3zAZNhaPXOznw2QXHMXWmgOao4hLWgk5wgUa - vXkQehqtB0kQhn8xXir8RbEzPDBToxVWnMJKVO27MtovjWUorNKRFlySwafKKuem/lBulU - 2VcUvSbB4mRSdZZimqmTieSO8s0onq3TASSAX7nmtkhx1x9cFY4l49TBcZNAiNogBeR40N - 6ByawjbptznmjkADf9qdI8rLmeuTnr1GgjJsQBsS8qdomG4ITkEG0WJHIbOr0AAAGUAAAA - DHJzYS1zaGEyLTUxMgAAAYCqcUD/BkdPxEc6RgaatVRyPllNTXA/7Oz7xK1ZGbhkvweO22 - itXpb/E1Cy/ir8pppAh2Trec0MPI3vgs6TLHcsOGQbtUMVckfY9SxErlq8H0dEjouzhvnt - rvkNnRojfdeAUR+6UGhlZpiF8kPbNMrxkqw2Ir5uLkReZphXlw/Qg3DY/eQ2Hlns8Up9ql - 4OD+pfoOQtegy5FICjec7fXqlb5KgpazwNoENxh/QoomndHWntnNW7B/fiJL++vGoc6OX4 - KFxRPFKQoyemQXVa3K3Unli8hYI8o4YzcLiYJ6Uk6MR+RmYvyz7ZF469d1SoO1enDQFOzJ - LSq0zaeFX9HmJ8/2rzKs7bnQKWUjm3C2KmyO/NtnZr/qB45tDlWNvuP7+phMfiJRvpvIyc - JZdyc9T1CWrzlQoIUaH08gizmyP37NAgqzUTb3NAnhGb1dp70mTfORRnRoKQtzX/DiSfVZ - EOk+qnuoJ7Gs4erWNRW2363SgbKKOhPCxeMfUY2zm628sAAAAEZmlsZQAAAAAAAAAGc2hh - NTEyAAAAUwAAAAtzc2gtZWQyNTUxOQAAAED54YE97IiluG2xM7OFysEUQfhKEztaT+vvD3 - kxDq9mHz0Gr6uiKd/gKL+/yGHPAoif74khm/gUe/A9AI7+JvcH + AAAAAAAzAAAAC3NzaC1lZDI1NTE5AAAAIINudhvW7P4c36bBwlWTaxnCCOaSfMrUbXHcP7 + 7zH6LyAAAAUwAAAAtzc2gtZWQyNTUxOQAAAEBp9J9YQhaz+tNIKtNpZe5sAxcqvMgcYlB+ + fVaDsYNOj445Bz7TBoFqjrs95yaF6pwARK11IEQTcwtrihLGzGkNAAAAA2dpdAAAAAAAAA + AGc2hhNTEyAAAAUwAAAAtzc2gtZWQyNTUxOQAAAECfVh7AzwqRBMbnHBApCnMpu9Y1qpGM + sOSL1EeV3SIOlrThNTCerUpcaizcSY9L8WwP2TXlqw2Sq1BGM+PPSN0C -----END SSH SIGNATURE----- SIG end - it 'returns nil' do - expect(signature.key_fingerprint).to be_nil + it 'returns public key fingerprint' do + expect(signature.key_fingerprint).to eq('3dNIFKfIAXZb/JL30KKv95cps+mZwVAuAYQhIWxAb+8') end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 67388f657f4..06ef68f190f 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -2457,6 +2457,8 @@ RSpec.describe API::Commits, feature_category: :source_code_management do end context 'with ssh signed commit' do + let_it_be(:project) { create(:project, :repository, :public, :in_group) } + let(:commit_id) { '7b5160f9bb23a3d58a0accdbe89da13b96b1ece9' } let!(:commit) { project.commit(commit_id) } diff --git a/spec/requests/api/graphql/project/tree/tree_spec.rb b/spec/requests/api/graphql/project/tree/tree_spec.rb index 77b72bf39a1..d71908d6458 100644 --- a/spec/requests/api/graphql/project/tree/tree_spec.rb +++ b/spec/requests/api/graphql/project/tree/tree_spec.rb @@ -167,6 +167,8 @@ RSpec.describe 'getting a tree in a project', feature_category: :source_code_man end context 'when the ref points to a SSH-signed commit' do + let_it_be(:project) { create(:project, :repository, :in_group) } + let_it_be(:ref) { 'ssh-signed-commit' } let_it_be(:commit) { project.commit(ref) } let_it_be(:current_user) { create(:user, email: commit.committer_email).tap { |user| project.add_owner(user) } } diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 52fdf6bc69e..969fdbe9838 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -5,11 +5,33 @@ require 'spec_helper' RSpec.describe ApplicationController, type: :request, feature_category: :shared do let_it_be(:user) { create(:user) } - before do - sign_in(user) - end - it_behaves_like 'Base action controller' do + before do + sign_in(user) + end + subject(:request) { get root_path } end + + describe 'session expiration' do + context 'when user is authenticated' do + it 'does not set the expire_after option' do + sign_in(user) + + get root_path + + expect(request.env['rack.session.options'][:expire_after]).to be_nil + end + end + + context 'when user is unauthenticated' do + it 'sets the expire_after option' do + get root_path + + expect(request.env['rack.session.options'][:expire_after]).to eq( + Settings.gitlab['unauthenticated_session_expire_delay'] + ) + end + end + end end diff --git a/spec/requests/oauth/authorizations_controller_spec.rb b/spec/requests/oauth/authorizations_controller_spec.rb index 7887bf52542..6ef8970a142 100644 --- a/spec/requests/oauth/authorizations_controller_spec.rb +++ b/spec/requests/oauth/authorizations_controller_spec.rb @@ -5,16 +5,19 @@ require 'spec_helper' RSpec.describe Oauth::AuthorizationsController, feature_category: :system_access do let_it_be(:user) { create(:user) } let_it_be(:application) { create(:oauth_application, redirect_uri: 'custom://test') } - let_it_be(:oauth_authorization_path) do - Gitlab::Routing.url_helpers.oauth_authorization_url( + + let(:params) do + { client_id: application.uid, response_type: 'code', scope: application.scopes, redirect_uri: application.redirect_uri, state: SecureRandom.hex - ) + } end + let(:oauth_authorization_path) { Gitlab::Routing.url_helpers.oauth_authorization_url(params) } + before do sign_in(user) end @@ -76,5 +79,23 @@ RSpec.describe Oauth::AuthorizationsController, feature_category: :system_access end end end + + context 'when the user is not signed in' do + before do + sign_out(user) + end + + it 'sets a lower session expiry and redirects to the sign in page' do + get oauth_authorization_path + + expect(request.env['rack.session.options'][:expire_after]).to eq( + Settings.gitlab['unauthenticated_session_expire_delay'] + ) + + expect(request.session['user_return_to']).to eq("/oauth/authorize?#{params.to_query}") + expect(response).to have_gitlab_http_status(:found) + expect(response).to redirect_to(new_user_session_path) + end + end end end diff --git a/spec/support/shared_examples/quick_actions/merge_request/rebase_quick_action_shared_examples.rb b/spec/support/shared_examples/quick_actions/merge_request/rebase_quick_action_shared_examples.rb index 92705fc1b4d..2c38c716736 100644 --- a/spec/support/shared_examples/quick_actions/merge_request/rebase_quick_action_shared_examples.rb +++ b/spec/support/shared_examples/quick_actions/merge_request/rebase_quick_action_shared_examples.rb @@ -59,7 +59,7 @@ RSpec.shared_examples 'rebase quick action' do it 'tells the user a rebase is in progress' do add_note('/rebase') - expect(page).to have_content 'A rebase is already in progress.' + expect(page).to have_content Gitlab::QuickActions::MergeRequestActions::REBASE_FAILURE_REBASE_IN_PROGRESS expect(page).not_to have_content 'Scheduled a rebase' end end @@ -70,7 +70,7 @@ RSpec.shared_examples 'rebase quick action' do it 'does not rebase the MR' do add_note("/rebase") - expect(page).to have_content 'This merge request cannot be rebased while there are conflicts.' + expect(page).to have_content Gitlab::QuickActions::MergeRequestActions::REBASE_FAILURE_UNMERGEABLE end end @@ -89,7 +89,7 @@ RSpec.shared_examples 'rebase quick action' do it 'does not rebase the MR' do add_note("/rebase") - expect(page).to have_content 'This merge request branch is protected from force push.' + expect(page).to have_content Gitlab::QuickActions::MergeRequestActions::REBASE_FAILURE_PROTECTED_BRANCH end end end diff --git a/spec/views/projects/commit/show.html.haml_spec.rb b/spec/views/projects/commit/show.html.haml_spec.rb index 4cfff00d390..7f3cecae8cb 100644 --- a/spec/views/projects/commit/show.html.haml_spec.rb +++ b/spec/views/projects/commit/show.html.haml_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe 'projects/commit/show.html.haml', feature_category: :source_code_management do - let(:project) { create(:project, :repository) } + let_it_be_with_refind(:project) { create(:project, :repository, :in_group) } + let(:commit) { project.commit } before do |