[core] Fix security issue for retry package #48767

dentiny · 2024-11-16T11:02:23Z

I randomly find lancedb issue: lancedb/lancedb#1480
which discloses a high-severity CVE

Considering as lancedb, ray only has one use case for retry package, I took the same approach as lancedb/lancedb#1749, which names all variables better with unit and default value.

Signed-off-by: dentiny <dentinyhao@gmail.com>

release/requirements_buildkite.txt

aslonnie · 2024-11-16T19:24:16Z

python/ray/util/retry.py

+def retry(
+    max_retry_count=_RAY_DEFAULT_MAX_RETRY_COUNT,
+    init_delay_sec=_RAY_DEFAULT_INIT_DELAY_SEC,
+    max_delay_sec=_RAY_DEFAULT_MAX_DELAY_SEC,
+    backoff=_RAY_DEFAULT_BACKOFF,
+    jitter_sec=_RAT_DEFAULT_JITTER_SEC,
+):


could you add a unit test for this function?

Do you know where should I place the unit test? I don't see test folder nearby

this retry thing is only used in release at that one place, so can you just leave it in the file that uses it and do not put it in ray package?

the unit test for the cluster_manager file is here:
https://github.com/ray-project/ray/blob/master/release/BUILD.bazel#L422

this retry thing is only used in release at that one place

Yeah, that's true; but I somehow feel it could be used elsewhere in the future, considering it's so common to have something to retry.

I would create a new folder under util, so in the future all unit test on utils go there.
Let me know if you don't like it :)

Unit tested added.

it is built as a stand-alone package, and does not depend on ray (it does not require pip install ray to work

https://buildkite.com/ray-project/premerge/builds/31058#019337be-867e-4fc4-b592-824e1f929966/186-553

[2024-11-17T01:43:23Z] ModuleNotFoundError: No module named 'ray'

if we need to reuse it in the future, it is easy to make a move a make a copy when we actually want to reuse it.

I don't quite agree with your argument.
The least visibility is to make the retry function as just a function inside of the release.py file;
but the problem is, without putting it into a centralized place, people are less likely to know about it and reuse.
A common consequence is everybody makes his/her own wheels for retry.

IMO, if a function is independent of application logic, and the implementation/interface is general enough for reuse, it should be placed under a common folder for better visibility.

it is built as a stand-alone package, and does not depend on ray (it does not require pip install ray to work

I'm quite confused on the argument. Does that mean, if I want to use the same retry logic in runtime env agent and release package, I have to write two identical functions?

I have to write two identical functions?

it is recommended for this case.

A little copying is better than a little dependency.

https://www.youtube.com/watch?v=PAAkCSZUG1c&t=568s

or if there is a retry package that is secure, you can also import it I guess..

Signed-off-by: dentiny <dentinyhao@gmail.com>

aslonnie

commented. adding retry.py under ray/util is the wrong place.

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny · 2024-11-17T02:31:38Z

commented. adding retry.py under ray/util is the wrong place.

Thanks @aslonnie ! Discussed offline, release and ray core have two build systems, move the retry file to release folder.

release/ray_release/tests/test_retry.py

release/ray_release/retry.py

release/ray_release/cluster_manager/minimal.py

Signed-off-by: dentiny <dentinyhao@gmail.com>

aslonnie

nice! thanks

release/ray_release/retry.py

aslonnie · 2024-11-17T06:24:10Z

release/ray_release/retry.py

+_DEFAULT_MAX_DELAY_SEC = 30
+_DEFAULT_BACKOFF = 2
+_DEFAULT_JITTER_SEC = 1
+_DEFAULT_EXCEPTIONS = (Exception,)


maybe not set (Exception,) as default? maybe use (TimeoutError,) or (IOError,) ? or force user to provide a non-empty list? or define an RetryError in this file and use that as default?

also this is a tuple, so you cannot use this as the default arg value, you need to use None, and:

if exceptions is None: exceptions = ()

maybe not set (Exception,) as default? maybe use (TimeoutError,) or (IOError,) ? or force user to provide a non-empty list?

I think one thing we miss in our error handling is a centralized status code.
One example might be grpc status code: https://grpc.github.io/grpc/core/md_doc_statuscodes.html
which defines which codes are retriable and which are not, also used in tensorflow and abseil.

Here I'm using the Exception by default, since I'm mimic retry.retry: https://pypi.org/project/retry/, and expects a no-op change.
Let me know if you think it acceptable (at least we've specified exception to retry on the release package).

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny · 2024-11-17T07:00:07Z

Hi @aslonnie , if you think ok, could you please help merge the PR? Happy to discuss more about it if any :) Thank you!

aslonnie · 2024-11-17T08:17:26Z

release/ray_release/retry.py

+_DEFAULT_MAX_RETRY_COUNT: int = 10
+_DEFAULT_INIT_DELAY_SEC: int = 1
+_DEFAULT_MAX_DELAY_SEC: int = 30
+_DEFAULT_BACKOFF: int = 2
+_DEFAULT_JITTER_SEC: int = 1


these assignment types are implied by the way.

I randomly find lancedb issue: lancedb/lancedb#1480 which discloses a high-severity CVE Considering as lancedb, ray only has one use case for `retry` package, I took the same approach as lancedb/lancedb#1749, which names all variables better with unit and default value. --------- Signed-off-by: dentiny <dentinyhao@gmail.com> Signed-off-by: hjiang <dentinyhao@gmail.com>

dentiny requested review from pcmoritz and jjyao November 16, 2024 11:02

dentiny requested a review from a team as a code owner November 16, 2024 11:02

dentiny added the go add ONLY when ready to merge, run all tests label Nov 16, 2024

fix security issue for retry package

b956372

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny force-pushed the hjiang/fix-retry-security branch from e99dbb1 to b956372 Compare November 16, 2024 11:18

aslonnie reviewed Nov 16, 2024

View reviewed changes

dentiny added 3 commits November 17, 2024 00:24

fix dependency file

1bc8bd9

Signed-off-by: dentiny <dentinyhao@gmail.com>

Add unit test for retry

c43a0fd

Signed-off-by: dentiny <dentinyhao@gmail.com>

add test on retry failure

06c66d9

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny requested a review from aslonnie November 17, 2024 01:16

aslonnie reviewed Nov 17, 2024

View reviewed changes

dentiny force-pushed the hjiang/fix-retry-security branch from 4eb389a to df54ed0 Compare November 17, 2024 02:30

move retry file around

f336c57

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny force-pushed the hjiang/fix-retry-security branch from df54ed0 to f336c57 Compare November 17, 2024 02:31

dentiny requested review from aslonnie November 17, 2024 02:44

aslonnie reviewed Nov 17, 2024

View reviewed changes

release/ray_release/tests/test_retry.py Show resolved Hide resolved

aslonnie reviewed Nov 17, 2024

View reviewed changes

release/ray_release/retry.py Outdated Show resolved Hide resolved

release/ray_release/cluster_manager/minimal.py Outdated Show resolved Hide resolved

release/ray_release/cluster_manager/minimal.py Outdated Show resolved Hide resolved

dentiny added 6 commits November 17, 2024 03:43

Add test to build

c1d8ef6

Signed-off-by: dentiny <dentinyhao@gmail.com>

Update retry config name

655d602

Signed-off-by: dentiny <dentinyhao@gmail.com>

update import

c013610

Signed-off-by: dentiny <dentinyhao@gmail.com>

Add param to add exceptions

9a786c7

Signed-off-by: dentiny <dentinyhao@gmail.com>

Add exceptions test

24383b9

Signed-off-by: dentiny <dentinyhao@gmail.com>

only handle certain errors

0e91ab8

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny requested a review from aslonnie November 17, 2024 03:56

aslonnie approved these changes Nov 17, 2024

View reviewed changes

release/ray_release/retry.py Outdated Show resolved Hide resolved

aslonnie reviewed Nov 17, 2024

View reviewed changes

type annotation

ead3c85

Signed-off-by: dentiny <dentinyhao@gmail.com>

aslonnie reviewed Nov 17, 2024

View reviewed changes

aslonnie merged commit ae0aae4 into ray-project:master Nov 17, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fix security issue for retry package #48767

[core] Fix security issue for retry package #48767

dentiny commented Nov 16, 2024

aslonnie Nov 16, 2024

dentiny Nov 17, 2024

aslonnie Nov 17, 2024

dentiny Nov 17, 2024

dentiny Nov 17, 2024

aslonnie Nov 17, 2024

dentiny Nov 17, 2024

dentiny Nov 17, 2024

dentiny Nov 17, 2024

aslonnie Nov 17, 2024

aslonnie left a comment

dentiny commented Nov 17, 2024

aslonnie left a comment

aslonnie Nov 17, 2024

dentiny Nov 17, 2024 •

edited

Loading

dentiny commented Nov 17, 2024

aslonnie Nov 17, 2024

[core] Fix security issue for retry package #48767

[core] Fix security issue for retry package #48767

Conversation

dentiny commented Nov 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslonnie left a comment

Choose a reason for hiding this comment

dentiny commented Nov 17, 2024

aslonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dentiny Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

dentiny commented Nov 17, 2024

Choose a reason for hiding this comment

dentiny Nov 17, 2024 •

edited

Loading