Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/matrix-synapse: migrate to generators.toYAML and other cleanup #120260

Closed

Conversation

sumnerevans
Copy link
Contributor

@sumnerevans sumnerevans commented Apr 22, 2021

Motivation for this change

I am doing all of these deprecations at once to try and avoid mkChangedOptionModule chains.

Note to Reviewers
  • I recommend going through each commit one-by-one.
Manual testing performed
Test 1

I tested using the following config:

Test 1 Nix config
services.matrix-synapse = {
  enable = true;
  enable_metrics = true;
  enable_registration = false;
  database_type = "sqlite3";
  server_name = "localhost";
  max_upload_size = "250M";
  listeners = [
    {
      port = 8008;
      bind_address = "::1";
      type = "http";
      tls = false;
      x_forwarded = true;
      resources = [
        {
          names = [ "client" "federation" ];
          compress = false;
        }
      ];
    }
  ];
};
  • I ensured that Synapse runs without error.
  • I inspected the systemd service config to ensure that everything looked right. This is the ExecStart line:
    ExecStart=/nix/store/8gbwdy0m7hda76mjycs7pifyry172kci-matrix-synapse-1.34.0/bin/homeserver \
      --config-path /nix/store/pihg9l7jgy2ii9hs8w4h8kn97n73xv60-homeserver.yaml --config-path /nix/store/47xi81pm06p6cpm211wl68cxac4zl0z8-extra-homeserver.yaml \
      --keys-directory /var/lib/matrix-synapse
    
  • I looked at the generated homeserver.yaml file to ensure that everything looked correct. Here is the generated config (piped through jq for clarity):
Generated config (piped through `jq`)
{
  "account_threepid_delegates": {},
  "allow_guest_access": false,
  "app_service_config_files": [],
  "bcrypt_rounds": 12,
  "database": {
    "args": {
      "database": "/var/lib/matrix-synapse/homeserver.db"
    },
    "name": "sqlite3"
  },
  "dynamic_thumbnails": false,
  "enable_metrics": true,
  "enable_registration": false,
  "enable_registration_captcha": false,
  "event_cache_size": "10K",
  "key_refresh_interval": "1d",
  "listeners": [
    {
      "bind_address": "::1",
      "port": 8008,
      "resources": [
        {
          "compress": false,
          "names": [
            "client",
            "federation"
          ]
        }
      ],
      "tls": false,
      "type": "http",
      "x_forwarded": true
    }
  ],
  "log_config": "/nix/store/2x89r9ji20wy2sggd017jfhlmn9ygsj3-log_config.yaml",
  "max_image_pixels": "32M",
  "max_upload_size": "250M",
  "media_store_path": "/var/lib/matrix-synapse/media",
  "no_tls": false,
  "perspectives": {
    "servers": {
      "matrix.org": {
        "verify_keys": {
          "ed25519:auto": {
            "key": "Noi6WqcDj0QmPxCNQqgezwTlBKrfqehY1u2FyWP9uYw"
          }
        }
      }
    }
  },
  "pid_file": "/run/matrix-synapse.pid",
  "rc_federation": {
    "concurrent": 3,
    "reject_limit": 50,
    "sleep_delay": 500,
    "sleep_limit": 10,
    "window_size": 1000
  },
  "rc_message": {
    "burst_count": 10,
    "per_second": 0.2
  },
  "recaptcha_private_key": "",
  "recaptcha_public_key": "",
  "recaptcha_siteverify_api": "https://www.google.com/recaptcha/api/siteverify",
  "redaction_retention_period": 7,
  "report_stats": false,
  "room_prejoin_state": {
    "additional_event_types": [],
    "disable_default_event_types": false
  },
  "server_name": "localhost",
  "signing_key_path": "/var/lib/matrix-synapse/homeserver.signing.key",
  "turn_shared_secret": "",
  "turn_uris": [],
  "turn_user_lifetime": "1h",
  "verbose": 0
}
Test 2 (deprecated conversions)

I tested using the following config:

Test 2 Nix config
  services.matrix-synapse = {
    enable = true;
    enable_metrics = true;
    enable_registration = false;

    rc_messages_per_second = "1.2";
    rc_message_burst_count = "11.0";
    federation_rc_window_size = "1024";
    federation_rc_sleep_limit = "42";
    federation_rc_sleep_delay = "404";
    federation_rc_reject_limit = "80";
    federation_rc_concurrent = "4";

    database_type = "sqlite3";
    server_name = "localhost";
    max_upload_size = "250M";
    listeners = [
      {
        port = 8008;
        bind_address = "::1";
        type = "http";
        tls = false;
        x_forwarded = true;
        resources = [
          {
            names = [ "client" "federation" ];
            compress = false;
          }
        ];
      }
    ];
  };
  • The following warnings were shown:

    trace: warning: The option `services.matrix-synapse.federation_rc_concurrent' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_federation.concurrent' that has a different type. Please read `services.matrix-synapse.rc_federation.concurrent' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.federation_rc_reject_limit' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_federation.reject_limit' that has a different type. Please read `services.matrix-synapse.rc_federation.reject_limit' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.federation_rc_sleep_delay' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_federation.sleep_delay' that has a different type. Please read `services.matrix-synapse.rc_federation.sleep_delay' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.federation_rc_sleep_limit' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_federation.sleep_limit' that has a different type. Please read `services.matrix-synapse.rc_federation.sleep_limit' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.federation_rc_window_size' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_federation.window_size' that has a different type. Please read `services.matrix-synapse.rc_federation.window_size' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.rc_message_burst_count' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_message.burst_count' that has a different type. Please read `services.matrix-synapse.rc_message.burst_count' documentation and update your configuration accordingly.
    trace: warning: The option `services.matrix-synapse.rc_messages_per_second' defined in `/etc/nixos/host-configurations/coruscant.nix' has been changed to `services.matrix-synapse.rc_message.per_second' that has a different type. Please read `services.matrix-synapse.rc_message.per_second' documentation and update your configuration accordingly.
    
  • I ensured that Synapse runs without error.

  • I looked at the generated homeserver.yaml file to ensure that everything looked correct. Here is the generated config (piped through jq for clarity):

Generated config (piped through `jq`)
{
  "account_threepid_delegates": {},
  "allow_guest_access": false,
  "app_service_config_files": [],
  "bcrypt_rounds": 13,
  "database": {
    "args": {
      "database": "/var/lib/matrix-synapse/homeserver.db"
    },
    "name": "sqlite3"
  },
  "dynamic_thumbnails": false,
  "enable_metrics": true,
  "enable_registration": false,
  "enable_registration_captcha": false,
  "event_cache_size": "10K",
  "key_refresh_interval": "1d",
  "listeners": [
    {
      "bind_address": "::1",
      "port": 8008,
      "resources": [
        {
          "compress": false,
          "names": [
            "client",
            "federation"
          ]
        }
      ],
      "tls": false,
      "type": "http",
      "x_forwarded": true
    }
  ],
  "log_config": "/nix/store/2x89r9ji20wy2sggd017jfhlmn9ygsj3-log_config.yaml",
  "max_image_pixels": "32M",
  "max_upload_size": "250M",
  "media_store_path": "/var/lib/matrix-synapse/media",
  "no_tls": false,
  "perspectives": {
    "servers": {
      "matrix.org": {
        "verify_keys": {
          "ed25519:auto": {
            "key": "Noi6WqcDj0QmPxCNQqgezwTlBKrfqehY1u2FyWP9uYw"
          }
        }
      }
    }
  },
  "pid_file": "/run/matrix-synapse.pid",
  "rc_federation": {
    "concurrent": 4,
    "reject_limit": 80,
    "sleep_delay": 404,
    "sleep_limit": 42,
    "window_size": 1024
  },
  "rc_message": {
    "burst_count": 11,
    "per_second": 1.2
  },
  "recaptcha_private_key": "",
  "recaptcha_public_key": "",
  "recaptcha_siteverify_api": "https://www.google.com/recaptcha/api/siteverify",
  "redaction_retention_period": 7,
  "report_stats": false,
  "room_prejoin_state": {
    "additional_event_types": [],
    "disable_default_event_types": false
  },
  "server_name": "localhost",
  "signing_key_path": "/var/lib/matrix-synapse/homeserver.signing.key",
  "turn_shared_secret": "",
  "turn_uris": [],
  "turn_user_lifetime": "1h",
  "verbose": 10
}
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch from 8d783b4 to f3062d3 Compare April 25, 2021 22:33
@sumnerevans sumnerevans marked this pull request as ready for review April 25, 2021 22:41
@sumnerevans sumnerevans changed the title WIP: nixos/matrix-synapse: start migration to generators.toYAML nixos/matrix-synapse: start migration to generators.toYAML Apr 26, 2021
@sumnerevans sumnerevans changed the title nixos/matrix-synapse: start migration to generators.toYAML nixos/matrix-synapse: migrate to generators.toYAML Apr 26, 2021
@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch 2 times, most recently from 27f2410 to a232790 Compare May 27, 2021 16:16
@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch from 40f5be3 to d3e8994 Compare May 29, 2021 05:31
@sumnerevans sumnerevans changed the title nixos/matrix-synapse: migrate to generators.toYAML nixos/matrix-synapse: migrate to generators.toYAML and general cleanup May 29, 2021
@sumnerevans sumnerevans changed the title nixos/matrix-synapse: migrate to generators.toYAML and general cleanup nixos/matrix-synapse: migrate to generators.toYAML and other cleanup May 29, 2021
@sumnerevans sumnerevans marked this pull request as ready for review May 29, 2021 06:06
@sumnerevans
Copy link
Contributor Author

@ajs124 how did you inspect the test machine and find the contents of the files?

@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch from d3e8994 to 3dffbc2 Compare May 29, 2021 06:11
Also, add the neccessary mkChangedOptionModule imports.

This matches what is in the ratelimiting config in Synapse. See
https://github.com/matrix-org/synapse/blob/develop/docs/sample_config.yaml#L822
Not sure why they were ever strings. This converts the strings to
integers if that was how th user specified it.
@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch from 3dffbc2 to 20c1540 Compare May 29, 2021 06:40
@sumnerevans sumnerevans requested review from ajs124 and benley May 29, 2021 06:43
@ajs124
Copy link
Member

ajs124 commented Jun 1, 2021

@ajs124 how did you inspect the test machine and find the contents of the files?

I evaluated the test from master and with this PR applied. Then I used nix-store -qR | grep homeserver.yaml to get the config file derivations and then realized those drvs.

=> error: Could not convert 1 to float.
*/
# Obviously, it is a bit hacky to use fromJSON this way.
toFloat = str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm qualified to approve this, although I guess it seems fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infinisil (or somebody else), could you maybe approve this addition to lib/strings.nix

@ajs124
Copy link
Member

ajs124 commented Jun 1, 2021

This PR removes the user_creation_max_duration option (not found in synapse codebase at all)

Was removed in matrix-org/synapse@1c4f05d

@sumnerevans sumnerevans force-pushed the matrix-synapse-yaml-generator branch from 20c1540 to aeb092a Compare June 1, 2021 12:35
@sumnerevans
Copy link
Contributor Author

This PR removes the user_creation_max_duration option (not found in synapse codebase at all)

Was removed in matrix-org/synapse@1c4f05d

Thanks for finding this! I've updated the commit message.

@sumnerevans sumnerevans mentioned this pull request Jun 1, 2021
9 tasks
@sumnerevans sumnerevans requested a review from ajs124 June 1, 2021 15:07

configFile = pkgs.writeText "homeserver.yaml" (
generators.toYAML { } (filterAttrs (_: v: v != null)
(fold recursiveUpdate { } [ yamlConfig cfg.settings ])));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, set the defaults in the config section by assigning to services.matrix-synapse directly with mkDefault's. Check out https://nixos.org/manual/nixos/stable/index.html#ex-settings-nix-representable for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand what you mean. I don't think I am setting any defaults here... it's just collapsing all of the individual options that already exist on the module into a single attrset so that it can be converted to YAML.

Copy link
Member

@infinisil infinisil Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. Still should be done this way though. E.g. see the how the ZNC module provides support for legacy options (ignore the mkOverride 900 bit):

services.znc.config = let
c = cfg.confOptions;
# defaults here should override defaults set in the non-legacy part
mkDefault = mkOverride 900;
in {
LoadModule = mkDefault c.modules;
Listener.l = {
Port = mkDefault c.port;
IPv4 = mkDefault true;
IPv6 = mkDefault true;
SSL = mkDefault c.useSSL;
URIPrefix = c.uriPrefix;
};
User.${c.userName} = {
Admin = mkDefault true;
Nick = mkDefault c.nick;
AltNick = mkDefault "${c.nick}_";
Ident = mkDefault c.nick;
RealName = mkDefault c.nick;
LoadModule = mkDefault c.userModules;
Network = mapAttrs (name: net: {
LoadModule = mkDefault net.modules;
Server = mkDefault "${net.server} ${optionalString net.useSSL "+"}${toString net.port} ${net.password}";
Chan = optionalAttrs net.hasBitlbeeControlChannel { "&bitlbee" = mkDefault {}; } //
listToAttrs (map (n: nameValuePair "#${n}" (mkDefault {})) net.channels);
extraConfig = if net.extraConf == "" then mkDefault null else net.extraConf;
}) c.networks;
extraConfig = [ c.passBlock ];
};
extraConfig = optional (c.extraZncConf != "") c.extraZncConf;
};

The reason it should be done this way is because otherwise if other modules try to access config.services.matrix-synapse.settings, they won't have the full view of the set options, but rather only a partial one that doesn't include the ones from yamlConfig here. And in general recursiveUpdate should be avoided in modules, because the module system already supports merging by itself, and a much better version of it.

};

configFile = pkgs.writeText "homeserver.yaml" (
generators.toYAML { } (filterAttrs (_: v: v != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check out #75584, which shows how you can use pkgs.formats.yaml for both the type and the generator in one.

And the filterAttrs here looks like it only filters top-level null's out, is that intended like this? And is there a problem with leaving in the nulls?

@mweinelt
Copy link
Member

Superseded by #158605.

@mweinelt mweinelt closed this Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants