NAS-131665 / 25.04 / Convert NFS config to new api. #15336

mgrimesix · 2025-01-08T15:08:44Z

Convert NFS config to new api.
Added a common 'TcpPort' type.

Passes NFS CI tests

Ignore the spurrious failure in test_008_check_root_dataset_settings.

bugclerk · 2025-01-08T15:09:09Z

Jira URL: https://ixsystems.atlassian.net/browse/NAS-131665

src/middlewared/middlewared/api/base/types/filesystem.py

Added a common 'TcpPort' type. Fix tcp port exclude option. Convert bindip_choices to new api. Convert NFS share CRUD to new api. Adds a new pydantic type: Dir Remove blocking calls in schema. Fixup remaining bits.

creatorcary · 2025-01-13T19:04:16Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    @classmethod
+    def check_bind_ip(cls, field_value: list):
+        """ Custom validator for IP addresses to avoid JSON serialization errors """
+        all(isinstance(v, IPvAnyAddress) for v in field_value)


This will not do anything except evaluate to False. Instead, we should define bindip: list[IPvAnyAddress] = [] (no field validator needed).

bindip: list[IPvAnyAddress] = [] introduces a different problem. A bug/feature in pydantic.
See the inline comment I added.

What's the bug?

>>> class A(BaseModel): ... attr: list[IPvAnyAddress] = [] ... >>> a=A() >>> a.model_dump() {'attr': []} >>> a=A(attr=['127.0.0.1', '192.68.0.1']) >>> a.model_dump() {'attr': [IPv4Address('127.0.0.1'), IPv4Address('192.68.0.1')]}

It was a JSON serialization error.

Hmm... that's a deeper issue then. We should be json serializing in our api, model_dump_json() converts IPvAnyAddress to string, for instance.

creatorcary · 2025-01-13T19:05:19Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    """ Specify a DNS domain (NFSv4 only) """
+    bindip: list[NonEmptyString] = Field(default_factory=[])  # Using IPvAnyAddress creates JSON serialization errors
+    """ Limit the server IP addresses available for NFS """
+    mountd_port: Annotated[TcpPort | None, AfterValidator(exclude_tcp_ports(EXCLUDED_PORTS))]


We can consider defining Annotated[TcpPort | ... ] as a type alias since it is reused.

creatorcary · 2025-01-13T19:07:35Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    """ Force Kerberos authentication on NFS shares. """
+    v4_domain: str
+    """ Specify a DNS domain (NFSv4 only) """
+    bindip: list[NonEmptyString] = Field(default_factory=[])  # Using IPvAnyAddress creates JSON serialization errors


Either default=[] or default_factory=list. In fact the prefered way is just to define bindip: list[NonEmptyString] = [] without needing Field.

creatorcary · 2025-01-13T19:20:18Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+
+@single_argument_result
+class NfsBindipChoicesResult(BaseModel):
+    """ Return a dictionary of IP addresses """


We can remove decorator and simply do result: dict instead of using ConfigDict, or result: dict[str, str] to be a little more precise.

creatorcary · 2025-01-13T19:22:24Z

src/middlewared/middlewared/plugins/nfs.py

@@ -222,8 +210,7 @@ async def nfs_compress(self, nfs):

        return nfs

-    @accepts()
-    @returns(Dict(additional_attrs=True))
+    @api_method(NfsBindipChoicesArgs, NfsBindipChoicesResult)


Unrelated to new api, but why are we not just returning a list?

It is curious, but this method has had this format for ages (i.e. before my time). The callers expect a dictionary and changing that is outside of this PR.

FWIW, I think this pattern can be useful when the value is different from what we want to display to the user. (That's the case with iscsi.portal.listen_ip_choices whose dict values will be different depending on whether ALUA is on or off, but the dict keys will be the same)

bmeagherix · 2025-01-13T19:01:23Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+            if not_unique:
+                raise ValueError(f"Entries must be unique, the following are not: {not_unique}")


You go to the trouble of having a not_unique list, and then error out on the first entry discovered. Should the if block be de-indented?

bmeagherix · 2025-01-13T19:01:52Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    """ Specify the security schema """
+    enabled: bool = True
+    """ Enable or disable the share """
+    locked: bool | None


Doc string?

bmeagherix · 2025-01-13T19:02:38Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    aliases: list[NonEmptyString] = []
+    """ IGNORED for now """
+    comment: str = ""
+    """ User comment associated with share """


Most docstrings (for NfsShareEntry) appear to be terminated by a period. Maybe be consistent. Meh.

bmeagherix · 2025-01-13T19:13:38Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    @field_validator('bindip')
+    @classmethod
+    def check_bind_ip(cls, field_value: list):
+        """ Custom validator for IP addresses to avoid JSON serialization errors """
+        all(isinstance(v, IPvAnyAddress) for v in field_value)
+        return field_value


This doesn't seem right for a few reasons:

result of all is not being tested

Don't we need to instantiate IPvAnyAddress(v) to see if it'll throw an error?

Maybe something like:

def check_bind_ip(cls, field_value: list): """ Custom validator for IP addresses to avoid JSON serialization errors """ for v in field_value: IPvAnyAddress(v) return field_value

bmeagherix · 2025-01-13T19:15:53Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+    @classmethod
+    def check_valid_network(cls, field_value: list):
+        """ Custom validator for IP networks """
+        all(isinstance(v, IPvAnyNetwork) for v in field_value)


Similar comment as for check_bind_ip.

bmeagherix · 2025-01-13T19:17:37Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+            if not_valid:
+                raise ValueError(f"Cannot contain spaces or quotes: {not_valid}")


Similar comment as for not_unique above.

bmeagherix · 2025-01-13T19:22:00Z

tests/api2/test_300_nfs.py

+            pp(["192.168.0.0/64"], False, "does not appear to be an IPv4 or IPv6", id="IPv4 - invalid range"),
+            pp(["bogus_network"], False, "does not appear to be an IPv4 or IPv6", id="IPv4 - invalid format"),


If the pydantic code starts actually testing then I wouldn't be surprised if this text changed, e.g. "value is not a valid IPv4 or IPv6 address". (Anyway, we'll see.)

creatorcary · 2025-01-13T19:53:23Z

src/middlewared/middlewared/api/v25_04_0/nfs.py

+
+    @field_validator('hosts')
+    @classmethod
+    def no_spaces_or_quotes(cls, field_value: list):


Instead of a field validator, we have a couple ways to use regex for validation using Field(pattern=...) or our own match_validator in api/base/validators.py.

Two things:
I don't think pydantic will apply a field pattern test to a list. It appears match_validator is the same. I need to check each item in the list.
I need to do not match logic here. Pattern would work if it could operate on lists. match_validator cannot do not validations.

mgrimesix added the WIP label Jan 8, 2025

bugclerk changed the title ~~Convert NFS config to new api.~~ NAS-131665 / 25.04 / Convert NFS config to new api. Jan 8, 2025

mgrimesix force-pushed the NAS-131665 branch from 2bd5447 to 44c4ff3 Compare January 10, 2025 00:20

anodos325 reviewed Jan 10, 2025

View reviewed changes

src/middlewared/middlewared/api/base/types/filesystem.py Outdated Show resolved Hide resolved

Convert NFS config to new api.

44a2d39

Added a common 'TcpPort' type. Fix tcp port exclude option. Convert bindip_choices to new api. Convert NFS share CRUD to new api. Adds a new pydantic type: Dir Remove blocking calls in schema. Fixup remaining bits.

mgrimesix force-pushed the NAS-131665 branch from b07780d to 44a2d39 Compare January 13, 2025 15:42

mgrimesix removed the WIP label Jan 13, 2025

mgrimesix requested review from a team and anodos325 January 13, 2025 16:14

anodos325 approved these changes Jan 13, 2025

View reviewed changes

Add None as option for locked.

ee5db4e

creatorcary requested changes Jan 13, 2025

View reviewed changes

bmeagherix requested changes Jan 13, 2025

View reviewed changes

creatorcary reviewed Jan 13, 2025

View reviewed changes

Address comments

83ed319

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAS-131665 / 25.04 / Convert NFS config to new api. #15336

NAS-131665 / 25.04 / Convert NFS config to new api. #15336

mgrimesix commented Jan 8, 2025 •

edited

Loading

bugclerk commented Jan 8, 2025

creatorcary Jan 13, 2025

mgrimesix Jan 13, 2025 •

edited

Loading

creatorcary Jan 13, 2025

mgrimesix Jan 13, 2025

creatorcary Jan 13, 2025

creatorcary Jan 13, 2025

creatorcary Jan 13, 2025

creatorcary Jan 13, 2025

creatorcary Jan 13, 2025 •

edited

Loading

mgrimesix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

bmeagherix Jan 13, 2025

creatorcary Jan 13, 2025 •

edited

Loading

mgrimesix Jan 13, 2025 •

edited

Loading

		if not_unique:
		raise ValueError(f"Entries must be unique, the following are not: {not_unique}")

		if not_valid:
		raise ValueError(f"Cannot contain spaces or quotes: {not_valid}")

		pp(["192.168.0.0/64"], False, "does not appear to be an IPv4 or IPv6", id="IPv4 - invalid range"),
		pp(["bogus_network"], False, "does not appear to be an IPv4 or IPv6", id="IPv4 - invalid format"),

NAS-131665 / 25.04 / Convert NFS config to new api. #15336

Are you sure you want to change the base?

NAS-131665 / 25.04 / Convert NFS config to new api. #15336

Conversation

mgrimesix commented Jan 8, 2025 • edited Loading

bugclerk commented Jan 8, 2025

Choose a reason for hiding this comment

mgrimesix Jan 13, 2025 • edited Loading

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

creatorcary Jan 13, 2025 • edited Loading

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

creatorcary Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

mgrimesix Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

mgrimesix commented Jan 8, 2025 •

edited

Loading

mgrimesix Jan 13, 2025 •

edited

Loading

creatorcary Jan 13, 2025 •

edited

Loading

creatorcary Jan 13, 2025 •

edited

Loading

mgrimesix Jan 13, 2025 •

edited

Loading