Add identifier field to AnnotatedChallenge subclasses - #10491

Thanks for the feedback! Working on it.

to deal with our code tests erroring because of these DeprecationWarnings, you could just ignore the warning in pytest.ini, but i have a slight preference that you fixup our code to ignore it inline like i did for another warnings at #10466.

Why set up ignores in the tests rather than just converting the tests (and/or code under test) to not pass domain to __init__()?

to deal with our code tests erroring because of these DeprecationWarnings, you could just ignore the warning in pytest.ini, but i have a slight preference that you fixup our code to ignore it inline like i did for another warnings at #10466.

Why set up ignores in the tests rather than just converting the tests (and/or code under test) to not pass domain to __init__()?

ah my comment wasn't clear

i also realize now that my link was bad as i misremembered the approach to warnings used in that PR. what i meant to reference was something like: https://github.com/certbot/certbot/pull/10294/files#diff-aa74ccedcdb0e9f365af3d08968229c2edc189ab0eaccbc77f4160b55baece84L172-R179

sorry about that

i definitely think converting test and prod code to use the new approach is better than ignores tho! i just think in some cases, doing that will be difficult

for example, since we sometimes store these annotated challenge objects in dicts and sets, ImmutableMap.__hash__ gets called. since this function accesses all slots which for now will continue to include domain, i believe this function would trigger deprecation warnings

i'm open to other ideas, but the way i was imagining you'd deal with this would be to define your own hash function on annotated challenge objects that looks something like:

def __hash__(self) -> int:
    with warnings.catch_warnings():
        warnings.filterwarnings('ignore', 'domain is deprecated')
        return super().__hash__()

this code could then be deleted when the domain attribute is removed from slots entirely

hopefully that makes more sense

to deal with our code tests erroring because of these DeprecationWarnings, you could just ignore the warning in pytest.ini, but i have a slight preference that you fixup our code to ignore it inline like i did for another warnings at #10466.

FYI pytest.ini has this comment that states it as more than a preference :D :

# Warnings being triggered by our plugins using deprecated features in
# acme/certbot should be fixed by having our plugins no longer using the
# deprecated code rather than adding them to the list of ignored warnings here.

I feel like the same logic should apply to inline warnings.catch_warnings() like you linked to:

        with warnings.catch_warnings():
            warnings.filterwarnings("ignore", "TLSServer is deprecated")

So that leaves us with "update every call site to actually do the right thing." The simplest "right thing" that is equivalent to what we do today is to just replace achall.domain with achall.identifier.value, without doing anything special to distinguish IP addresses vs domains. The more complicated "right thing" is to check if achall.identifier.typ is IDENTIFIER_FQDN, and error out if not (in anticipation of future work where we just do the right thing for both kinds of identifiers). One problem with that more complicated right thing is that it adds code paths, which requires coverage. And ideally those code paths go away eventually?

My overall takeaway is: fixing AnnotatedChallenge to know about non-FQDN identifiers is really the stage at which we need to decide to what extent our plugins are going to support IP address identifiers in the automagic configuration modes of our Nginx and Apache plugins. I talked about that question a bit in #10346. If we don't want to support them in the automagic modes, great, we can add exceptions for seeing IP addresses, add testing of those code paths, and that will be permanent. If we do want to support them, I'd rather not add temporarily exception-raising code paths along with the unittests of those exceptions.

i'm aware of what pytest.ini says. i think we're talking about different things when it comes to ignoring warnings. my suggestion about catch_warnings and filterwarnings was to fixup the __hash__ function on AnnotatedChallenges while __slots__ continued to contain a domain attribute

If we do want to support them, I'd rather not add temporarily exception-raising code paths along with the unittests of those exceptions.

for the sake of argument, let's say we do want to support installing IP certs with apache and nginx eventually. i don't imagine we'll add support for obtaining IP certs and installing them with both apache and nginx all in a single release of certbot. in the meantime, just replacing achall.domain with achall.identifier.value and letting the different plugins run until they blow up because they need to treat IP addresses differently doesn't seem great to me. what do you propose we do instead as a temporary measure if not check achall.identifier.typ and raise a user friendly error?

what do you propose we do instead as a temporary measure if not check achall.identifier.typ and raise a user friendly error?

We already check for IP addresses in main, and return a user-friendly error there:

(that code stays the same in #10495)

The question is whether we should additionally check locally at each reference of achall.identifier.value, or just rely on that top level check? I think I'm talking myself around to "rely on the global check", at least for this PR.

that check won't be hit for say certbot certonly --apache tho would it? maybe there's a bit of a middle ground between checking every reference and checking no references? how's this for a plan?

first, just convert every achall.domain access to achall.identifier.value

second, we can rely on the high level checks the few different places they occur in main.py around our installer API, but do you mind also adding an inline comment to each one saying something like?

# our apache and nginx plugins are currently relying on this check for a user friendly error
# message about their lack of support for IP certificates. if you're removing this check, please
# consider also updating other code to keep a sane UX here

for our authenticator APIs, including those of apache and nginx, if we expect extra work needs to be done to get them to be able to successfully obtain certs for IP addresses (or if we're just unsure whether they work right now or not), we add a simple check to the top of each of those plugins perform function that errors if an achall for an IP address is passed? with the way our authenticator plugin API works, i believe a check like that is sufficient

i'm imagining the code for a check like that looking something like:

if any(achall.identifier.typ == 'ip' for achall in achalls):
    raise ...

if getting code coverage on that raise line is difficult, i'd be fine with us not worrying about test coverage there, even if we have to add # pragma: no cover comments. getting code coverage would be ideal if it's not too tricky tho

i do not think we need a check like this in our DNS plugins as we should never get a DNS achall containing an IP identifier

maybe/hopefully an approach like this simplifies the problem a fair bit? how does it sound to you?

Yep, that sounds like a great approach. Pushed an update that does basically all of that, except adding coverage for the ConfigurationErrors in http_01.py (nginx and apache flavors). Passes tests other than the coverage tests; I'll add test cases for those two if's on Monday.

Alright, pushed with a test update; should pass now.