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.

Sign up for GitHub

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

Require Valid Elasticache Engine Type #8539

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+67 −29
Open

Require Valid Elasticache Engine Type #8539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d1dc2b
Require Valid Elasticache Engine Type
armichaud Jan 31, 2025
4cf4176
Reformat
armichaud Jan 31, 2025
f08c559
Syntax
armichaud Jan 31, 2025
09ce681
double quotes
armichaud Jan 31, 2025
0ee28c3
fix test
armichaud Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions moto/elasticache/responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
PasswordTooShort,
)
from .models import ElastiCacheBackend, elasticache_backends
from .utils import AuthenticationTypes
from .utils import AuthenticationTypes, EngineTypes, ValidAuthModeKeys


class ElastiCacheResponse(BaseResponse):
Expand Down Expand Up @@ -40,11 +40,19 @@ def create_user(self) -> str:
if passwords:
authentication_type = AuthenticationTypes.PASSWORD.value

if engine:
engine_types = [e.value for e in EngineTypes]
if engine not in engine_types:
raise InvalidParameterValueException(
f'Unknown parameter for Engine: "{engine}", must be one of: {", ".join(engine_types)}'
)

if authentication_mode:
for key in authentication_mode.keys():
if key not in ["Type", "Passwords"]:
valid_keys = [e.value for e in ValidAuthModeKeys]
if key not in valid_keys:
raise InvalidParameterValueException(
f'Unknown parameter in AuthenticationMode: "{key}", must be one of: Type, Passwords'
f'Unknown parameter in AuthenticationMode: "{key}", must be one of: {", ".join(valid_keys)}'
)

authentication_type = authentication_mode.get("Type")
Expand Down
10 changes: 10 additions & 0 deletions moto/elasticache/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,13 @@ class AuthenticationTypes(str, Enum):
NOPASSWORD = "no-password-required"
PASSWORD = "password"
IAM = "iam"


class EngineTypes(str, Enum):
REDIS = "redis"
VALKEY = "valkey"


class ValidAuthModeKeys(str, Enum):
TYPE = "Type"
PASSWORD = "Passwords"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get that you were probably just following the existing convention, but I don't think we gain anything by using Enum here. An array constant, e.g. VALID_ENGINE_TYPES = ["redis", "valkey"], would be simpler, allowing you to compare directly without the separate step of converting the enum values into an array.

72 changes: 46 additions & 26 deletions tests/test_elasticache/test_elasticache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ def test_create_user_no_password_required():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
NoPasswordRequired=True,
)

assert resp["UserId"] == user_id
assert resp["UserName"] == "User1"
assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["MinimumEngineVersion"] == "6.0"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
Expand All @@ -43,7 +43,7 @@ def test_create_user_with_password_too_short():
client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
Passwords=["mysecretpass"],
)
Expand All @@ -52,22 +52,42 @@ def test_create_user_with_password_too_short():
assert err["Message"] == "Passwords length must be between 16-128 characters."


@mock_aws
def test_create_user_with_wrong_engine_type():
client = boto3.client("elasticache", region_name="ap-southeast-1")
user_id = "user1"
with pytest.raises(ClientError) as exc:
client.create_user(
UserId=user_id,
UserName="User1",
Engine="invalidengine",
AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)
err = exc.value.response["Error"]
assert err["Code"] == "InvalidParameterValue"
assert (
err["Message"]
== 'Unknown parameter for Engine: "invalidengine", must be one of: redis, valkey'
)


Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking that the returned error code is "InvalidParameterValue" should suffice. We don't want to assert on something that would break if another engine type is added in the future. If you want to be more thorough with your asserts while keeping your test future-proof, you can just check that "Unknown parameter for Engine" is in the message.

@mock_aws
def test_create_user_with_password():
client = boto3.client("elasticache", region_name="ap-southeast-1")
user_id = "user1"
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
Copy link
Collaborator

Choose a reason for hiding this comment

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

AWS allows for Redis or redis to be passed as the engine type, so let's leave all create_user calls as they are and just assert (as you have) that it's redis in the responses.

AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)

assert resp["UserId"] == user_id
assert resp["UserName"] == "User1"
assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["MinimumEngineVersion"] == "6.0"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
Expand All @@ -83,7 +103,7 @@ def test_create_user_without_password():
client = boto3.client("elasticache", region_name="ap-southeast-1")
with pytest.raises(ClientError) as exc:
client.create_user(
UserId="user1", UserName="User1", Engine="Redis", AccessString="?"
UserId="user1", UserName="User1", Engine="redis", AccessString="?"
)
err = exc.value.response["Error"]
assert err["Code"] == "InvalidParameterValue"
Expand All @@ -100,13 +120,13 @@ def test_create_user_with_iam():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
AuthenticationMode={"Type": "iam"},
)

assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
assert resp["Authentication"]["Type"] == "iam"
Expand All @@ -119,7 +139,7 @@ def test_create_user_invalid_authentication_type():
client.create_user(
UserId="user1",
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="?",
AuthenticationMode={"Type": "invalidtype"},
)
Expand All @@ -139,7 +159,7 @@ def test_create_user_with_iam_with_passwords():
client.create_user(
UserId="user1",
UserName="user1",
Engine="Redis",
Engine="redis",
AccessString="?",
AuthenticationMode={"Type": "iam"},
Passwords=["mysecretpassthatsverylong"],
Expand All @@ -158,7 +178,7 @@ def test_create_user_authmode_password_with_multiple_password_fields():
client.create_user(
UserId="user1",
UserName="user1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
AuthenticationMode={"Type": "password", "Passwords": ["authmodepassword"]},
Passwords=["requestpassword"],
Expand All @@ -179,7 +199,7 @@ def test_create_user_with_authmode_password_without_passwords():
client.create_user(
UserId="user1",
UserName="user1",
Engine="Redis",
Engine="redis",
AccessString="?",
AuthenticationMode={"Type": "password"},
)
Expand All @@ -199,13 +219,13 @@ def test_create_user_with_authmode_no_password():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
AuthenticationMode={"Type": "no-password-required"},
)

assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
assert resp["Authentication"]["Type"] == "no-password-required"
Expand All @@ -221,14 +241,14 @@ def test_create_user_with_no_password_required_and_authmode_nopassword():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
NoPasswordRequired=True,
AuthenticationMode={"Type": "no-password-required"},
)

assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
assert resp["Authentication"]["Type"] == "no-password"
Expand All @@ -245,7 +265,7 @@ def test_create_user_with_no_password_required_and_authmode_different():
client.create_user(
UserId="user1",
UserName="user1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
NoPasswordRequired=True,
AuthenticationMode={"Type": auth_mode},
Expand All @@ -266,7 +286,7 @@ def test_create_user_with_authmode_password():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
AuthenticationMode={
"Type": "password",
Expand All @@ -275,7 +295,7 @@ def test_create_user_with_authmode_password():
)

assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
assert resp["Authentication"]["Type"] == "password"
Expand All @@ -290,7 +310,7 @@ def test_create_user_with_authmode_password_multiple():
resp = client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
AuthenticationMode={
"Type": "password",
Expand All @@ -299,7 +319,7 @@ def test_create_user_with_authmode_password_multiple():
)

assert resp["Status"] == "active"
assert resp["Engine"] == "Redis"
assert resp["Engine"] == "redis"
assert resp["AccessString"] == "on ~* +@all"
assert resp["UserGroupIds"] == []
assert resp["Authentication"]["Type"] == "password"
Expand All @@ -313,7 +333,7 @@ def test_create_user_twice():
client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)
Expand All @@ -322,7 +342,7 @@ def test_create_user_twice():
client.create_user(
UserId=user_id,
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)
Expand All @@ -348,7 +368,7 @@ def test_delete_user():
client.create_user(
UserId="user1",
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)
Expand Down Expand Up @@ -391,7 +411,7 @@ def test_describe_users():
client.create_user(
UserId="user1",
UserName="User1",
Engine="Redis",
Engine="redis",
AccessString="on ~* +@all",
Passwords=["mysecretpassthatsverylong"],
)
Expand All @@ -403,7 +423,7 @@ def test_describe_users():
"UserId": "user1",
"UserName": "User1",
"Status": "active",
"Engine": "Redis",
"Engine": "redis",
"MinimumEngineVersion": "6.0",
"AccessString": "on ~* +@all",
"UserGroupIds": [],
Expand Down
Loading