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.

Sign up for GitHub

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

fix(controller): Ensure that OAuth2Client reconciliation creates hydra client for specs #83

Merged
merged 6 commits into from
Sep 14, 2021
Merged

fix(controller): Ensure that OAuth2Client reconciliation creates hydra client for specs #83

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
efb43a1
Ensure that OAuth2Client reconciliation creates hydra client for specs
alexandrem Aug 27, 2021
938e38c
Remove unnecessary comment change
alexandrem Aug 27, 2021
3868188
Regenerate mocks
alexandrem Sep 2, 2021
9633930
Add oauth2 factory for mock testing and fix concurrency bug
alexandrem Sep 2, 2021
5b02a08
Add some comments to new types and functions
alexandrem Sep 2, 2021
9fba335
Remove debug print statement
alexandrem Sep 2, 2021
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
51 changes: 0 additions & 51 deletions api/v1alpha1/oauth2client_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ limitations under the License.
package v1alpha1

import (
"encoding/json"
"fmt"

"github.com/ory/hydra-maester/hydra"
"github.com/pkg/errors"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
Expand Down Expand Up @@ -185,49 +180,3 @@ type OAuth2ClientList struct {
func init() {
SchemeBuilder.Register(&OAuth2Client{}, &OAuth2ClientList{})
}

// ToOAuth2ClientJSON converts an OAuth2Client into a OAuth2ClientJSON object that represents an OAuth2 client digestible by ORY Hydra
func (c *OAuth2Client) ToOAuth2ClientJSON() (*hydra.OAuth2ClientJSON, error) {
meta, err := json.Marshal(c.Spec.Metadata)
if err != nil {
return nil, errors.WithMessage(err, "unable to encode `metadata` property value to json")
}

return &hydra.OAuth2ClientJSON{
ClientName: c.Spec.ClientName,
GrantTypes: grantToStringSlice(c.Spec.GrantTypes),
ResponseTypes: responseToStringSlice(c.Spec.ResponseTypes),
RedirectURIs: redirectToStringSlice(c.Spec.RedirectURIs),
PostLogoutRedirectURIs: redirectToStringSlice(c.Spec.PostLogoutRedirectURIs),
AllowedCorsOrigins: redirectToStringSlice(c.Spec.AllowedCorsOrigins),
Audience: c.Spec.Audience,
Scope: c.Spec.Scope,
Owner: fmt.Sprintf("%s/%s", c.Name, c.Namespace),
TokenEndpointAuthMethod: string(c.Spec.TokenEndpointAuthMethod),
Metadata: meta,
}, nil
}

func responseToStringSlice(rt []ResponseType) []string {
var output = make([]string, len(rt))
for i, elem := range rt {
output[i] = string(elem)
}
return output
}

func grantToStringSlice(gt []GrantType) []string {
var output = make([]string, len(gt))
for i, elem := range gt {
output[i] = string(elem)
}
return output
}

func redirectToStringSlice(ru []RedirectURI) []string {
var output = make([]string, len(ru))
for i, elem := range ru {
output[i] = string(elem)
}
return output
}
84 changes: 53 additions & 31 deletions controllers/oauth2client_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,46 +18,58 @@ package controllers
import (
"context"
"fmt"

"github.com/go-logr/logr"
hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
"github.com/ory/hydra-maester/hydra"
"github.com/pkg/errors"
apiv1 "k8s.io/api/core/v1"
apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
"github.com/ory/hydra-maester/hydra"
)

const (
ClientIDKey = "client_id"
ClientSecretKey = "client_secret"
FinalizerName = "finalizer.ory.hydra.sh"

DefaultNamespace = "default"
)

type clientMapKey struct {
type clientKey struct {
url string
port int
endpoint string
forwardedProto string
}

type HydraClientInterface interface {
GetOAuth2Client(id string) (*hydra.OAuth2ClientJSON, bool, error)
ListOAuth2Client() ([]*hydra.OAuth2ClientJSON, error)
PostOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error)
PutOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error)
DeleteOAuth2Client(id string) error
}

// OAuth2ClientReconciler reconciles a OAuth2Client object
// OAuth2ClientReconciler reconciles a OAuth2Client object.
type OAuth2ClientReconciler struct {
HydraClient HydraClientInterface
Log logr.Logger
otherClients map[clientMapKey]HydraClientInterface
client.Client
HydraClient hydra.Client
Log logr.Logger
ControllerNamespace string

oauth2Clients map[clientKey]hydra.Client
piotrmsc marked this conversation as resolved.
Show resolved Hide resolved
}

// New returns a new Oauth2ClientReconciler.
func New(c client.Client, hydraClient hydra.Client, log logr.Logger, ns string) *OAuth2ClientReconciler {
if ns == "" {
ns = DefaultNamespace
}

return &OAuth2ClientReconciler{
Client: c,
HydraClient: hydraClient,
Log: log,
ControllerNamespace: ns,
oauth2Clients: make(map[clientKey]hydra.Client, 0),
}
}

// +kubebuilder:rbac:groups=hydra.ory.sh,resources=oauth2clients,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -200,12 +212,12 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, c *hy
return err
}

hydra, err := r.getHydraClientForClient(*c)
hydraClient, err := r.getHydraClientForClient(*c)
if err != nil {
return err
}

oauth2client, err := c.ToOAuth2ClientJSON()
oauth2client, err := hydra.FromOAuth2Client(c)
if err != nil {
if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err); updateErr != nil {
return updateErr
Expand All @@ -214,15 +226,15 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, c *hy
}

if credentials != nil {
if _, err := hydra.PostOAuth2Client(oauth2client.WithCredentials(credentials)); err != nil {
if _, err := hydraClient.PostOAuth2Client(oauth2client.WithCredentials(credentials)); err != nil {
if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err); updateErr != nil {
return updateErr
}
}
return r.ensureEmptyStatusError(ctx, c)
}

created, err := hydra.PostOAuth2Client(oauth2client)
created, err := hydraClient.PostOAuth2Client(oauth2client)
if err != nil {
if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err); updateErr != nil {
return updateErr
Expand Down Expand Up @@ -260,20 +272,20 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, c *hy
}

func (r *OAuth2ClientReconciler) updateRegisteredOAuth2Client(ctx context.Context, c *hydrav1alpha1.OAuth2Client, credentials *hydra.Oauth2ClientCredentials) error {
hydra, err := r.getHydraClientForClient(*c)
hydraClient, err := r.getHydraClientForClient(*c)
if err != nil {
return err
}

oauth2client, err := c.ToOAuth2ClientJSON()
oauth2client, err := hydra.FromOAuth2Client(c)
if err != nil {
if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusUpdateFailed, err); updateErr != nil {
return updateErr
}
return errors.WithStack(err)
}

if _, err := hydra.PutOAuth2Client(oauth2client.WithCredentials(credentials)); err != nil {
if _, err := hydraClient.PutOAuth2Client(oauth2client.WithCredentials(credentials)); err != nil {
if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusUpdateFailed, err); updateErr != nil {
return updateErr
}
Expand Down Expand Up @@ -352,17 +364,27 @@ func parseSecret(secret apiv1.Secret, authMethod hydrav1alpha1.TokenEndpointAuth
}, nil
}

func (r *OAuth2ClientReconciler) getHydraClientForClient(oauth2client hydrav1alpha1.OAuth2Client) (HydraClientInterface, error) {
func (r *OAuth2ClientReconciler) getHydraClientForClient(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This IMO should have a test, unit at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll work on this today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually I find it a bit anti-idiomatic to create a unit test for a private function.

I'm wondering if we should we create an integration test for this instead?

oauth2client hydrav1alpha1.OAuth2Client) (hydra.Client, error) {
spec := oauth2client.Spec
key := clientMapKey{
url: spec.HydraAdmin.URL,
port: spec.HydraAdmin.Port,
endpoint: spec.HydraAdmin.Endpoint,
forwardedProto: spec.HydraAdmin.ForwardedProto,
}
if c, ok := r.otherClients[key]; ok {
return c, nil
if spec.HydraAdmin.URL != "" {
key := clientKey{
url: spec.HydraAdmin.URL,
port: spec.HydraAdmin.Port,
endpoint: spec.HydraAdmin.Endpoint,
forwardedProto: spec.HydraAdmin.ForwardedProto,
}
if c, ok := r.oauth2Clients[key]; ok {
return c, nil
}
client, err := hydra.New(spec, "", false)
if err != nil {
return nil, errors.Wrap(err, "cannot create oauth2 client from CRD")
}
r.oauth2Clients[key] = client
return client, nil
}

if r.HydraClient == nil {
return nil, errors.New("Not default client or other clients configured")
}
Expand Down
22 changes: 12 additions & 10 deletions controllers/oauth2client_controller_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ import (

. "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/controllers/mocks"
"github.com/ory/hydra-maester/hydra"
. "github.com/stretchr/testify/mock"
apiv1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -27,6 +23,11 @@ import (
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
"github.com/ory/hydra-maester/controllers"
"github.com/ory/hydra-maester/controllers/mocks"
"github.com/ory/hydra-maester/hydra"
)

const (
Expand Down Expand Up @@ -460,12 +461,13 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
return nil
}

func getAPIReconciler(mgr ctrl.Manager, mock controllers.HydraClientInterface) reconcile.Reconciler {
return &controllers.OAuth2ClientReconciler{
Client: mgr.GetClient(),
Log: ctrl.Log.WithName("controllers").WithName("OAuth2Client"),
HydraClient: mock,
}
func getAPIReconciler(mgr ctrl.Manager, mock hydra.Client) reconcile.Reconciler {
return controllers.New(
mgr.GetClient(),
mock,
ctrl.Log.WithName("controllers").WithName("OAuth2Client"),
"",
)
}

func testInstance(name, secretName string) *hydrav1alpha1.OAuth2Client {
Expand Down
54 changes: 45 additions & 9 deletions hydra/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,51 @@ import (
"net/http"
"net/url"
"path"

hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
"github.com/ory/hydra-maester/helpers"
)

type Client struct {
type Client interface {
piotrmsc marked this conversation as resolved.
Show resolved Hide resolved
GetOAuth2Client(id string) (*OAuth2ClientJSON, bool, error)
ListOAuth2Client() ([]*OAuth2ClientJSON, error)
PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error)
PutOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error)
DeleteOAuth2Client(id string) error
}

type InternalClient struct {
HydraURL url.URL
HTTPClient *http.Client
ForwardedProto string
}

func (c *Client) GetOAuth2Client(id string) (*OAuth2ClientJSON, bool, error) {
// New returns a new hydra InternalClient instance.
func New(spec hydrav1alpha1.OAuth2ClientSpec, tlsTrustStore string, insecureSkipVerify bool) (Client, error) {
address := fmt.Sprintf("%s:%d", spec.HydraAdmin.URL, spec.HydraAdmin.Port)
u, err := url.Parse(address)
if err != nil {
return nil, err
}

c, err := helpers.CreateHttpClient(insecureSkipVerify, tlsTrustStore)
if err != nil {
return nil, err
}

client := &InternalClient{
HydraURL: *u.ResolveReference(&url.URL{Path: spec.HydraAdmin.Endpoint}),
HTTPClient: c,
}

if spec.HydraAdmin.ForwardedProto != "" && spec.HydraAdmin.ForwardedProto != "off" {
client.ForwardedProto = spec.HydraAdmin.ForwardedProto
}

return client, nil
}

func (c *InternalClient) GetOAuth2Client(id string) (*OAuth2ClientJSON, bool, error) {

var jsonClient *OAuth2ClientJSON

Expand All @@ -40,7 +76,7 @@ func (c *Client) GetOAuth2Client(id string) (*OAuth2ClientJSON, bool, error) {
}
}

func (c *Client) ListOAuth2Client() ([]*OAuth2ClientJSON, error) {
func (c *InternalClient) ListOAuth2Client() ([]*OAuth2ClientJSON, error) {

var jsonClientList []*OAuth2ClientJSON

Expand All @@ -62,7 +98,7 @@ func (c *Client) ListOAuth2Client() ([]*OAuth2ClientJSON, error) {
}
}

func (c *Client) PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error) {
func (c *InternalClient) PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error) {

var jsonClient *OAuth2ClientJSON

Expand All @@ -86,7 +122,7 @@ func (c *Client) PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error
}
}

func (c *Client) PutOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error) {
func (c *InternalClient) PutOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error) {

var jsonClient *OAuth2ClientJSON

Expand All @@ -107,7 +143,7 @@ func (c *Client) PutOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error)
return jsonClient, nil
}

func (c *Client) DeleteOAuth2Client(id string) error {
func (c *InternalClient) DeleteOAuth2Client(id string) error {

req, err := c.newRequest(http.MethodDelete, id, nil)
if err != nil {
Expand All @@ -123,14 +159,14 @@ func (c *Client) DeleteOAuth2Client(id string) error {
case http.StatusNoContent:
return nil
case http.StatusNotFound:
fmt.Printf("client with id %s does not exist", id)
fmt.Printf("InternalClient with id %s does not exist", id)
return nil
default:
return fmt.Errorf("%s %s http request returned unexpected status code %s", req.Method, req.URL.String(), resp.Status)
}
}

func (c *Client) newRequest(method, relativePath string, body interface{}) (*http.Request, error) {
func (c *InternalClient) newRequest(method, relativePath string, body interface{}) (*http.Request, error) {

var buf io.ReadWriter
if body != nil {
Expand Down Expand Up @@ -162,7 +198,7 @@ func (c *Client) newRequest(method, relativePath string, body interface{}) (*htt

}

func (c *Client) do(req *http.Request, v interface{}) (*http.Response, error) {
func (c *InternalClient) do(req *http.Request, v interface{}) (*http.Response, error) {
resp, err := c.HTTPClient.Do(req)
if err != nil {
return nil, err
Expand Down
Loading