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

Implement cert delete functionality #1817

Merged
merged 2 commits into from
May 4, 2015

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Apr 20, 2015

Cert deletes were marked as TODO, this makes them TODONE

In order to make the cert manager testable before implementing the delete I refactored it so that you can inject how it writes (and the other config items that were previously hard coded). Commits are:

  1. the refactor of the cert manager
  2. the unit tests for the cert manager with a fake cert writer - tests both adds and deletes as well as valid configurations
  3. implement the real delete and call the clean up code from deleting service units (services) and individual routes

@pmorie @abhgupta @smarterclayton @rajatchopra PTAL

@pweil-
Copy link
Contributor Author

pweil- commented Apr 20, 2015

[test]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1990/)

// certManagerConfig provides the configuration necessary for certmanager to manipulate certificates.
type certManagerConfig struct {
// certificateKeyFunc is used to find the edge certificate (which also has the key) from the cert map of the ServiceAliasConfig
certificateKeyFunc certificateKeyFunc
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call it a "cert" in these names

Copy link
Contributor

Choose a reason for hiding this comment

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

Field names

@smarterclayton
Copy link
Contributor

Status of this?

@pweil-
Copy link
Contributor Author

pweil- commented Apr 27, 2015

in progress, just has been neglected in favor of security contexts and default certs. Will finish this week.

@pweil- pweil- force-pushed the certmanager-test branch 2 times, most recently from 69931e9 to aedfd42 Compare May 1, 2015 15:18
@pweil-
Copy link
Contributor Author

pweil- commented May 1, 2015

Ok, I've added a status to ServiceAliasConfig that is updated after a successful cert write. The certmanager will skip configurations that have the status set to the new value. When a route is added it is always created fresh and replaced in the map so the status will be removed and force the rewrite. I will add this to the QE test cases for this card. At V(4) the certmanager will notify when cert writes are skipped.

I'll make the follow up issue for the periodic sync.

PTAL

@pweil-
Copy link
Contributor Author

pweil- commented May 1, 2015

[test]

@pweil-
Copy link
Contributor Author

pweil- commented May 1, 2015

#2029


if caOk {
buffer.Write(newLine)
buffer.Write([]byte(caCertObj.Contents))
}

cm.writeCertificate(certDir, config.Host, buffer.Bytes())
err := cm.w.WriteCertificate(cm.cfg.certDir, certObj.ID, buffer.Bytes())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should always be if err := ...; err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing (the one below too)

@pweil-
Copy link
Contributor Author

pweil- commented May 1, 2015

re[test]

@smarterclayton
Copy link
Contributor

LGTM [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1990/) (Image: devenv-fedora_1431)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to b3b2413

openshift-bot pushed a commit that referenced this pull request May 4, 2015
@openshift-bot openshift-bot merged commit cca8921 into openshift:master May 4, 2015
@pweil- pweil- mentioned this pull request Jun 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants