ericsson / ered Goto Github PK

There are some quirks regarding pubsub. SUBSCRIBE and other commands don't really return anything, just push messages. When subscribing to multiple channels, ered currently gets out of sync. Thus, we can't say that pubsub is supported yet.

Minimize the copying of data between processes. Do as much as possible in the calling process.

The readme file should have examples of how to use the library as well as an overview of its API.

I'd like to propose to evaluate and (selectively) adopt secure software development best practices recommended by the Open Source Security Foundation (OpenSSF) [1]. The OpenSSF Scorecard project checks various development best practices of open source projects hosted on GitHub and provides guidance on how to improve those practices [2]. The overall goal of this issue is to strengthen the (supply chain) security posture of the ered project.

[1] https://openssf.org/
[2] https://github.com/ossf/scorecard/tree/main#scorecard-checks

Below are the results as of today. I would recommend looking into

• the branch protection settings

• pinning the versions of GitHub actions

• enabling dependabot (at least for the GitHub actions)

  {
  "date": "2024-03-20T23:07:30+01:00",
  "repo": {
  "name": "github.com/Ericsson/ered",
  "commit": "fd716d22cf10912ff140e46f7a2aca413c80066f"
  },
  "scorecard": {
  "version": "(devel)",
  "commit": "unknown"
  },
  "score": 4.2,
  "checks": [
  {
  "details": null,
  "score": 10,
  "reason": "no binaries found in the repo",
  "name": "Binary-Artifacts",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#binary-artifacts",
  "short": "Determines if the project has generated executable (binary) artifacts in the source repository."
  }
  },
  {
  "details": [
  "Warn: 'force pushes' enabled on branch 'main'",
  "Info: 'allow deletion' disabled on branch 'main'",
  "Warn: status checks do not require up-to-date branches for 'main'",
  "Warn: 'last push approval' disabled on branch 'main'",
  "Info: PRs are required in order to make changes on branch 'main'",
  "Warn: no status checks found to merge onto branch 'main'",
  "Warn: number of required reviewers is 0 on branch 'main', while the ideal suggested is 2",
  "Warn: stale review dismissal disabled on branch 'main'",
  "Warn: settings do not apply to administrators on branch 'main'",
  "Warn: codeowner review is not required on branch 'main'"
  ],
  "score": 1,
  "reason": "branch protection is not maximal on development and all release branches",
  "name": "Branch-Protection",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection",
  "short": "Determines if the default and release branches are protected with GitHub's branch protection settings."
  }
  },
  {
  "details": null,
  "score": 9,
  "reason": "21 out of 23 merged PRs checked by a CI test -- score normalized to 9",
  "name": "CI-Tests",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#ci-tests",
  "short": "Determines if the project runs tests before pull requests are merged."
  }
  },
  {
  "details": null,
  "score": 0,
  "reason": "no effort to earn an OpenSSF best practices badge detected",
  "name": "CII-Best-Practices",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#cii-best-practices",
  "short": "Determines if the project has an OpenSSF (formerly CII) Best Practices Badge."
  }
  },
  {
  "details": null,
  "score": 7,
  "reason": "found 7 unreviewed changesets out of 26 -- score normalized to 7",
  "name": "Code-Review",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#code-review",
  "short": "Determines if the project requires human code review before pull requests (aka merge requests) are merged."
  }
  },
  {
  "details": [
  "Info: mysql-otp contributor org/company found, ericsson software technology contributor org/company found, ericsson contributor org/company found, "
  ],
  "score": 10,
  "reason": "project has 3 contributing companies or organizations -- score normalized to 10",
  "name": "Contributors",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#contributors",
  "short": "Determines if the project has a set of contributors from multiple organizations (e.g., companies)."
  }
  },
  {
  "details": null,
  "score": 10,
  "reason": "no dangerous workflow patterns detected",
  "name": "Dangerous-Workflow",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#dangerous-workflow",
  "short": "Determines if the project's GitHub Action workflows avoid dangerous patterns."
  }
  },
  {
  "details": [
  "Warn: tool 'RenovateBot' is not used",
  "Warn: tool 'Dependabot' is not used",
  "Warn: tool 'PyUp' is not used"
  ],
  "score": 0,
  "reason": "no update tool detected",
  "name": "Dependency-Update-Tool",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#dependency-update-tool",
  "short": "Determines if the project uses a dependency update tool."
  }
  },
  {
  "details": [
  "Warn: no OSSFuzz integration found",
  "Warn: no GoBuiltInFuzzer integration found",
  "Warn: no PythonAtherisFuzzer integration found",
  "Warn: no CLibFuzzer integration found",
  "Warn: no CppLibFuzzer integration found",
  "Warn: no SwiftLibFuzzer integration found",
  "Warn: no RustCargoFuzzer integration found",
  "Warn: no JavaJazzerFuzzer integration found",
  "Warn: no ClusterFuzzLite integration found",
  "Warn: no HaskellPropertyBasedTesting integration found",
  "Warn: no TypeScriptPropertyBasedTesting integration found",
  "Warn: no JavaScriptPropertyBasedTesting integration found"
  ],
  "score": 0,
  "reason": "project is not fuzzed",
  "name": "Fuzzing",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#fuzzing",
  "short": "Determines if the project uses fuzzing."
  }
  },
  {
  "details": [
  "Info: FSF or OSI recognized license: LICENSE:1",
  "Info: License file found in expected location: LICENSE:1"
  ],
  "score": 10,
  "reason": "license file detected",
  "name": "License",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#license",
  "short": "Determines if the project has defined a license."
  }
  },
  {
  "details": null,
  "score": 1,
  "reason": "2 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 1",
  "name": "Maintained",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#maintained",
  "short": "Determines if the project is "actively maintained"."
  }
  },
  {
  "details": [
  "Warn: no GitHub/GitLab publishing workflow detected."
  ],
  "score": -1,
  "reason": "packaging workflow not detected",
  "name": "Packaging",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#packaging",
  "short": "Determines if the project is published as a package that others can easily download, install, easily update, and uninstall."
  }
  },
  {
  "details": [
  "Warn: GitHub-owned GitHubAction not pinned by hash: .github/workflows/ci.yml:13",
  "Warn: GitHub-owned GitHubAction not pinned by hash: .github/workflows/ci.yml:38",
  "Warn: GitHub-owned GitHubAction not pinned by hash: .github/workflows/redis-compatibility.yml:16",
  "Warn: third-party GitHubAction not pinned by hash: .github/workflows/redis-compatibility.yml:18",
  "Info: 0 out of 3 GitHub-owned GitHubAction dependencies pinned",
  "Info: 0 out of 1 third-party GitHubAction dependencies pinned"
  ],
  "score": 0,
  "reason": "dependency not pinned by hash detected -- score normalized to 0",
  "name": "Pinned-Dependencies",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies",
  "short": "Determines if the project has declared and pinned the dependencies of its build process."
  }
  },
  {
  "details": [
  "Warn: 0 commits out of 27 are checked with a SAST tool"
  ],
  "score": 0,
  "reason": "SAST tool is not run on all commits -- score normalized to 0",
  "name": "SAST",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#sast",
  "short": "Determines if the project uses static code analysis."
  }
  },
  {
  "details": [
  "Warn: no security policy file detected",
  "Warn: no security file to analyze",
  "Warn: no security file to analyze",
  "Warn: no security file to analyze"
  ],
  "score": 0,
  "reason": "security policy file not detected",
  "name": "Security-Policy",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#security-policy",
  "short": "Determines if the project has published a security policy."
  }
  },
  {
  "details": null,
  "score": -1,
  "reason": "no releases found",
  "name": "Signed-Releases",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#signed-releases",
  "short": "Determines if the project cryptographically signs release artifacts."
  }
  },
  {
  "details": [
  "Warn: no topLevel permission defined: .github/workflows/ci.yml:1",
  "Warn: no topLevel permission defined: .github/workflows/redis-compatibility.yml:1",
  "Info: no jobLevel write permissions found"
  ],
  "score": 0,
  "reason": "detected GitHub workflow tokens with excessive permissions",
  "name": "Token-Permissions",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions",
  "short": "Determines if the project's workflows follow the principle of least privilege."
  }
  },
  {
  "details": null,
  "score": 10,
  "reason": "0 existing vulnerabilities detected",
  "name": "Vulnerabilities",
  "documentation": {
  "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#vulnerabilities",
  "short": "Determines if the project has open, known unfixed vulnerabilities."
  }
  }
  ],
  "metadata": null
  }

Check that all nodes return the same content of CLUSTER SLOTS.

The replicas for each node are in arbitrary order, so that list needs to be sorted before comparing the result. (Or possibly ignore replicas.)

Add options for username and password.

We can send them in the HELLO command.

Pipelines (a list of commands executed as a batch) are assumed to apply to keys in a single slot and is currently handled like this:

1. The commands are sent to the node responsible for the slot

2. The client waits for all responses (the same number of responses, with some exceptions for pubsub commands)

3. The client scans the replies for -MOVED, -ASK, -TRYAGAIN and -CLUSTERDOWN messages. If none of this is present, the replies are returned to the caller. Otherwise, the first occurrence of any of these errors determines the action.

   Reply	Action
   -MOVED	The whole pipeline is resent to the new node responsible for the slot
   -ASK	Only the commands that returned -ASK are resent to the new node, with each command prepended by ASKING
   -TRYAGAIN	The whole pipeline is sent again, after a delay
   -CLUSTERDOWN	The replies are returned to the caller (and a slotmap update is triggered)

The motivation for the handling of MOVED is that if one slot is moved, all keys in this slot are moved, so it makes sense to resend all commands to this new node.

The motivation for the handling of ASK is that during a slot migration, not all keys in the same slot are moved, so if keys in the same slot are accessed (using tags, keys like {a}x and {a}y), a subset of them may have been moved.

Problems:

• Resending commands that haven't failed may result in the wrong result. Commands that are not idempotent (like INCR) give the wrong result if executed twice.
• Resending only a subset the commands of the commands (the ones that failed with -ASK) may give an unexpected result. This is especially important with transactions.

Note that if an error occurs in a transaction (like -ASK or -MOVED) the EXEC command fails with an error. Thus, it is safe to resend the whole transaction.

It can be argued that with automatic redirects, the user cannot expect that the commands in a pipeline are executed in the right order and on the same node. If atomicity is required, a transaction must be used. And we must handle transactions correctly as an atomic unit.

In general, processes should belong to a supervision tree. At least most of them.

This is considered good OTP style. (Hypothetically, if trap exit is later added to a process started using spawn_link, the process is not killed by the link. If it's instead managed by a supervisor, it is brutally killed even if it's trapping exits. One should not re-invent supervisors.)

If all nodes change their IP addresses, the init node could be a hostname that resolves to the IP updated addresses. If we keep the init node address, we can use it as a fallback if we've lost connection to all nodes.

When gen_tcp:recv (or ssl:recv) is used, the process can't respond to supervisor events. We should use {active, once} or {active, N} instead.

We use one process to send data to a socket and another to receive the responses. Currently the sending process is the controlling process. We should make the receive process the controlling process instead, because gen_tcp:send can block.

It is useful for developers if the same client can connect to a standalone node, not only a cluster. For this, we need to handle an error response form CLUSTER SLOTS and handle this as if the whole cluster is handled by a single node.

The requirement that the cluster needs to have at least two masters for 'cluster_ok' may need to be reconsidered.