Skip to content
Snippets Groups Projects

Add SMTP timeout configuration options

Merged Stan Hu requested to merge sh-add-smtp-timeouts into master
All threads resolved!

What does this MR do?

In https://github.com/mikel/mail/pull/1427/files, mail v2.8.1 gem modified the default open and read timeout values to 5 seconds instead of the default Net::SMTP 30 and 60 seconds, respectively.

Since upgrading to GitLab 15.10.3, some users have observed receiving Net::ReadTimeout errors. This commit restores the original defaults and allows users to configure the timeouts.

Discourse came across a similar problem with the same solution. Some SMTP servers are slow to respond: https://meta.discourse.org/t/smtp-net-readtimeout-without-relation-to-net-or-login-problems-smtp-host-is-just-slow/235727

Related issues

Relates to gitlab#410336 (closed)

Checklist

See Definition of done.

For anything in this list which will not be completed, please provide a reason in the MR discussion.

Required

  • MR title and description are up to date, accurate, and descriptive.
  • MR targeting the appropriate branch.
  • Latest Merge Result pipeline is green.
  • When ready for review, MR is labeled "~workflow::ready for review" per the Distribution MR workflow.

For GitLab team members

If you don't have access to this, the reviewer should trigger these jobs for you during the review process.

  • The manual Trigger:ee-package jobs have a green pipeline running against latest commit.
  • If config/software or config/patches directories are changed, make sure the build-package-on-all-os job within the Trigger:ee-package downstream pipeline succeeded.
  • If you are changing anything SSL related, then the Trigger:package:fips manual job within the Trigger:ee-package downstream pipeline must succeed.
  • If CI configuration is changed, the branch must be pushed to dev.gitlab.org to confirm regular branch builds aren't broken.

Expected (please provide an explanation if not completing)

  • Test plan indicating conditions for success has been posted and passes.
  • Documentation created/updated.
  • Tests added.
  • Integration tests added to GitLab QA.
  • Equivalent MR/issue for the GitLab Chart opened: gitlab-org/charts/gitlab!3156 (merged)
  • Validate potential values for new configuration settings. Formats such as integer 10, duration 10s, URI scheme://user:passwd@host:port may require quotation or other special handling when rendered in a template and written to a configuration file.
Edited by João Alexandre Cunha

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • assigned to @stanhu

  • Stan Hu changed the description

    changed the description

  • Stan Hu marked the checklist item MR title and description are up to date, accurate, and descriptive. as completed

    marked the checklist item MR title and description are up to date, accurate, and descriptive. as completed

  • Stan Hu marked the checklist item MR targeting the appropriate branch. as completed

    marked the checklist item MR targeting the appropriate branch. as completed

  • Stan Hu marked the checklist item Latest Merge Result pipeline is green. as completed

    marked the checklist item Latest Merge Result pipeline is green. as completed

  • Stan Hu changed milestone to %16.0

    changed milestone to %16.0

  • A deleted user added featureaddition typefeature labels
  • Contributor
    1 Warning
    :warning: You've made some changes at the locations which contain user facing configuration.
    That's OK as long as you're refactoring existing code and not adding any new
    configuration. If you are adding new user facing configuration, consider adding
    to gitlab.rb.template located in files/gitlab-config-template/gitlab.rb.template .
    Otherwise, please consider adding the typemaintenance label in that case.
    1 Message
    :book: Please add the workflowready for review label once you think the MR is ready to for an initial review.

    Merge requests are handled according to the workflow documented in our handbook and should receive a response within the limit documented in our First-response SLO.

    If you don't receive a response, please mention @gitlab-org/distribution, or one of our Project Maintainers

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Author Owner

    I have Postfix installed without TLS, but the timeouts should still apply. This email works:

     Notify.test_email('email@example.com', 'Message Subject', 'Message Body').deliver_now

    Testing, without settings:

    $ sudo cat /opt/gitlab/embedded/service/gitlab-rails/config/initializers/smtp_settings.rb
    # This file is managed by gitlab-ctl. Manual changes will be
    # erased! To change the contents below, edit /etc/gitlab/gitlab.rb
    # and run `sudo gitlab-ctl reconfigure`.
    
    if Rails.env.production?
      secrets = Gitlab::Email::SmtpConfig.secrets
      smtp_settings = {
        user_name: secrets.username,
        password: secrets.password,
        address: "localhost",
        port: 25,
        domain: "localhost",
        enable_starttls_auto: false,
        tls: false,
        ssl: false,
        openssl_verify_mode: "none",
    
        ca_file: "/opt/gitlab/embedded/ssl/certs/cacert.pem",
    
    
      }
    
      Gitlab::Application.config.action_mailer.delivery_method = :smtp
      ActionMailer::Base.delivery_method = :smtp
    
      ActionMailer::Base.smtp_settings = smtp_settings
    end

    Via sudo gitlab-rails console:

    irb(main):001:0> ActionMailer::Base.smtp_settings
    =>
    {:user_name=>nil,
     :password=>nil,
     :address=>"localhost",
     :port=>25,
     :domain=>"localhost",
     :enable_starttls_auto=>false,
     :tls=>false,
     :ssl=>false,
     :openssl_verify_mode=>"none",
     :ca_file=>"/opt/gitlab/embedded/ssl/certs/cacert.pem"}

    Testing, with new settings:

    gitlab_rails['smtp_read_timeout'] = 10
    gitlab_rails['smtp_open_timeout'] = 10
    $ sudo cat /opt/gitlab/embedded/service/gitlab-rails/config/initializers/smtp_settings.rb
    # This file is managed by gitlab-ctl. Manual changes will be
    # erased! To change the contents below, edit /etc/gitlab/gitlab.rb
    # and run `sudo gitlab-ctl reconfigure`.
    
    if Rails.env.production?
      secrets = Gitlab::Email::SmtpConfig.secrets
      smtp_settings = {
        user_name: secrets.username,
        password: secrets.password,
        address: "localhost",
        port: 25,
        domain: "localhost",
        enable_starttls_auto: false,
        tls: false,
        ssl: false,
        openssl_verify_mode: "none",
    
        ca_file: "/opt/gitlab/embedded/ssl/certs/cacert.pem",
        open_timeout: 10,
        read_timeout: 10,
      }
    
      Gitlab::Application.config.action_mailer.delivery_method = :smtp
      ActionMailer::Base.delivery_method = :smtp
    
      ActionMailer::Base.smtp_settings = smtp_settings
    end

    In gitlab-rails console:

    irb(main):001:0> ActionMailer::Base.smtp_settings
    =>
    {:user_name=>nil,
     :password=>nil,
     :address=>"localhost",
     :port=>25,
     :domain=>"localhost",
     :enable_starttls_auto=>false,
     :tls=>false,
     :ssl=>false,
     :openssl_verify_mode=>"none",
     :ca_file=>"/opt/gitlab/embedded/ssl/certs/cacert.pem",
     :open_timeout=>10,
     :read_timeout=>10}
  • Stan Hu marked the checklist item When ready for review, MR is labeled "~workflow::ready for review" per the Distribution MR workflow. as completed

    marked the checklist item When ready for review, MR is labeled "~workflow::ready for review" per the Distribution MR workflow. as completed

  • Stan Hu added 1 commit

    added 1 commit

    • d2e6828a - Add SMTP timeout configuration options

    Compare with previous version

  • mentioned in issue gitlab#410336 (closed)

  • Stan Hu changed the description

    changed the description

  • Stan Hu
  • Stan Hu added 1 commit

    added 1 commit

    • 50e7a5b9 - Add SMTP timeout configuration options

    Compare with previous version

  • Stan Hu resolved all threads

    resolved all threads

  • Stan Hu changed the description

    changed the description

  • Stan Hu marked the checklist item Test plan indicating conditions for success has been posted and passes. as completed

    marked the checklist item Test plan indicating conditions for success has been posted and passes. as completed

  • Stan Hu marked the checklist item Documentation created/updated. as completed

    marked the checklist item Documentation created/updated. as completed

  • Stan Hu marked the checklist item Tests added. as completed

    marked the checklist item Tests added. as completed

  • Stan Hu marked the checklist item Integration tests added to GitLab QA. as completed

    marked the checklist item Integration tests added to GitLab QA. as completed

  • Stan Hu marked the checklist item Validate potential values for new configuration settings. Formats such as integer 10, duration 10s, URI scheme://user:passwd@host:port may require quotation or other special handling when rendered in a template and written to a configuration file. as completed

    marked the checklist item Validate potential values for new configuration settings. Formats such as integer 10, duration 10s, URI scheme://user:passwd@host:port may require quotation or other special handling when rendered in a template and written to a configuration file. as completed

  • requested review from @Alexand

  • João Alexandre Cunha marked the checklist item The manual Trigger:ee-package jobs have a green pipeline running against latest commit. as completed

    marked the checklist item The manual Trigger:ee-package jobs have a green pipeline running against latest commit. as completed

  • I was able to reproduce the same and I was also able to confirm the new default [30, 60] is being applied:

    # my gitlab.rb
    
    gitlab_rails['smtp_enable'] = true
    gitlab_rails['smtp_address'] = 'localhost'
    gitlab_rails['smtp_port'] = 25
    gitlab_rails['smtp_domain'] = 'localhost'
    gitlab_rails['smtp_tls'] = false
    gitlab_rails['smtp_openssl_verify_mode'] = 'none'
    gitlab_rails['smtp_enable_starttls_auto'] = false
    gitlab_rails['smtp_ssl'] = false
    gitlab_rails['smtp_force_ssl'] = false

    After gitlab-ctl reconfigure:

    cat /opt/gitlab/embedded/service/gitlab-rails/config/initializers/smtp_settings.rb
    # This file is managed by gitlab-ctl. Manual changes will be
    # erased! To change the contents below, edit /etc/gitlab/gitlab.rb
    # and run `sudo gitlab-ctl reconfigure`.
    
    if Rails.env.production?
      secrets = Gitlab::Email::SmtpConfig.secrets
      smtp_settings = {
        user_name: secrets.username,
        password: secrets.password,
        address: "localhost",
        port: 25,
        domain: "localhost",
        enable_starttls_auto: false,
        tls: false,
        ssl: false,
        openssl_verify_mode: "none",
    
        ca_file: "/opt/gitlab/embedded/ssl/certs/cacert.pem",
        open_timeout: 30,
        read_timeout: 60,
      }
    
      Gitlab::Application.config.action_mailer.delivery_method = :smtp
      ActionMailer::Base.delivery_method = :smtp
    
      ActionMailer::Base.smtp_settings = smtp_settings
    end
    irb(main):006:0> ActionMailer::Base.smtp_settings
    =>
    {:user_name=>nil,
     :password=>nil,
     :address=>"localhost",
     :port=>25,
     :domain=>"localhost",
     :enable_starttls_auto=>false,
     :tls=>false,
     :ssl=>false,
     :openssl_verify_mode=>"none",
     :ca_file=>"/opt/gitlab/embedded/ssl/certs/cacert.pem",
     :open_timeout=>30,
     :read_timeout=>60}
  • João Alexandre Cunha marked the checklist item If config/software or config/patches directories are changed, make sure the build-package-on-all-os job within the Trigger:ee-package downstream pipeline succeeded. as completed

    marked the checklist item If config/software or config/patches directories are changed, make sure the build-package-on-all-os job within the Trigger:ee-package downstream pipeline succeeded. as completed

  • João Alexandre Cunha marked the checklist item If you are changing anything SSL related, then the Trigger:package:fips manual job within the Trigger:ee-package downstream pipeline must succeed. as completed

    marked the checklist item If you are changing anything SSL related, then the Trigger:package:fips manual job within the Trigger:ee-package downstream pipeline must succeed. as completed

  • João Alexandre Cunha marked the checklist item If CI configuration is changed, the branch must be pushed to dev.gitlab.org to confirm regular branch builds aren't broken. as completed

    marked the checklist item If CI configuration is changed, the branch must be pushed to dev.gitlab.org to confirm regular branch builds aren't broken. as completed

  • João Alexandre Cunha approved this merge request

    approved this merge request

  • Thanks, Stan. It all LGTM. :sailboat:

    @balasankarc, could you please take the maintainer review?

  • requested review from @balasankarc

  • João Alexandre Cunha removed review request for @Alexand

    removed review request for @Alexand

  • Stan Hu mentioned in merge request !6887 (merged)

    mentioned in merge request !6887 (merged)

  • removed Pick into 15.10 label

  • Stan Hu mentioned in merge request !6888 (merged)

    mentioned in merge request !6888 (merged)

  • removed Pick into 15.11 label

  • Balasankar 'Balu' C approved this merge request

    approved this merge request

  • mentioned in commit 3e42fbda

  • added workflowstaging label and removed workflowcanary label

  • Stan Hu mentioned in merge request gitlab!124757 (merged)

    mentioned in merge request gitlab!124757 (merged)

  • Please register or sign in to reply
    Loading