The fix looks good, but would you mind adding a short integration test

Yes, this is a good idea. I'm not totally sure how to proceed though. How does this sound:

• Issue a certificate against local Pebble.
• Set up a local web server that offers a directory object with an ARI path, and handles the ARI path by always returning values meaning "renew now".
• Edit the renewal config for that cert to change the server value.
• Run certbot renew.
• Expect the certificate got renewed.

Oh, does pebble not implement ARI? If not, how come? I was definitely assuming that it does. I think the most correct thing would probably be to put it in there, but that's probably a larger ask than I realized.

Pebble does support ARI, but it doesn't have a way to twiddle ARI to say "renew now." We could add that of course.

But the main thing I was thinking with my example is that we wanted to specifically have an integration test for this bug. That is, there should be a renewal config where the server for that lineage is different than the globally set server. I think that requires having a separate ACME server. I guess we could set up a whole second Pebble but that seems slightly overkill vs starting up an HTTP server within the Python integration test. WDYT?

Ok so first question, the ARI PR changed certbot-ci.utils.certbot_call from using force-renew to not in main, what was the effect of that? To make sure we're hitting the ari code? But then presumably we were doing some testing where we did in fact want it to renew, is that not being tested now? Or is ari turned off in the test pebble instance?

As for this bug...starting two pebbles would be closer to a realistic test, which is the point of integration tests, but yeah that might be too much overhead. But maybe not? But actually, does it matter? This bug happens when you try to renew, and the lineage config has one server saved in the renewal config but another set by cli/default server if none specified. So we could issue a cert against pebble, and then call renew without the flag specifically setting pebble as the server, and that should repro without the fix.

the ARI PR changed certbot-ci.utils.certbot_call from using force-renew to not in main, what was the effect of that?

Ugh, this was meant to be a local-only change that was useful when I was running certbot_test to manually test the new functionality. I aimed to revert it before PR review, but it looks like I accidentally put it back in after a subsequent round of testing and rebasing (and I only just now noticed your comment about it).

I am also surprised it didn't break any cases. Looking at the code, the affected line is only used by main, which is what gets called if you run certbot_test from the commandline. The actual test cases call internally via certbot_test() and pass force_renew=True or =False based on what they want.

is ari turned off in the test pebble instance?

In the test Pebble instance, ARI works, but it returns a default renewal period that's in the future.

we could issue a cert against pebble, and then call renew without the flag specifically setting pebble as the server, and that should repro without the fix.

There are two symptoms of this bug:

1. We see an error logged for the failed ARI request (because it went to the wrong server, and that server doesn't know about the cert / issuer).
2. Early renewal based on ARI doesn't happen (because we're not actually querying the right server).

I think the integration test should be checking (2), and for that we need to have one server that says "please renew now," and it should be different from the default server.

There are two symptoms of this bug:

1. We see an error logged for the failed ARI request (because it went to the wrong server, and that server doesn't know about the cert / issuer).
2. Early renewal based on ARI doesn't happen (because we're not actually querying the right server).

I think the integration test should be checking (2), and for that we need to have one server that says "please renew now," and it should be different from the default server.

Ah yes, I was only thinking about checking (1). Presumably that error would always happen when we're not checking the right server though, no? If we can confirm we're querying the right server by not seeing the error, what is the additional benefit to (2)? Were you thinking of a more general test to make sure we actually renew early based on ari interval? In that case I'd still prefer to be as close as possible to a real situation; could we configure pebble to make a really short lived cert maybe? Probably adding a config to pebble for immediate ari would be cleaner though.

Ah yes, I was only thinking about checking (1). Presumably that error would always happen when we're not checking the right server though, no? If we can confirm we're querying the right server by not seeing the error, what is the additional benefit to (2)?
Were you thinking of a more general test to make sure we actually renew early based on ari interval?

Yeah, as you say I was thinking it would be useful to have the more general test. I also think it would be a more robust test. My experience with checking for the presence of log lines is that it's prone to failing spuriously (e.g. due to slight rewordings or refactorings). Checking for the absence of log lines has the opposite problem: slight rewording could result in the test always passing, even if the actual functionality regresses.

If we test for "does it actually renew early," we get a double benefit with one test case:

• integration test for ARI checking in general (to complement the unittest)
• integration test for the specific case of lineage server that differs from default server.

It's true that Pebble should probably have a hook to say "trigger ARI now." If we land that in Pebble, do you think the two-Pebbles approach would be the right one here?

That's a good point about the rewording of log lines, especially in integration tests.

It's true that Pebble should probably have a hook to say "trigger ARI now." If we land that in Pebble, do you think the two-Pebbles approach would be the right one here?

I still don't understand why we'd need two pebbles in that case. What we're testing isn't "specific server 1 (used to issue cert) vs specific server 2 being used on renew," it's "specific server 1 vs default certbot server being used on renew." A call to "certbot renew" could simply not specify a server, and then if it renews we'd be sure it's actually using the server from the renewal config file, and not trying to ping the default certbot server.

The integration test framework already sets --server <some pebble URL> for each call, so both the initial issuance (certbot --server <some pebble> --no-verify-ssl --http-01-port .... certonly -d example123.com) and the renewal (certbot --server <some pebble> --no-verify-ssl --http-01-port .... renew) would have the same server.

But I suppose for the renew command I can have a call to IntegrationTestsContext.certbot(['renew', '--server', '<nonsense URL>']). The --server will be appended after the initial --server created by test setup, so will take precedence as the global value for the --server flag. And then the lineage config would have the real, functional <some pebble> URL, and we would only see the ARI response telling us to renew early if we chose the real, functional <some pebble>. I think that makes sense and can proceed with implementing.

It should be pretty easy to modify the integration framework to simply not set the server for a particular call, no? Probably by doing something like adding an optional parameter? Or would that require too many layers of plumbing that would become messy? We have simply not had need to do it in the past but it's our own integration framework that we have developed for convenience and is entirely internal so we don't have to worry about breaking changes or anything.

I've rebased on main to pull in the new integration test from #10311.

I also updated that integration test for this specific case. Now, on renewal, it inhibits the --server flag. That way we can tell the difference between checking ARI against the lineage server (which will work, and say "renew now") vs checking it against the built-in default (which would fail).

To confirm, if a user calls certbot renew --server http://example.com/, any certs that need renewal will use http://example.com/. Should we still check ARI against the old lineage server if it's different?

By the way, worth noting that I took a shortcut by making "" a special value for omitting the directory URL; probably all the layers that take directory_url should be an Optional instead. I'll work on that now.