1. 19 Apr, 2021 2 commits
    • Harkrishn Patro's avatar
      ACL channels permission handling for save/load scenario. (#8794) · 7a3d1487
      Harkrishn Patro authored
      
      
      In the initial release of Redis 6.2 setting a user to only allow pubsub access to
      a specific channel, and doing ACL SAVE, resulted in an assertion when
      ACL LOAD was used. This was later changed by #8723 (not yet released),
      but still not properly resolved (now it errors instead of crash).
      
      The problem is that the server that generates an ACL file, doesn't know what
      would be the setting of the acl-pubsub-default config in the server that will load it.
      so ACL SAVE needs to always start with resetchannels directive.
      
      This should still be compatible with old acl files (from redis 6.0), and ones from earlier
      versions of 6.2 that didn't mess with channels.
      Co-authored-by: default avatarHarkrishn Patro <harkrisp@amazon.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7a3d1487
    • sundb's avatar
      Fix ouput buffer limit test (#8803) · 3a955d9a
      sundb authored
      The tail size of c->reply is 16kb, but in the test only publish a
      few chars each time, due to a change in #8699, the obuf limit
      is now checked a new memory allocation is made, so this test
      would have sometimes failed to trigger a soft limit disconnection
      in time.
      
      The solution is to write bigger payloads to the output buffer, but
      still limit their rate (not more than 100k/s).
      3a955d9a
  2. 18 Apr, 2021 3 commits
    • Yossi Gottlieb's avatar
      Revert cluster slot migration tests. (#8806) · c0f5c678
      Yossi Gottlieb authored
      Disables #8649 and subsequent attempts to stabilize the test.
      c0f5c678
    • Oran Agra's avatar
      Fix timing of new replication test (#8807) · a9897b00
      Oran Agra authored
      In github actions CI with valgrind, i saw that even the fast replica
      (one that wasn't paused), didn't get to complete the replication fast
      enough, and ended up getting disconnected by timeout.
      
      Additionally, due to a typo in uname, we didn't get to actually run the
      CPU efficiency part of the test.
      a9897b00
    • Oran Agra's avatar
      Improve testsuite print of log file (#8805) · f4b5a4d8
      Oran Agra authored
      1. the `dump_logs` option would have printed only logs of servers that were
         spawn before the test proc started, and not ones that the test proc
         started inside it.
      2. when a server proc catches an exception it should normally forward the
         exception upwards, specifically when it's an assertion that should be
         caught by a test proc above. however, in `durable` mode, we caught all
         exceptions printed them to stdout and let the code continue,
         this was wrong to do for assertions, which should have still been
         propagated to the test function.
      3. don't bother to search for crash log to print if we printed the the
         entire log anyway
      4. if no crash log was found, no need to print anything (i.e. the fact it
         wasn't found)
      5. rename warnings_from_file to crashlog_from_file
      f4b5a4d8
  3. 15 Apr, 2021 1 commit
    • guybe7's avatar
      Add a timeout mechanism for replicas stuck in fullsync (#8762) · d63d0260
      guybe7 authored
      Starting redis 6.0 (part of the TLS feature), diskless master uses pipe from the fork
      child so that the parent is the one sending data to the replicas.
      This mechanism has an issue in which a hung replica will cause the master to wait
      for it to read the data sent to it forever, thus preventing the fork child from terminating
      and preventing the creations of any other forks.
      
      This PR adds a timeout mechanism, much like the ACK-based timeout,
      we disconnect replicas that aren't reading the RDB file fast enough.
      d63d0260
  4. 14 Apr, 2021 1 commit
  5. 13 Apr, 2021 2 commits
  6. 12 Apr, 2021 1 commit
    • Yossi Gottlieb's avatar
      Fix failing cluster tests. (#8763) · 5e3a15ae
      Yossi Gottlieb authored
      Disable replica migration to avoid a race condition where the
      migrated-from node turns into a replica.
      
      Long term, this test should probably be improved to handle multiple
      slots and accept such auto migrations but this is a quick fix to
      stabilize the CI without completely dropping this test.
      5e3a15ae
  7. 07 Apr, 2021 1 commit
  8. 06 Apr, 2021 2 commits
  9. 05 Apr, 2021 1 commit
    • Huang Zhw's avatar
      Fix "default" and overwritten / reset users will not have pubsub channels... · 3b74b550
      Huang Zhw authored
      Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. (#8723)
      
      Background:
      Redis 6.2 added ACL control for pubsub channels (#7993), which were supposed
      to be permissive by default to retain compatibility with redis 6.0 ACL. 
      But due to a bug, only newly created users got this `acl-pubsub-default` applied,
      while overwritten (updated) users got reset to `resetchannels` (denied).
      
      Since the "default" user exists before loading the config file,
      any ACL change to it, results in an update / overwrite.
      
      So when a "default" user is loaded from config file or include ACL
      file with no channels related rules, the user will not have any
      permissions to any channels. But other users will have default
      permissions to any channels.
      
      When upgraded from 6.0 with config rewrite, this will lead to
      "default" user channels permissions lost.
      When users are loaded from include file, then call "acl load", users
      will also lost channels permissions.
      
      Similarly, the `reset` ACL rule, would have reset the user to be denied
      access to any channels, ignoring `acl-pubsub-default` and breaking
      compatibility with redis 6.0.
      
      The implication of this fix is that it regains compatibility with redis 6.0,
      but breaks compatibility with redis 6.2.0 and 2.0.1. e.g. after the upgrade,
      the default user will regain access to pubsub channels.
      
      Other changes:
      Additionally this commit rename server.acl_pubusub_default to
      server.acl_pubsub_default and fix typo in acl tests.
      3b74b550
  10. 04 Apr, 2021 1 commit
    • Sokolov Yura's avatar
      Add cluster-allow-replica-migration option. (#5285) · 1cab9620
      Sokolov Yura authored
      
      
      Previously (and by default after commit) when master loose its last slot
      (due to migration, for example), its replicas will migrate to new last slot
      holder.
      
      There are cases where this is not desired:
      * Consolidation that results with removed nodes (including the replica, eventually).
      * Manually configured cluster topologies, which the admin wishes to preserve.
      
      Needlessly migrating a replica triggers a full synchronization and can have a negative impact, so
      we prefer to be able to avoid it where possible.
      
      This commit adds 'cluster-allow-replica-migration' configuration option that is
      enabled by default to preserve existed behavior. When disabled, replicas will
      not be auto-migrated.
      
      Fixes #4896
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      1cab9620
  11. 01 Apr, 2021 4 commits
    • Valentino Geron's avatar
      Fix XAUTOCLAIM response to return the next available id as the cursor (#8725) · 44d8b039
      Valentino Geron authored
      This command used to return the last scanned entry id as the cursor,
      instead of the next one to be scanned.
      so in the next call, the user could / should have sent `(cursor` and not
      just `cursor` if he wanted to avoid scanning the same record twice.
      
      Scanning the record twice would look odd if someone is checking what
      exactly was scanned, but it also has a side effect of incrementing the
      delivery count twice.
      44d8b039
    • Oran Agra's avatar
      Solve sentinel test issue in TLS due to recent tests change. (#8728) · 370ab4c4
      Oran Agra authored
      5629dbe7 added a change that configures the tcp (plaintext) port
      alongside the tls port, this causes the INFO command for tcp_port
      to return that instead of the tls port when running in tls, and that broke
      the sentinel tests that query it.
      
      the fix is to add a method that gets the right port from CONFIG instead
      of relying on the tcp_port info field.
      370ab4c4
    • guybe7's avatar
      zsetAdd: Fix wrong reply in case of INCR and GT/LT (#8717) · 843f769b
      guybe7 authored
      If GT/LT fails the operation we need to reply with
      nill (like failure due to NX).
      
      Other changes:
      Add the missing $encoding suffix to many zset tests
      
      Note: there's a behavior change just in case of INCR + GT/LT that fails.
      The old code was replying with the wrong (rejected) score, and now it'll reply with nil.
      
      Note that that's anyway a corner case so this "behavior change" shouldn't have too much affect.
      Using GT/LT with INCR has a predictable result even before we run the command
      (INCR GT will only only / always fail if the increment is negative).
      843f769b
    • sundb's avatar
      Use chi-square for random distributivity verification in test (#8709) · 569a3f45
      sundb authored
      Problem:
      Currently, when performing random distribution verification, we determine
      the probability of each element occurring in the sum, but the probability is
      only an estimate, these tests had rare sporadic failures, and we cannot verify
      what the probability of failure will be.
      
      Solution:
      Using the chi-square distribution instead of the original random distribution
      validation makes the test more reasonable and easier to find problems.
      569a3f45
  12. 30 Mar, 2021 5 commits
    • Jérôme Loyet's avatar
      Add replica-announced config option (#8653) · 91f4f416
      Jérôme Loyet authored
      The 'sentinel replicas <master>' command will ignore replicas with
      `replica-announced` set to no.
      
      The goal of disabling the config setting replica-announced is to allow ghost
      replicas. The replica is in the cluster, synchronize with its master, can be
      promoted to master and is not exposed to sentinel clients. This way, it is
      acting as a live backup or living ghost.
      
      In addition, to prevent the replica to be promoted as master, set
      replica-priority to 0.
      91f4f416
    • Yossi Gottlieb's avatar
      Cluster migration test cleanup. (#8726) · 6a052af8
      Yossi Gottlieb authored
      * Dump more output on error (always, cluster tests currently have no
      verbose flag).
      * Slow down redis-cli check iteration.
      6a052af8
    • Viktor Söderqvist's avatar
      Add support for plaintext clients in TLS cluster (#8587) · 5629dbe7
      Viktor Söderqvist authored
      The cluster bus is established over TLS or non-TLS depending on the configuration tls-cluster. The client ports distributed in the cluster and sent to clients are assumed to be TLS or non-TLS also depending on tls-cluster.
      
      The cluster bus is now extended to also contain the non-TLS port of clients in a TLS cluster, when available. The non-TLS port of a cluster node, when available, is sent to clients connected without TLS in responses to CLUSTER SLOTS, CLUSTER NODES, CLUSTER SLAVES and MOVED and ASK redirects, instead of the TLS port.
      
      The user was able to override the client port by defining cluster-announce-port. Now cluster-announce-tls-port is added, so the user can define an alternative announce port for both TLS and non-TLS clients.
      
      Fixes #8134
      5629dbe7
    • JunhuaY's avatar
      re-fix config rewrite for empty save directive (#8722) · 28375ff6
      JunhuaY authored
      the bug was also discussed in #8716, and was solved in #8719, but incompletely:
      when the server is started, and the save option is default, if you issue the " config set save "" "
      to change the save option, and then issue the “config rewrite” command, the " save "" " won't be saved.
      28375ff6
    • Oran Agra's avatar
      solve race conditions in psync2-pingoff test (#8720) · cd81dcf1
      Oran Agra authored
      Another test race condition in the macos tests.
      the test was waiting for PINGs to be generated and put on the replication stream,
      but waiting for 1 or 2 seconds doesn't really guarantee that.
      then the test that expected 6 full syncs, found only 4
      cd81dcf1
  13. 29 Mar, 2021 3 commits
  14. 26 Mar, 2021 1 commit
    • Huang Zhw's avatar
      make processCommand check publish channel permissions. (#8534) · e138698e
      Huang Zhw authored
      Add publish channel permissions check in processCommand.
      
      processCommand didn't check publish channel permissions, so we can
      queue a publish command in a transaction. But when exec the transaction,
      it will fail with -NOPERM.
      
      We also union keys/commands/channels permissions check togegher in
      ACLCheckAllPerm. Remove pubsubCheckACLPermissionsOrReply in 
      publishCommand/subscribeCommand/psubscribeCommand. Always 
      check permissions in processCommand/execCommand/
      luaRedisGenericCommand.
      e138698e
  15. 25 Mar, 2021 1 commit
    • Oran Agra's avatar
      Fix SLOWLOG for blocked commands (#8632) · 497351ad
      Oran Agra authored
      * SLOWLOG didn't record anything for blocked commands because the client
        was reset and argv was already empty. there was a fix for this issue
        specifically for modules, now it works for all blocked clients.
      * The original command argv (before being re-written) was also reset
        before adding the slowlog on behalf of the blocked command.
      * Latency monitor is now updated regardless of the slowlog flags of the
        command or its execution (their purpose is to hide sensitive info from
        the slowlog, not hide the fact the latency happened).
      * Latency monitor now uses real_cmd rather than c->cmd (which may be
        different if the command got re-written, e.g. GEOADD)
      
      Changes:
      * Unify shared code between slowlog insertion in call() and
        updateStatsOnUnblock(), hopefully prevent future bugs from happening
        due to the later being overlooked.
      * Reset CLIENT_PREVENT_LOGGING in resetClient rather than after command
        processing.
      * Add a test for SLOWLOG and BLPOP
      
      Notes:
      - real_cmd == c->lastcmd, except inside MULTI and Lua.
      - blocked commands never happen in these cases (MULTI / Lua)
      - real_cmd == c->cmd, except for when the command is rewritten (e.g.
        GEOADD)
      - blocked commands (currently) are never rewritten
      - other than the command's CLIENT_PREVENT_LOGGING, and the
        execution flag CLIENT_PREVENT_LOGGING, other cases that we want to
        avoid slowlog are on AOF loading (specifically CMD_CALL_SLOWLOG will
        be off when executed from execCommand that runs from an AOF)
      497351ad
  16. 24 Mar, 2021 3 commits
  17. 22 Mar, 2021 3 commits
  18. 17 Mar, 2021 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Fix script kill to work also on scripts that use pcall (#8661) · 9ae4f5c7
      Meir Shpilraien (Spielrein) authored
      pcall function runs another LUA function in protected mode, this means
      that any error will be caught by this function and will not stop the LUA
      execution. The script kill mechanism uses error to stop the running script.
      Scripts that uses pcall can catch the error raise by the script kill mechanism,
      this will cause a script like this to be unkillable:
      
      local f = function()
              while 1 do
                      redis.call('ping')
              end
      end
      while 1 do
              pcall(f)
      end
      
      The fix is, when we want to kill the script, we set the hook function to be invoked 
      after each line. This will promise that the execution will get another
      error before it is able to enter the pcall function again.
      9ae4f5c7
  19. 16 Mar, 2021 2 commits
    • Huang Zhw's avatar
      When tests exit normally, some processes may still be alive (#8647) · a19c4058
      Huang Zhw authored
      In certain scenario start_server may think it failed to start a redis server
      although it started successfully. in these cases, it'll not terminate it, and
      it'll remain running when the test is over.
      
      In start_server if config doesn't have bind (the minimal.conf in introspection.tcl),
      it will try to bind ipv4 and ipv6. One may success while other fails. It will
      output "Could not create server TCP listening socket".
      wait_server_started uses this message to check whether instance started
      successfully. So it will consider that it failed even though redis started successfully.
      
      Additionally, in some cases it wasn't clear to users why the server exited,
      since the warning message printed to the log, could in some cases be harmless,
      and in some cases fatal.
      
      This PR adds makes a clear distinction between a warning log message and
      a fatal one, and changes the test suite to look for the fatal message.
      a19c4058
    • Madelyn Olson's avatar
      Redact slowlog entries for config with sensitive data. (#8584) · e1d98bca
      Madelyn Olson authored
      Redact config set requirepass/masterauth/masteruser from slowlog in addition to showing ACL commands without sensitive values. 
      e1d98bca
  20. 15 Mar, 2021 1 commit
    • guybe7's avatar
      Missing EXEC on modules propagation after failed EVAL execution (#8654) · dba33a94
      guybe7 authored
      1. moduleReplicateMultiIfNeeded should use server.in_eval like
         moduleHandlePropagationAfterCommandCallback
      2. server.in_eval could have been set to 1 and not reset back
         to 0 (a lot of missed early-exits after in_eval is already 1)
      
      Note: The new assertions in processCommand cover (2) and I added
      two module tests to cover (1)
      
      Implications:
      If an EVAL that failed (and thus left server.in_eval=1) runs before a module
      command that replicates, the replication stream will contain MULTI (because
      moduleReplicateMultiIfNeeded used to check server.lua_caller which is NULL
      at this point) but not EXEC (because server.in_eval==1)
      This only affects modules as module.c the only user of server.in_eval.
      
      Affects versions 6.2.0, 6.2.1
      dba33a94
  21. 12 Mar, 2021 1 commit