fix: Fix integration test failures

[holmanb]

Proposed Commit Message

Additional Context

tests fixed:

https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests/test_kernel_command_line_match/test_lxd_datasource_kernel_override_nocloud_net_ds_nocloud_net_s_https___raw_githubusercontent_com_canonical_cloud_init_main_tests_data_kernel_cmdline_match__/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests.datasources.test_nocloud/TestFTP/test_nocloud_ftp_unencrypted_server_succeeds/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests.datasources.test_nocloud/TestFTP/test_nocloud_ftps_encrypted_server_succeeds/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests.datasources.test_nocloud/TestFTP/test_nocloud_ftp_encrypted_server_fails/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests/test_kernel_command_line_match/test_lxd_disable_cloud_init_cmdline/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests/test_kernel_command_line_match/test_lxd_disable_cloud_init_file/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests/test_kernel_command_line_match/test_lxd_disable_cloud_init_env/
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-noble-lxd_vm/lastCompletedBuild/testReport/tests.integration_tests.datasources.test_nocloud/TestFTP/test_nocloud_ftps_unencrypted_server_fails/

Test Steps

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[a-dubs]

thanks for cleaning up these broken tests @holmanb !

[blackboxsw]

Thanks @holmanb. Couple of thoughts below for your review. Generally what needs fixing is: extracting the return response out of finally: and differentiating a File not found error from ftplib vs scheme errors.

--- a/cloudinit/url_helper.py
+++ b/cloudinit/url_helper.py
@@ -146,14 +146,20 @@ def read_ftps(url: str, timeout: float = 5.0, **kwargs: dict) -> "FtpResponse":
                 ftp_tls.retrbinary(
                     f"RETR {url_parts.path}", callback=buffer.write
                 )
-
                 response = FtpResponse(buffer.getvalue(), url)
-                LOG.debug("Closing connection")
             except ftplib.error_perm as e:
+                if "No such file" in str(e):
+                    raise UrlError(
+                        cause=f"File {url_parts.path} not found",
+                        code=NOT_FOUND,
+                        headers=None,
+                        url=url_parts.path,
+                    ) from e
                 LOG.warning(
                     "Attempted to connect to an insecure ftp server but used "
                     "a scheme of ftps://, which is not allowed. Use ftp:// "
-                    "to allow connecting to insecure ftp servers."
+                    "to allow connecting to insecure ftp servers. %s",
+                    e,
                 )
                 raise UrlError(
                     cause=(
@@ -166,8 +172,8 @@ def read_ftps(url: str, timeout: float = 5.0, **kwargs: dict) -> "FtpResponse":
                     url=url,
                 ) from e
             finally:
+                LOG.debug("Closing connection")
                 ftp_tls.close()
-                return response
         else:
             try:
                 ftp = ftplib.FTP()
@@ -188,6 +194,13 @@ def read_ftps(url: str, timeout: float = 5.0, **kwargs: dict) -> "FtpResponse":
                 ftp.retrbinary(f"RETR {url_parts.path}", callback=buffer.write)
                 response = FtpResponse(buffer.getvalue(), url)
             except ftplib.all_errors as e:
+                if "No such file" in str(e):
+                    raise UrlError(
+                        cause=f"File {url_parts.path} not found",
+                        code=NOT_FOUND,
+                        headers=None,
+                        url=url_parts.path,
+                    ) from e
                 code = ftp_get_return_code_from_exception(e)
                 raise UrlError(
                     cause=(
@@ -201,7 +214,7 @@ def read_ftps(url: str, timeout: float = 5.0, **kwargs: dict) -> "FtpResponse":
             finally:
                 LOG.debug("Closing connection")
                 ftp.close()
-            return response
+        return response
 
 
 def _read_file(path: str, **kwargs) -> "FileResponse":

[blackboxsw]

Let's just move this log into finally where we are actually closing the connection.

[blackboxsw]

response isn't set during error conditions/filenotfound etc. So, let's pull this out of the finally clause.
While we are at it, let's consolidate the two if/else clauses and only return response if we successfully make it to the end of our try/except handling of either case statement.

[blackboxsw]

The issue we have here in error handling is that "File not found" errors result in ftplib.error_perm permanent errors that we are casting as URLError(cause="Attempted to connect to an insecure ftp server but used a scheme of ftps://" We actually need to look either at error code '550' perhaps or "No such file or directory" in str(e) and raise the appropriate code=NOT_FOUND instead of warnings about invalid ftp: scheme.

Consolidated diff suggestion for your thoughts below.

[holmanb]

Nice catch, thanks! This doesn't get raised to users from the testing code path that I'm fixing, but yes this is more correct for other cases.

I don't like using exception strings as a differentiator. What if we just use another try/except block to differentiate the exception message.

[holmanb]

Thanks for the review @blackboxsw! I just pushed a fixup commit - let me know what you think.

[blackboxsw]

LGTM! ship it.

[blackboxsw]

WFM, looks good and well contained. No need to create a local response and carry it to the end of the function.

[holmanb]

Thanks for the reviews @a-dubs and @blackboxsw!