Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify KRA transport cert cache #616

Closed
wants to merge 1 commit into from
Closed

Simplify KRA transport cert cache #616

wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 17, 2017 •
edited

In-memory cache causes problem in forking servers. A file based cache is
good enough. It's easier to understand and avoids performance regression
and synchronization issues when cert becomes out-of-date.

https://pagure.io/freeipa/issue/6787

Signed-off-by: Christian Heimes cheimes@redhat.com

@tiran
Copy link
Member Author

tiran commented Mar 17, 2017

Needs to be merged into ipa-4.5 branch, too.

@HonzaCholasta
Copy link
Contributor

NACK on the completely unnecessary changes in _TransportCertCache interface, variable names and formatting. Otherwise LGTM.

@MartinBasti
Copy link
Contributor

MartinBasti commented Mar 17, 2017 •
edited

Please open a ticket and put it into commit messsage

HonzaCholasta
HonzaCholasta reviewed Mar 17, 2017
View reviewed changes
ipaclient/plugins/vault.py Outdated
# cache it again
_transport_cert_cache.store_cert(
self.api.env.domain, transport_cert
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling vaultconfig_show already writes the cert to the file. There is no reason to do it again here.

@tiran
Copy link
Member Author

tiran commented Mar 17, 2017

@HonzaCholasta I don't agree with you. Mutable mapping is too complex for a simple cache. My approach is KISS.

@tiran
Simplify KRA transport cert cache
3aa2cf9 
In-memory cache causes problem in forking servers. A file based cache is
good enough. It's easier to understand and avoids performance regression
and synchronization issues when cert becomes out-of-date.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@HonzaCholasta
Copy link
Contributor

HonzaCholasta commented Mar 20, 2017 •
edited

@tiran,

  • patches targeted at backport branches should be as small as possible,
  • it's hard to see what is an actual change and what is just a formatting / naming change,
  • the formatting changes do not add any value, the code is already PEP8 compliant,
  • using a mapping interface is KISS too (it does not have to implement the full mutable mapping interface, just get/set/del key).

I don't think this needs to be discussed further, either do the requested changes or this PR won't be merged.

HonzaCholasta
HonzaCholasta reviewed Mar 20, 2017
View reviewed changes
ipaclient/plugins/vault.py
os.rename(f.name, filename)
except Exception:
os.unlink(f.name)
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

The commit message is missing any information about this change.

@tiran
Copy link
Member Author

tiran commented Mar 20, 2017

Size of a patch is a wrong metric. It's about code complexity. My patch reduces code complexity and logic complexity. It also fixes at least two bugs: multi-process concurrency bug and logging bug that prevents the code from working correctly.

HonzaCholasta
HonzaCholasta reviewed Mar 20, 2017
View reviewed changes
ipaclient/plugins/vault.py
transport_cert = self._transport_certs[domain]
except KeyError:
transport_cert = None
def load_cert(self, domain):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to get? The _cert suffix seems superfluous, given the class is named _Transport*Cert*Cache.

HonzaCholasta
HonzaCholasta reviewed Mar 20, 2017
View reviewed changes
ipaclient/plugins/vault.py

return transport_cert
def store_cert(self, domain, transport_cert):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to put?

HonzaCholasta
HonzaCholasta reviewed Mar 20, 2017
View reviewed changes
ipaclient/plugins/vault.py

self._transport_certs[domain] = transport_cert
def remove_cert(self, domain):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to remove or delete?

@HonzaCholasta
Copy link
Contributor

@tiran, you are right about the interface change, I was seeing things that are not there, I'm sorry. Please address inline comments (mainly the one about missing info in commit message, others are mostly nitpicks) and it's an ACK.

@tiran tiran added the rejected Pull Request has been rejected label Mar 22, 2017
@tiran tiran closed this Mar 22, 2017
@HonzaCholasta
Copy link
Contributor

I guess you must have missed my last comment about the PR being almost OK - reopening.

@HonzaCholasta HonzaCholasta reopened this Mar 27, 2017
@HonzaCholasta HonzaCholasta removed the rejected Pull Request has been rejected label Mar 27, 2017
@tiran
Copy link
Member Author

tiran commented Mar 27, 2017

I did not miss #616 (comment)

@HonzaCholasta HonzaCholasta added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 28, 2017
@HonzaCholasta
Copy link
Contributor

master:

  • abefb64 Simplify KRA transport cert cache

ipa-4-5:

  • 2723b5f Simplify KRA transport cert cache

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

@HonzaCholasta HonzaCholasta HonzaCholasta left review comments

Assignees
No one assigned
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@tiran @HonzaCholasta @MartinBasti