integration: add test for early renewal from ARI#10311
Conversation
This gets us ARI-override support.
ohemorange
left a comment
There was a problem hiding this comment.
As mentioned in the comment, I recommend moving the pebble management code into utils, something like set_immediate_ari_period. But otherwise the api makes sense to me.
| # Tell Pebble to make ARI look urgent | ||
| with open(join(context.config_dir, 'live/{0}/cert.pem').format(certname), "r") as c: | ||
| certificate_pem = c.read() | ||
|
|
||
| ari_response = json.dumps({ | ||
| "suggestedWindow": { | ||
| "start": "2020-01-01T00:00:00Z", | ||
| "end": "2020-01-01T00:00:00Z" | ||
| } | ||
| }) | ||
|
|
||
| set_renewal_info_body = json.dumps( | ||
| { | ||
| "certificate": certificate_pem, | ||
| "ariResponse": ari_response | ||
| }) | ||
|
|
||
| # POST to Pebble | ||
| misc.suppress_x509_verification_warnings() | ||
| url = PEBBLE_MANAGEMENT_URL + '/set-renewal-info/' | ||
| print(f"sending to {url}: {set_renewal_info_body}") | ||
| resp = requests.post(url, verify=False, timeout=10, data=set_renewal_info_body) | ||
| if resp.status_code != 200: | ||
| print(f"setting renewal info: {resp.status_code} {resp.text}") | ||
|
|
||
| assert resp.status_code == 200 |
There was a problem hiding this comment.
I would suggest moving this all (# Tell Pebble to make ARI look urgent...assert resp.status_code == 200) into certbot-ci/src/certbot_integration_tests/utils/misc.py; then suppress_x509_verification_warnings can stay in internal, but more importantly it can be used in other cases, and makes it clear what this test is actually testing
ohemorange
left a comment
There was a problem hiding this comment.
I have a bunch of style nits. Other than that, does pebble now give a normal ari response by default? If not, we should probably add a test with a future ari period. If it does, I'm having trouble imagining a case where we'd want anything other than "already passed," in which case the api could be even simpler for the caller. Unless there's something you had in mind? Maybe for the other upcoming test?
Yeah, Pebble already returned a default renewal period of 2/3, even before my changes in v2.8.0. Two minor reasons I wanted Pebble's hooks to allow setting an arbitrary response:
I don't have particular plans to add integration tests either of these cases to Certbot but could if you feel they are important! |
|
My hesitation comes from the idea that these tests are generally nice and modular (unlike many of our other tests...) and so you don't really have to know what it's doing under the hood to test a specific feature. But I guess you wouldn't really want an ARI response set unless you're testing ARI in particular; otherwise you'd just use As for the specific examples, tbh I'd probably just throw that sort of thing in unit tests if anything. I don't know that we need either but if you're excited about them I wouldn't stop you. |
This depends on a pending Pebble pull request and so will fail integration tests until/unless that lands: letsencrypt/pebble#501
However, I'd appreciate some eyes on this PR in this regard: is the interface we're using in Pebble useful and appropriate? If not, we can adjust the Pebble PR.
Inspired based on conversation on #10307, but note that this just tests the general case; it does not test the "default server differs from lineage server" case yet; when I try adding that I get some bugs that may reflect a problem in #10307 I need to fix (or may reflect that I need to inhibit the
--serverflag rather than trying to override it late in the command line).