Repository navigation
Fix --webroot-path action - #10509
Conversation
In the actual CLI, the `action` for the `--domain` arg parses domain strings into san.DNSName. Make the test reflect that.
This was adding to webroot_map with a key of type `san.DNSName`, but the other places that add to webroot_map use a key of type `str`. We should use `str` here too.
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
since this PR exists and no one else seems to be complaining at #10506, i vote we go forward with this instead of my reversion PR
because of that, i think this needs a changelog entry. i'd propose what i did in my PR which is to create newsfragments/10506.fixed with the contents:
Fixed a regression that caused certbot to crash if multiple --webroot-path values were set on the command line.
i don't think we need to block this PR on that though as we can always quickly add it in a follow up PR
other than that and my inline comment with praise and an idea you can implement if you really want to for some reason, this LGTM!
| self.parser = argparse.ArgumentParser() | ||
| self.parser.add_argument("-d", "--domains", | ||
| action="append", default=[]) | ||
| action=cli._DomainsAction, default=[]) |
There was a problem hiding this comment.
i love that making this a more real test is so simple and reliably catches the bug. great idea imo 👏
part of me wants to rename _DomainsAction to DomainsAction as using code "private" to the cli module is a bit hacky here, but i'm honestly not too worried about that right now. i'm just happy to have a real fix with tests here
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
This fix looks good to me, appreciate the fixed test as well. We should go ahead and get this out, but are there other places that might have had similar problems now that we know this might be a pattern in our code? Probably worth a scan.
Fixes #10506. When --webroot-path was specified multiple times, Certbot was erroring with `DNSName SAN compared to non-SAN`. That's because, in the _WebrootPathAction that builds `namespace.webroot_path`, we were passing `domain` (type `san.DNSName`) as the keys. The other code that modifies or accesses `namespace.webroot_path` expects the keys to be of type `str`. In particular `webroot.Authenticator._set_webroots` does: ```python for achall in achalls: self.conf("map").setdefault(achall.domain, webroot_path) ``` Where `achall.domain` is a `str`. Two existing unittests would have caught this: `test_multiwebroot` and `test_webroot_map_partial_without_perform`. However, they faked out the parsing of the `--domains` flag, and that faked out code was not updated in #10468. Since this bug is caused by an interaction between the types produced by the `--domains` flag and those produced by the `--webroot-path` flag, the tests failed to catch the problem. I've updated the tests and confirmed that they fail before the fix is applied.
Cherry-picks #10509 which fixes #10506. This will eventually make its way into the 5.2.2 point release Co-authored-by: Jacob Hoffman-Andrews <[email protected]>
…10514) as i mentioned at #10509 (comment), i didn't love how the tests were using `_DomainsAction` when i think the leading underscore suggests the class is internal to the cli/cl_utils module this PR fixes that also, i don't think this PR requires two reviews
Fixes #10506.
When --webroot-path was specified multiple times, Certbot was erroring with
DNSName SAN compared to non-SAN. That's because, in the _WebrootPathAction that buildsnamespace.webroot_path, we were passingdomain(typesan.DNSName) as the keys. The other code that modifies or accessesnamespace.webroot_pathexpects the keys to be of typestr. In particularwebroot.Authenticator._set_webrootsdoes:Where
achall.domainis astr.Two existing unittests would have caught this:
test_multiwebrootandtest_webroot_map_partial_without_perform. However, they faked out the parsing of the--domainsflag, and that faked out code was not updated in #10468. Since this bug is caused by an interaction between the types produced by the--domainsflag and those produced by the--webroot-pathflag, the tests failed to catch the problem. I've updated the tests and confirmed that they fail before the fix is applied.