Merge pull request #17 from jakkab/cr-status

Improve CR status handling

README.md

@@ -27,6 +27,7 @@ The project is based on [Kubebuilder](https://github.com/kubernetes-sigs/kubebui
 - kustomize
 - [ginkgo](https://onsi.github.io/ginkgo/) for local integration testing
 - access to K8s environment: minikube or a remote K8s cluster
+- [mockery](https://github.com/vektra/mockery) to generate mocks for testing purposes
 
 
 
@@ -49,3 +50,12 @@ To deploy the controller, edit the value of the ```--hydra-url``` argument in th
 |-----------------|----------|------------------------------|---------------|------------------------------------------------------|
 | **hydra-url**   | yes      | ORY Hydra's service address  | -             | ` ory-hydra-admin.ory.svc.cluster.local`             |
 | **hydra-port**  | no       | ORY Hydra's service port     | `4445`        | `4445`                                               |
+
+## Development
+
+### Testing
+
+Use mockery to generate mock types that implement existing interfaces. To generate a mock type for an interface, navigate to the directory containing that interface and run this command:
+```
+mockery -name={INTERFACE_NAME}
+```

api/v1alpha1/oauth2client_types.go

@@ -20,6 +20,13 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+type StatusCode string
+
+const (
+	StatusRegistrationFailed StatusCode = "CLIENT_REGISTRATION_FAILED"
+	StatusCreateSecretFailed StatusCode = "SECRET_CREATION_FAILED"
+)
+
 // OAuth2ClientSpec defines the desired state of OAuth2Client
 type OAuth2ClientSpec struct {
 	// +kubebuilder:validation:MaxItems=4
@@ -58,7 +65,16 @@ type OAuth2ClientStatus struct {
 	// ClientID is the id for this client.
 	ClientID *string `json:"clientID,omitempty"`
 	// ObservedGeneration represents the most recent generation observed by the daemon set controller.
-	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+	ObservedGeneration  int64               `json:"observedGeneration,omitempty"`
+	ReconciliationError ReconciliationError `json:"reconciliationError,omitempty"`
+}
+
+// ReconciliationError represents an error that occurred during the reconciliation process
+type ReconciliationError struct {
+	// Code is the status code of the reconciliation error
+	Code StatusCode `json:"statusCode,omitempty"`
+	// Description is the description of the reconciliation error
+	Description string `json:"description,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1alpha1/zz_generated.deepcopy.go

@@ -120,6 +120,7 @@ func (in *OAuth2ClientStatus) DeepCopyInto(out *OAuth2ClientStatus) {
 		*out = new(string)
 		**out = **in
 	}
+	out.ReconciliationError = in.ReconciliationError
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OAuth2ClientStatus.
@@ -131,3 +132,18 @@ func (in *OAuth2ClientStatus) DeepCopy() *OAuth2ClientStatus {
 	in.DeepCopyInto(out)
 	return out
 }
+
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *ReconciliationError) DeepCopyInto(out *ReconciliationError) {
+	*out = *in
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ReconciliationError.
+func (in *ReconciliationError) DeepCopy() *ReconciliationError {
+	if in == nil {
+		return nil
+	}
+	out := new(ReconciliationError)
+	in.DeepCopyInto(out)
+	return out
+}

config/crd/bases/hydra.ory.sh_oauth2clients.yaml

@@ -432,6 +432,16 @@ spec:
                 observed by the daemon set controller.
               format: int64
               type: integer
+            reconciliationError:
+              properties:
+                description:
+                  description: Description is the description of the reconciliation
+                    error
+                  type: string
+                statusCode:
+                  description: Code is the status code of the reconciliation error
+                  type: string
+              type: object
             secret:
               description: Secret points to the K8s secret that contains this client's
                 id and password

controllers/mocks/HydraClientInterface.go

@@ -0,0 +1,101 @@
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+
+package mocks
+
+import hydra "github.com/ory/hydra-maester/hydra"
+import mock "github.com/stretchr/testify/mock"
+
+// HydraClientInterface is an autogenerated mock type for the HydraClientInterface type
+type HydraClientInterface struct {
+	mock.Mock
+}
+
+// DeleteOAuth2Client provides a mock function with given fields: id
+func (_m *HydraClientInterface) DeleteOAuth2Client(id string) error {
+	ret := _m.Called(id)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string) error); ok {
+		r0 = rf(id)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// GetOAuth2Client provides a mock function with given fields: id
+func (_m *HydraClientInterface) GetOAuth2Client(id string) (*hydra.OAuth2ClientJSON, bool, error) {
+	ret := _m.Called(id)
+
+	var r0 *hydra.OAuth2ClientJSON
+	if rf, ok := ret.Get(0).(func(string) *hydra.OAuth2ClientJSON); ok {
+		r0 = rf(id)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*hydra.OAuth2ClientJSON)
+		}
+	}
+
+	var r1 bool
+	if rf, ok := ret.Get(1).(func(string) bool); ok {
+		r1 = rf(id)
+	} else {
+		r1 = ret.Get(1).(bool)
+	}
+
+	var r2 error
+	if rf, ok := ret.Get(2).(func(string) error); ok {
+		r2 = rf(id)
+	} else {
+		r2 = ret.Error(2)
+	}
+
+	return r0, r1, r2
+}
+
+// PostOAuth2Client provides a mock function with given fields: o
+func (_m *HydraClientInterface) PostOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error) {
+	ret := _m.Called(o)
+
+	var r0 *hydra.OAuth2ClientJSON
+	if rf, ok := ret.Get(0).(func(*hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON); ok {
+		r0 = rf(o)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*hydra.OAuth2ClientJSON)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(*hydra.OAuth2ClientJSON) error); ok {
+		r1 = rf(o)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// PutOAuth2Client provides a mock function with given fields: o
+func (_m *HydraClientInterface) PutOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error) {
+	ret := _m.Called(o)
+
+	var r0 *hydra.OAuth2ClientJSON
+	if rf, ok := ret.Get(0).(func(*hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON); ok {
+		r0 = rf(o)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*hydra.OAuth2ClientJSON)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(*hydra.OAuth2ClientJSON) error); ok {
+		r1 = rf(o)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}

controllers/oauth2client_controller.go

@@ -101,7 +101,17 @@ func (r *OAuth2ClientReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, client *hydrav1alpha1.OAuth2Client) error {
 	created, err := r.HydraClient.PostOAuth2Client(client.ToOAuth2ClientJSON())
 	if err != nil {
-		return err
+		client.Status.ObservedGeneration = client.Generation
+		client.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{
+			Code:        hydrav1alpha1.StatusRegistrationFailed,
+			Description: err.Error(),
+		}
+		if updateErr := r.Status().Update(ctx, client); updateErr != nil {
+			r.Log.Error(err, fmt.Sprintf("error registring client %s/%s ", client.Name, client.Namespace), "oauth2client", "register")
+			return updateErr
+		}
+
+		return nil
 	}
 
 	clientSecret := apiv1.Secret{
@@ -121,6 +131,10 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, clien
 	err = r.Create(ctx, &clientSecret)
 	if err != nil {
 		r.Log.Error(err, fmt.Sprintf("error creating secret for client %s/%s ", client.Name, client.Namespace), "oauth2client", "register")
+		client.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{
+			Code:        hydrav1alpha1.StatusCreateSecretFailed,
+			Description: err.Error(),
+		}
 	} else {
 		client.Status.Secret = &clientSecret.Name
 	}

controllers/oauth2client_controller_integration_test.go

@@ -5,11 +5,15 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/ory/hydra-maester/controllers/mocks"
+	"github.com/pkg/errors"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
 	"github.com/ory/hydra-maester/controllers"
 	"github.com/ory/hydra-maester/hydra"
+	. "github.com/stretchr/testify/mock"
 	apiv1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -24,20 +28,23 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 )
 
-const timeout = time.Second * 5
+const (
+	timeout      = time.Second * 5
+	tstNamespace = "default"
+	tstScopes    = "a b c"
+)
+
+var tstClientID = "testClientID"
+var tstSecret = "testSecret"
 
 var _ = Describe("OAuth2Client Controller", func() {
 	Context("in a happy-path scenario", func() {
 
-		var tstName = "test"
-		var tstNamespace = "default"
-		var tstScopes = "a b c"
-		var tstClientID = "testClientID"
-		var tstSecret = "testSecret"
-
-		var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Name: tstName, Namespace: tstNamespace}}
 		It("should call create OAuth2 client in Hydra and a Secret", func() {
 
+			tstName := "test"
+			expectedRequest := &reconcile.Request{NamespacedName: types.NamespacedName{Name: tstName, Namespace: tstNamespace}}
+
 			s := scheme.Scheme
 			err := hydrav1alpha1.AddToScheme(s)
 			Expect(err).NotTo(HaveOccurred())
@@ -48,12 +55,21 @@ var _ = Describe("OAuth2Client Controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 			c := mgr.GetClient()
 
-			mch := (&mockHydraClient{}).
-				withSecret(tstSecret).
-				withClientID(tstClientID)
+			mch := mocks.HydraClientInterface{}
+			mch.On("PostOAuth2Client", AnythingOfType("*hydra.OAuth2ClientJSON")).Return(func(o *hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON {
+				return &hydra.OAuth2ClientJSON{
+					ClientID:      &tstClientID,
+					Secret:        &tstSecret,
+					Name:          o.Name,
+					GrantTypes:    o.GrantTypes,
+					ResponseTypes: o.ResponseTypes,
+					Scope:         o.Scope,
+				}
+			}, func(o *hydra.OAuth2ClientJSON) error {
+				return nil
+			})
 
-			recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, mch))
-			//_, requests := SetupTestReconcile(getApiReconciler(mgr))
+			recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, &mch))
 
 			Expect(add(mgr, recFn)).To(Succeed())
 
@@ -66,7 +82,7 @@ var _ = Describe("OAuth2Client Controller", func() {
 				mgrStopped.Wait()
 			}()
 
-			instance := testInstance(tstName, tstNamespace, tstScopes)
+			instance := testInstance(tstName)
 			err = c.Create(context.TODO(), instance)
 			// The instance object may not be a valid object because it might be missing some required fields.
 			// Please modify the instance object by adding required fields and then remove the following if statement.
@@ -76,7 +92,7 @@ var _ = Describe("OAuth2Client Controller", func() {
 			}
 			Expect(err).NotTo(HaveOccurred())
 			defer c.Delete(context.TODO(), instance)
-			Eventually(requests, timeout).Should(Receive(Equal(expectedRequest)))
+			Eventually(requests, timeout).Should(Receive(Equal(*expectedRequest)))
 
 			//Verify the created CR instance status
 			var retrieved hydrav1alpha1.OAuth2Client
@@ -93,6 +109,142 @@ var _ = Describe("OAuth2Client Controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(createdSecret.Data["client_secret"]).To(Equal([]byte(tstSecret)))
 		})
+
+		It("should call create OAuth2 client in Hydra and update object status if the call failed", func() {
+
+			tstName := "test2"
+			expectedRequest := &reconcile.Request{NamespacedName: types.NamespacedName{Name: tstName, Namespace: tstNamespace}}
+
+			s := scheme.Scheme
+			err := hydrav1alpha1.AddToScheme(s)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Setup the Manager and Controller.  Wrap the Controller Reconcile function so it writes each request to a
+			// channel when it is finished.
+			mgr, err := manager.New(cfg, manager.Options{Scheme: s})
+			Expect(err).NotTo(HaveOccurred())
+			c := mgr.GetClient()
+
+			mch := mocks.HydraClientInterface{}
+			mch.On("PostOAuth2Client", Anything).Return(nil, errors.New("error"))
+
+			recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, &mch))
+
+			Expect(add(mgr, recFn)).To(Succeed())
+
+			//Start the manager and the controller
+			stopMgr, mgrStopped := StartTestManager(mgr)
+
+			//Ensure manager is stopped properly
+			defer func() {
+				close(stopMgr)
+				mgrStopped.Wait()
+			}()
+
+			instance := testInstance(tstName)
+			err = c.Create(context.TODO(), instance)
+			// The instance object may not be a valid object because it might be missing some required fields.
+			// Please modify the instance object by adding required fields and then remove the following if statement.
+			if apierrors.IsInvalid(err) {
+				Fail(fmt.Sprintf("failed to create object, got an invalid object error: %v", err))
+				return
+			}
+			Expect(err).NotTo(HaveOccurred())
+			defer c.Delete(context.TODO(), instance)
+			Eventually(requests, timeout).Should(Receive(Equal(*expectedRequest)))
+
+			//Verify the created CR instance status
+			var retrieved hydrav1alpha1.OAuth2Client
+			ok := client.ObjectKey{Name: tstName, Namespace: tstNamespace}
+			err = c.Get(context.TODO(), ok, &retrieved)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(retrieved.Status.ClientID).To(BeNil())
+			Expect(retrieved.Status.Secret).To(BeNil())
+			Expect(retrieved.Status.ReconciliationError).NotTo(BeNil())
+			Expect(retrieved.Status.ReconciliationError.Code).To(Equal(hydrav1alpha1.StatusRegistrationFailed))
+			Expect(retrieved.Status.ReconciliationError.Description).To(Equal("error"))
+
+		})
+
+		tstClientID = "testClientID2"
+
+		It("should call create OAuth2 client in Hydra, create Secret and update object status if Secret creation failed", func() {
+
+			tstName := "test3"
+			expectedRequest := &reconcile.Request{NamespacedName: types.NamespacedName{Name: tstName, Namespace: tstNamespace}}
+
+			s := scheme.Scheme
+			err := hydrav1alpha1.AddToScheme(s)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Setup the Manager and Controller.  Wrap the Controller Reconcile function so it writes each request to a
+			// channel when it is finished.
+			mgr, err := manager.New(cfg, manager.Options{Scheme: s})
+			Expect(err).NotTo(HaveOccurred())
+			c := mgr.GetClient()
+
+			mch := mocks.HydraClientInterface{}
+			mch.On("PostOAuth2Client", AnythingOfType("*hydra.OAuth2ClientJSON")).Return(func(o *hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON {
+				return &hydra.OAuth2ClientJSON{
+					ClientID:      &tstClientID,
+					Secret:        &tstSecret,
+					Name:          o.Name,
+					GrantTypes:    o.GrantTypes,
+					ResponseTypes: o.ResponseTypes,
+					Scope:         o.Scope,
+				}
+			}, func(o *hydra.OAuth2ClientJSON) error {
+				return nil
+			})
+
+			recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, &mch))
+
+			Expect(add(mgr, recFn)).To(Succeed())
+
+			//Start the manager and the controller
+			stopMgr, mgrStopped := StartTestManager(mgr)
+
+			//Ensure manager is stopped properly
+			defer func() {
+				close(stopMgr)
+				mgrStopped.Wait()
+			}()
+
+			//ensure conflicting entry exists
+			secret := apiv1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      tstName,
+					Namespace: tstNamespace,
+				},
+			}
+			err = c.Create(context.TODO(), &secret)
+			Expect(err).NotTo(HaveOccurred())
+
+			instance := testInstance(tstName)
+			err = c.Create(context.TODO(), instance)
+			// The instance object may not be a valid object because it might be missing some required fields.
+			// Please modify the instance object by adding required fields and then remove the following if statement.
+			if apierrors.IsInvalid(err) {
+				Fail(fmt.Sprintf("failed to create object, got an invalid object error: %v", err))
+				return
+			}
+			Expect(err).NotTo(HaveOccurred())
+			defer c.Delete(context.TODO(), instance)
+			Eventually(requests, timeout).Should(Receive(Equal(*expectedRequest)))
+
+			//Verify the created CR instance status
+			var retrieved hydrav1alpha1.OAuth2Client
+			ok := client.ObjectKey{Name: tstName, Namespace: tstNamespace}
+			err = c.Get(context.TODO(), ok, &retrieved)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(retrieved.Status.ClientID).NotTo(BeNil())
+			Expect(retrieved.Status.Secret).To(BeNil())
+			Expect(retrieved.Status.ReconciliationError).NotTo(BeNil())
+			Expect(retrieved.Status.ReconciliationError.Code).To(Equal(hydrav1alpha1.StatusCreateSecretFailed))
+			Expect(retrieved.Status.ReconciliationError.Description).To(Equal(`secrets "test3" already exists`))
+		})
 	})
 })
 
@@ -123,7 +275,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 	return nil
 }
 
-func getAPIReconciler(mgr ctrl.Manager, mock *mockHydraClient) reconcile.Reconciler {
+func getAPIReconciler(mgr ctrl.Manager, mock controllers.HydraClientInterface) reconcile.Reconciler {
 	return &controllers.OAuth2ClientReconciler{
 		Client:      mgr.GetClient(),
 		Log:         ctrl.Log.WithName("controllers").WithName("OAuth2Client"),
@@ -131,67 +283,16 @@ func getAPIReconciler(mgr ctrl.Manager, mock *mockHydraClient) reconcile.Reconci
 	}
 }
 
-func testInstance(name, namespace, scopes string) *hydrav1alpha1.OAuth2Client {
+func testInstance(name string) *hydrav1alpha1.OAuth2Client {
 
 	return &hydrav1alpha1.OAuth2Client{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
-			Namespace: namespace,
+			Namespace: tstNamespace,
 		},
 		Spec: hydrav1alpha1.OAuth2ClientSpec{
 			GrantTypes:    []hydrav1alpha1.GrantType{"client_credentials"},
 			ResponseTypes: []hydrav1alpha1.ResponseType{"token"},
-			Scope:         scopes,
+			Scope:         tstScopes,
 		}}
 }
-
-//TODO: Replace with full-fledged mocking framework (mockery/go-mock)
-type mockHydraClient struct {
-	resSecret   string
-	resClientID string
-	postedData  *hydra.OAuth2ClientJSON
-}
-
-func (m *mockHydraClient) withSecret(secret string) *mockHydraClient {
-	m.resSecret = secret
-	return m
-}
-
-func (m *mockHydraClient) withClientID(clientID string) *mockHydraClient {
-	m.resClientID = clientID
-	return m
-}
-
-//Returns the data previously "stored" by PostOAuth2Client
-func (m *mockHydraClient) GetOAuth2Client(id string) (*hydra.OAuth2ClientJSON, bool, error) {
-	res := &hydra.OAuth2ClientJSON{
-		ClientID:      &m.resClientID,
-		Secret:        &m.resSecret,
-		Name:          m.postedData.Name,
-		GrantTypes:    m.postedData.GrantTypes,
-		ResponseTypes: m.postedData.ResponseTypes,
-		Scope:         m.postedData.Scope,
-	}
-	return res, true, nil
-}
-
-func (m *mockHydraClient) PostOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error) {
-	m.postedData = o
-	res := &hydra.OAuth2ClientJSON{
-		ClientID:      &m.resClientID,
-		Secret:        &m.resSecret,
-		Name:          o.Name,
-		GrantTypes:    o.GrantTypes,
-		ResponseTypes: o.ResponseTypes,
-		Scope:         o.Scope,
-	}
-	return res, nil
-}
-
-func (m *mockHydraClient) DeleteOAuth2Client(id string) error {
-	panic("Should not be invoked!")
-}
-
-func (m *mockHydraClient) PutOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error) {
-	panic("Should not be invoked!")
-}

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/go-logr/logr v0.1.0
 	github.com/onsi/ginkgo v1.6.0
 	github.com/onsi/gomega v1.4.2
+	github.com/pkg/errors v0.8.1
 	github.com/stretchr/testify v1.3.0
 	golang.org/x/net v0.0.0-20180906233101-161cd47e91fd
 	k8s.io/api v0.0.0-20190409021203-6e4e0e4f393b

go.sum

@@ -66,6 +66,7 @@ github.com/prometheus/procfs v0.0.0-20180725123919-05ee40e3a273/go.mod h1:c3At6R
 github.com/spf13/afero v1.2.2/go.mod h1:9ZxEEn6pIJ8Rxe320qSDBk6AsU0r9pR7Q4OcevTdifk=
 github.com/spf13/pflag v1.0.2 h1:Fy0orTDgHdbnzHcsOgfCN4LtHf0ec3wwtiwJqwvf3Gc=
 github.com/spf13/pflag v1.0.2/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
+github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=