Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Adds retry support to methods called by the operator #58

Merged
merged 4 commits into from
Jun 21, 2017
Merged

Adds retry support to methods called by the operator #58

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5626c52
Add retry support for methods called by the operator
rossf7 Jun 21, 2017
0868c93
Add missing godoc
rossf7 Jun 21, 2017
f9316a9
Fix returning of errors
rossf7 Jun 21, 2017
df6c2c6
Merge branch 'master' into add-retry
rossf7 Jun 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion service/ca/pki.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,39 @@ package ca
import (
"fmt"
"strings"
"time"

"github.com/cenkalti/backoff"
"github.com/giantswarm/certctl/service/pki"
"github.com/giantswarm/certctl/service/token"
"github.com/giantswarm/certificatetpr"
microerror "github.com/giantswarm/microkit/error"
)

// SetupPKI creates a PKI backend and policy if one does not exists for the cluster.
const (
setupPKIMaxElapsedTime = 30 * time.Second
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The timeouts are short because the timeout for the secrets watch is only 90 seconds. The operator needs to issue all certs in that time.

)

// SetupPKIAndWait creates a PKI backend and policy if one does not exist for
// the cluster. If an error occurs an exponential backoff is used. After the
// max elapsed time the error is returned to the caller.
func (s *Service) SetupPKIAndWait(cert certificatetpr.Spec) error {
initBackoff := backoff.NewExponentialBackOff()
initBackoff.MaxElapsedTime = setupPKIMaxElapsedTime

operation := func() error {
err := s.SetupPKI(cert)
if err != nil {
s.Logger.Log("info", "failed to setup PKI - retrying")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error handling is a bit uncommon. I would prefer returning the error here masked like we usually do and do logging additionally if desired. Below should be returned nil. Looks strange that the error is always returned. In case SetupPKI does weird things we forward the weirdness here. Lets not do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that SetupPKI would have the normal logging. In the wait wrapper I just wanted to indicate that a retry was taking place.

But it is different to our normal error handling and I get the point about returning the error. I'll take another look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

return err
}

return backoff.Retry(operation, initBackoff)
}

// SetupPKI creates a PKI backend and policy if one does not exist for the cluster.
func (s *Service) SetupPKI(cert certificatetpr.Spec) error {
s.Config.Logger.Log("debug", fmt.Sprintf("setting up PKI for cluster '%s'", cert.ClusterID))

Expand Down
26 changes: 26 additions & 0 deletions service/crt/cert_signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,38 @@ package crt
import (
"strings"

"time"

"github.com/cenkalti/backoff"
"github.com/giantswarm/certctl/service/cert-signer"
"github.com/giantswarm/certctl/service/spec"
"github.com/giantswarm/certificatetpr"
microerror "github.com/giantswarm/microkit/error"
)

const (
issueMaxElapsedTime = 30 * time.Second
)

// IssueAndWait generates a certificate using the PKI backend for the cluster.
// If an error occurs an exponential backoff is used. After the max elapsed time
// the error is returned to the caller.
func (s *Service) IssueAndWait(cert certificatetpr.Spec) error {
initBackoff := backoff.NewExponentialBackOff()
initBackoff.MaxElapsedTime = issueMaxElapsedTime

operation := func() error {
err := s.Issue(cert)
if err != nil {
s.Logger.Log("info", "failed to issue cert - retrying")
}

return err
}

return backoff.Retry(operation, initBackoff)
}

// Issue generates a certificate using the PKI backend signed by the certificate
// authority associated with the configured cluster ID. The certificate is saved
// as a set of k8s secrets.
Expand Down
31 changes: 29 additions & 2 deletions service/crt/k8s_secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,19 @@ package crt

import (
"fmt"
"time"

"github.com/cenkalti/backoff"
"github.com/giantswarm/certificatetpr"
microerror "github.com/giantswarm/microkit/error"
"k8s.io/client-go/pkg/api/errors"
"k8s.io/client-go/pkg/api/v1"
)

const (
deleteSecretMaxElapsedTime = 30 * time.Second
)

// CreateCertificate saves the certificate as a k8s secret.
func (s *Service) CreateCertificate(secret certificateSecret) error {
var err error
Expand Down Expand Up @@ -39,9 +45,30 @@ func (s *Service) CreateCertificate(secret certificateSecret) error {
return nil
}

// DeleteCertificate deletes the k8s secret that stores the certificate.
// DeleteCertificateAndWait tries to delete the k8s secret. If an error occurs
// an exponential backoff is used. After the max elapsed time the error will be
// returned to the caller. The secret deletion is idempotent so no error is
// returned if the secret has already been deleted.
func (s *Service) DeleteCertificateAndWait(cert certificatetpr.Spec) error {
initBackoff := backoff.NewExponentialBackOff()
initBackoff.MaxElapsedTime = deleteSecretMaxElapsedTime

operation := func() error {
err := s.DeleteCertificate(cert)
if err != nil {
s.Logger.Log("info", "failed to delete secret - retrying")
}

return err
}

return backoff.Retry(operation, initBackoff)
}

// DeleteCertificate deletes the k8s secret that stores the certificate. The secret
// deletion is idempotent so no error is returned if the secret has already
// been deleted.
func (s *Service) DeleteCertificate(cert certificatetpr.Spec) error {
// Delete the secret which should be idempotent.
err := s.Config.K8sClient.Core().Secrets(v1.NamespaceDefault).Delete(getSecretName(cert), &v1.DeleteOptions{})
if errors.IsNotFound(err) {
return nil
Expand Down
6 changes: 3 additions & 3 deletions service/crt/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,11 @@ func (s *Service) addFunc(obj interface{}) {
cert := *obj.(*certificatetpr.CustomObject)
s.Config.Logger.Log("debug", fmt.Sprintf("creating certificate '%s'", cert.Spec.CommonName))

if err := s.Config.CAService.SetupPKI(cert.Spec); err != nil {
if err := s.Config.CAService.SetupPKIAndWait(cert.Spec); err != nil {
s.Config.Logger.Log("error", fmt.Sprintf("could not setup PKI '%#v'", err))
return
}
if err := s.Issue(cert.Spec); err != nil {
if err := s.IssueAndWait(cert.Spec); err != nil {
s.Config.Logger.Log("error", fmt.Sprintf("could not issue cert '%#v'", err))
return
}
Expand All @@ -171,7 +171,7 @@ func (s *Service) deleteFunc(obj interface{}) {
cert := *obj.(*certificatetpr.CustomObject)
s.Config.Logger.Log("debug", fmt.Sprintf("deleting certificate '%s'", cert.Spec.CommonName))

if err := s.DeleteCertificate(cert.Spec); err != nil {
if err := s.DeleteCertificateAndWait(cert.Spec); err != nil {
s.Config.Logger.Log("error", fmt.Sprintf("could not delete certificate '%#v'", err))
return
}
Expand Down