Skip to content

WIP: Rough fix for vanishing renewal config.#7798

Open
dmwilcox wants to merge 7 commits intocertbot:mainfrom
dmwilcox:dmw/issue-7695
Open

WIP: Rough fix for vanishing renewal config.#7798
dmwilcox wants to merge 7 commits intocertbot:mainfrom
dmwilcox:dmw/issue-7695

Conversation

@dmwilcox
Copy link
Copy Markdown
Contributor

@dmwilcox dmwilcox commented Feb 23, 2020

Addresses #7694

Findings: Issue was caused by modifying config by reference and subsequently relying on reference in caller being passed to all subsequent users. Issue did not manifest in 'renew' subcommand but was present in 'certonly' and 'run' code paths due to how object reference was passed.

Suggested permanent fix: Collapse config object references, store it for each given domain in a canonical object attribute and always use it from there.

Still needs:

  • a decision on making renewal.py:_reconstitute public for real or wrapping it in a public function
  • is RenewableCertificate a reasonable candidate for holding config object reference?
  • tests -- I have some partially working ones but will seek advice in the EFF #certbot mattermost channel

@bmw
Copy link
Copy Markdown
Member

bmw commented Feb 27, 2020

To provide an update here, I haven't forgotten about this, but I have a lot on my plate at the moment and @maximillianh was going to find someone else to take a look.

@m0namon m0namon self-assigned this Mar 4, 2020
@m0namon
Copy link
Copy Markdown
Contributor

m0namon commented Mar 19, 2020

Thanks a bunch @dmwilcox ! Sorry for the delay here. I'm taking a closer look later today, but here are some initial thoughts.

It seems that the main issue is that _reconstitute is only called in the renew code path, and not in the other code paths-- does that match what you found in your your investigation?

a decision on making renewal.py:_reconstitute public for real or wrapping it in a public function

I think that would make sense. It also seems like this function (and its helpers) might belong better in storage.py.

tests -- I have some partially working ones but will seek advice in the EFF #certbot mattermost channel

Feel free to ping us in the mattermost or here about testing! All the failures at the moment are in cert_manager_test.py-- you should be able to run these tests locally with pytest after setting up your virtual environment.

@bmw bmw added priority: unplanned Work that we believe should be done, but does not have a higher priority. priority: significant Issues with higher than average priority that do not need to be in the current milestone. and removed priority: unplanned Work that we believe should be done, but does not have a higher priority. labels Mar 25, 2020
@bmw
Copy link
Copy Markdown
Member

bmw commented Aug 20, 2020

Just to flag this to help with the review process here, it looks like there are some merge conflicts preventing CI from being run on this PR.

@dmwilcox
Copy link
Copy Markdown
Contributor Author

dmwilcox commented Aug 21, 2020

All direct calls to create a RenewableCert object (except one) go through storage.get_renewable_cert(). This ensures renewal options are restored to the internal config object if available. This was done by moving parts of renewal._reconstitute() into storage.get_renewable_cert() so those operations could be shared between the 'renew', 'run' and 'certonly' sub-commands.

@adferrand adferrand mentioned this pull request Sep 1, 2020
Closed
2 tasks
atombrella
atombrella reviewed Sep 2, 2020
View reviewed changes
certbot/certbot/_internal/configuration.py
setattr(self.namespace, name, value)

def __str__(self):
s = list()
Copy link
Copy Markdown
Contributor

@atombrella atombrella Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly a choice of style and speed, s = [] is faster. Generally, literals are better for speed and probably also more pythonic.

@adferrand adferrand mentioned this pull request Oct 1, 2020
Merged
@bmw bmw changed the base branch from master to main February 10, 2025 18:13
@bmw bmw unassigned m0namon Feb 11, 2025
@ohemorange
Copy link
Copy Markdown
Contributor

ohemorange commented Mar 20, 2026

Given that #8343 was merged as an alternate fix, is there any reason to keep this open?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@atombrella atombrella atombrella left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

priority: significant Issues with higher than average priority that do not need to be in the current milestone.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dmwilcox @bmw @m0namon @ohemorange @atombrella