Silent Mode: Block many outbound HTTP requests
What does this MR do and why?
Silent Mode: Block many outbound HTTP requests.
Resolves #410048 (closed)
Related to #409662 (closed), #393639 (closed)
Screenshots or screen recordings
Gitlab::HTTP
behaves as usual:
Silent Mode is disabled by default. ➜ gitlab git:(mk/silent-mode-block-outbound-gitlab-http) bin/rails c
--------------------------------------------------------------------------------
Ruby: ruby 3.0.5p211 (2022-11-24 revision ba5cf0f7c5) [arm64-darwin21]
GitLab: 16.0.0-pre (89f7dec48d7) EE
GitLab Shell: 14.19.0
PostgreSQL: 12.13
Geo enabled: yes
Geo server: primary
--------------------------------------------------------------------------------
Loading development environment (Rails 6.1.7.2)
[2] pry(main)> Gitlab::HTTP.post('https://httpstat.us/200', body: 'foo')
=> "200 OK"
[3] pry(main)> Gitlab::HTTP.post('https://httpstat.us/404', body: 'foo')
=> "404 Not Found"
[4] pry(main)> Gitlab::CurrentSettings.silent_mode_enabled?
=> false
Enable Silent Mode
[5] pry(main)> Gitlab::CurrentSettings.update!(silent_mode_enabled: true)
=> true
[6] pry(main)> Gitlab::CurrentSettings.silent_mode_enabled?
=> true
Gitlab::HTTP
POST requests are blocked with an error that looks similar to an HTTParty error.
[7] pry(main)> Gitlab::HTTP.post('https://httpstat.us/200', body: 'foo')
Gitlab::HTTP::SilentModeBlockedError: only get, head, options, and trace methods are allowed in silent mode
from /Users/mkozonogitlab/Developer/gdk/gitlab/lib/gitlab/http.rb:99:in `raise_if_blocked_by_silent_mode'
[8] pry(main)> Gitlab::HTTP.post('https://httpstat.us/404', body: 'foo')
Gitlab::HTTP::SilentModeBlockedError: only get, head, options, and trace methods are allowed in silent mode
from /Users/mkozonogitlab/Developer/gdk/gitlab/lib/gitlab/http.rb:99:in `raise_if_blocked_by_silent_mode'
Logs:
{"severity":"INFO","time":"2023-05-11T02:18:11.928Z","correlation_id":null,"message":"Outbound HTTP request blocked","http_method":"Net::HTTP::Post","silent_mode_enabled":true}
{"severity":"INFO","time":"2023-05-11T02:18:15.683Z","correlation_id":null,"message":"Outbound HTTP request blocked","http_method":"Net::HTTP::Post","silent_mode_enabled":true}
Disable Silent Mode
[9] pry(main)> Gitlab::CurrentSettings.update!(silent_mode_enabled: false)
=> true
[10] pry(main)> Gitlab::CurrentSettings.silent_mode_enabled?
=> false
Gitlab::HTTP
POST requests work again:
[11] pry(main)> Gitlab::HTTP.post('https://httpstat.us/200', body: 'foo')
=> "200 OK"
[12] pry(main)> Gitlab::HTTP.post('https://httpstat.us/404', body: 'foo')
=> "404 Not Found"
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
Project webhooks will perform outbound POST requests using Gitlab::HTTP
. So Silent Mode should block those requests.
How to set up a Project webhook
- Create a project if you have none
- In the Project main page, visit Settings > Webhooks
- Fill in URL with
https://httpstat.us/200
. This is just a URL which will always return 200 OK. - Check the checkbox for Comments. Or whatever triggers you want to test.
- Click Add webhook
Confirm that the webhook succeeds when Silent Mode is disabled
- In the Project Hooks box, click the Test dropdown.
- Choose Comments to fire that trigger.
- Notice that with Silent Mode disabled, you will see a blue flash message
Hook executed successfully: HTTP 200
.
Enable Silent Mode
Confirm that the webhook is blocked when Silent Mode is enabled
- In the Project Hooks box which was setup previously, click the Test dropdown.
- Choose Comments to fire that trigger.
- Notice the 500 error page indicating an error was raised prior to sending the HTTP request. (Non-development environments would respond with a less verbose 500.)
The GitLab Rails app should raise an error any time a POST, PUT, PATCH, or DELETE request is made from Gitlab::HTTP
in the GitLab Rails app.
Webhooks are normally fired asynchronously. You can also observe a more comprehensive test:
Confirm that the webhook is blocked in Sidekiq jobs
-
Create an issue in the project with the new webhook.
-
gdk tail rails-background-jobs
-
Add a comment to the issue.
-
Notice a failed
WebHookWorker
job when Silent Mode is enabled:2023-05-11_03:03:18.38612 rails-background-jobs : {"severity":"WARN","time":"2023-05-11T03:03:18.385Z","retry":4,"queue":"default","backtrace":true,"version":0,"dead":false,"args":["1","[FILTERED]","note_hooks","{}"],"class":"WebHookWorker","jid":"ea437a5f0908b6b12d944ca4","created_at":"2023-05-11T03:03:18.143Z","correlation_id":"01H04C6KWM0R2PQB3MJDGCFBCB","meta.caller_id":"NewNoteWorker","meta.remote_ip":"172.16.123.1","meta.feature_category":"integrations","meta.user":"root","meta.user_id":1,"meta.project":"flightjs/Flight","meta.root_namespace":"flightjs","meta.client_id":"user/1","meta.root_caller_id":"Projects::NotesController#create","meta.related_class":"ProjectHook","meta.subscription_plan":"default","worker_data_consistency":"delayed","wal_locations":{},"wal_location_source":"primary","idempotency_key":"resque:gitlab:duplicate:default:07f3d1c6a9468f469900f9624467da6d2cf64448d61777a20e1fbe8e6fb8d289","size_limiter":"validated","enqueued_at":"2023-05-11T03:03:18.147Z","job_size_bytes":2410,"pid":57960,"message":"WebHookWorker JID-ea437a5f0908b6b12d944ca4: fail: 0.030162 sec","job_status":"fail","scheduling_latency_s":0.001032,"redis_calls":5,"redis_duration_s":0.000696,"redis_read_bytes":17,"redis_write_bytes":275,"redis_queues_calls":1,"redis_queues_duration_s":0.000202,"redis_queues_read_bytes":1,"redis_queues_write_bytes":123,"redis_shared_state_calls":4,"redis_shared_state_duration_s":0.000494,"redis_shared_state_read_bytes":16,"redis_shared_state_write_bytes":152,"db_count":1,"db_write_count":0,"db_cached_count":0,"db_replica_count":0,"db_primary_count":1,"db_main_count":1,"db_ci_count":0,"db_geo_count":0,"db_main_replica_count":0,"db_ci_replica_count":0,"db_replica_cached_count":0,"db_primary_cached_count":0,"db_main_cached_count":0,"db_ci_cached_count":0,"db_geo_cached_count":0,"db_main_replica_cached_count":0,"db_ci_replica_cached_count":0,"db_replica_wal_count":0,"db_primary_wal_count":0,"db_main_wal_count":0,"db_ci_wal_count":0,"db_geo_wal_count":0,"db_main_replica_wal_count":0,"db_ci_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_wal_cached_count":0,"db_main_wal_cached_count":0,"db_ci_wal_cached_count":0,"db_geo_wal_cached_count":0,"db_main_replica_wal_cached_count":0,"db_ci_replica_wal_cached_count":0,"db_replica_duration_s":0.0,"db_primary_duration_s":0.002,"db_main_duration_s":0.002,"db_ci_duration_s":0.0,"db_geo_duration_s":0.0,"db_main_replica_duration_s":0.0,"db_ci_replica_duration_s":0.0,"cpu_s":0.004533,"worker_id":"sidekiq_0","rate_limiting_gates":[],"duration_s":0.030162,"completed_at":"2023-05-11T03:03:18.178Z","load_balancing_strategy":"primary_no_wal","exception.class":"Gitlab::HTTP::SilentModeBlockedError","exception.message":"only get, head, options, and trace methods are allowed in silent mode","exception.backtrace":["lib/gitlab/http.rb:99:in `raise_if_blocked_by_silent_mode'","lib/gitlab/http.rb:48:in `perform_request'","app/services/web_hook_service.rb:123:in `make_request'","app/services/web_hook_service.rb:72:in `execute'","app/workers/web_hook_worker.rb:27:in `perform'","lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing.rb:16:in `perform'","lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb:44:in `perform'","lib/gitlab/sidekiq_middleware/duplicate_jobs/server.rb:8:in `call'","lib/gitlab/sidekiq_middleware/worker_context.rb:9:in `wrap_in_optional_context'","lib/gitlab/sidekiq_middleware/worker_context/server.rb:19:in `block in call'","lib/gitlab/application_context.rb:118:in `block in use'","lib/gitlab/application_context.rb:118:in `use'","lib/gitlab/application_context.rb:57:in `with_context'","lib/gitlab/sidekiq_middleware/worker_context/server.rb:17:in `call'","lib/gitlab/sidekiq_status/server_middleware.rb:7:in `call'","lib/gitlab/sidekiq_versioning/middleware.rb:9:in `call'","lib/gitlab/sidekiq_middleware/query_analyzer.rb:7:in `block in call'","lib/gitlab/database/query_analyzer.rb:37:in `within'","lib/gitlab/sidekiq_middleware/query_analyzer.rb:7:in `call'","lib/gitlab/sidekiq_middleware/admin_mode/server.rb:14:in `call'","lib/gitlab/sidekiq_middleware/instrumentation_logger.rb:9:in `call'","lib/gitlab/sidekiq_middleware/batch_loader.rb:7:in `call'","lib/gitlab/sidekiq_middleware/extra_done_log_metadata.rb:7:in `call'","lib/gitlab/sidekiq_middleware/request_store_middleware.rb:10:in `block in call'","lib/gitlab/with_request_store.rb:17:in `enabling_request_store'","lib/gitlab/with_request_store.rb:10:in `with_request_store'","lib/gitlab/sidekiq_middleware/request_store_middleware.rb:9:in `call'","lib/gitlab/sidekiq_middleware/server_metrics.rb:83:in `block in call'","lib/gitlab/sidekiq_middleware/server_metrics.rb:110:in `block in instrument'","lib/gitlab/metrics/background_transaction.rb:33:in `run'","lib/gitlab/sidekiq_middleware/server_metrics.rb:110:in `instrument'","lib/gitlab/sidekiq_middleware/server_metrics.rb:82:in `call'","lib/gitlab/sidekiq_middleware/monitor.rb:10:in `block in call'","lib/gitlab/sidekiq_daemon/monitor.rb:46:in `within_job'","lib/gitlab/sidekiq_middleware/monitor.rb:9:in `call'","lib/gitlab/sidekiq_middleware/size_limiter/server.rb:13:in `call'","lib/gitlab/sidekiq_logging/structured_logger.rb:21:in `call'"],"db_duration_s":0.000364}
Why are these errors ok?
This isn't ideal UX, but this MR is intended as a starting point for iteration. The Silent Mode feature is currently an "Experiment". The UX impact and potential mitigation of an error at each call site must be evaluated on a case-by-case basis. Importantly, end users won't interact with GitLab in Silent Mode. Only sysadmins testing non-production instances will perform tests with Silent Mode enabled.
There is no feature flag, since the Application Setting silent_mode_enabled
(with no UI control) acts as one. It would be redundant to cover this toggle with another.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %16.1
added featureaddition groupgeo typefeature labels
assigned to @mkozono
added devopssystems sectioncore platform labels
- A deleted user
added backend label
2 Warnings 9d9cda91: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 202b1831: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
-
doc/administration/silent_mode/index.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Hitesh Raghuvanshi (
@hraghuvanshi
) (UTC+5.5, 15.5 hours ahead of@mkozono
)Max Woolf (
@mwoolf
) (UTC+1, 11 hours ahead of@mkozono
)Application Security Reviewer review is optional for Application Security Rohit Shambhuni (
@rshambhuni
) (UTC+5.5, 15.5 hours ahead of@mkozono
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger-
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
mentioned in merge request !96415 (closed)
added 2 commits
- Resolved by Michael Kozono
@pedropombeiro Would you please review for backend? By the way, I can split the 2nd commit into a follow up MR if preferred.
@truegreg Would you please review for AppSec?
requested review from @pedropombeiro and @truegreg
mentioned in issue #409662 (closed)
mentioned in issue #393639 (closed)
added 2 commits
- A deleted user
added documentation label
removed review request for @truegreg
requested review from @ameyadarshan
@ameyadarshan
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
- Resolved by Michael Kozono
- Resolved by Pedro Pombeiro
- Resolved by Michael Kozono
@mkozono The manual testing looks fine, all steps went according to the expectations in the MR description. I've added a few comments (most non-blocking), please let me know what you think.
- Resolved by Michael Kozono
mentioned in merge request gitlab-com/www-gitlab-com!113988 (merged)
@ameyadarshan Sorry, I force pushed over one of my new commits in which I locally applied some suggestions, and it removed your required approval.
- Resolved by Michael Kozono
requested review from @alipniagov
requested review from @mkaeppler and removed review request for @alipniagov
requested review from @stanhu and removed review request for @pedropombeiro, @mkaeppler, and @ameyadarshan
requested review from @mkaeppler and @ameyadarshan and removed review request for @stanhu
- Resolved by Matthias Käppler
- Resolved by Michael Kozono
- Resolved by Matthias Käppler
- Resolved by Matthias Käppler
added 1207 commits
Toggle commit list- Resolved by Matthias Käppler
Thanks for addressing my feedback @mkozono, this LGTM!
One last question before I merge: since this contains docs changes, did you want to have a tech writer have a look, or should we go ahead? I believe it is optional.
enabled an automatic merge when the pipeline for 636fa3b2 succeeds
mentioned in commit 4e94a044
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
mentioned in merge request !122139 (merged)
mentioned in issue #415227 (closed)
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!2224 (merged)
mentioned in merge request !125226 (merged)
mentioned in issue #419052 (closed)
mentioned in merge request !125024 (merged)
mentioned in merge request omnibus-gitlab!7102 (merged)