feat: call cluster service to create identity cluster relation while creating user #745

dipak-pawar · 2018-12-17T11:58:00Z

Fixes: #744

…creating user

…auth into cluster_svc

codecov · 2018-12-17T12:15:49Z

Codecov Report

Merging #745 into master will decrease coverage by 0.02%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
- Coverage   79.06%   79.03%   -0.03%     
==========================================
  Files          94       94              
  Lines        8563     8623      +60     
==========================================
+ Hits         6770     6815      +45     
- Misses       1320     1329       +9     
- Partials      473      479       +6

Impacted Files	Coverage Δ
controller/users.go	`76.02% <33.33%> (-0.78%)`	⬇️
cluster/service/cluster.go	`86.32% <82.22%> (-9.6%)`	⬇️
wit/service/wit_service.go	`88.88% <0%> (-1.44%)`	⬇️
cluster/factory/cluster_cache_factory.go	`100% <0%> (ø)`	⬆️
cluster/service/cache.go	`86.66% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4581cc...ebaf3d2. Read the comment docs.

xcoulon

looks good, just a few suggestions/comments/questions

xcoulon · 2018-12-17T13:12:55Z

cluster/service/cluster.go

+	signer := cluster.NewJWTSASigner(ctx, s.config, options...)
+	remoteClusterService, err := signer.CreateSignedClient()
+	if err != nil {
+		return err


maybe wrap this error?

xcoulon · 2018-12-17T13:13:23Z

cluster/service/cluster.go

+	}
+	res, err := remoteClusterService.LinkIdentityToClusterClusters(goasupport.ForwardContextRequestID(ctx), clusterclient.LinkIdentityToClusterClustersPath(), identityToClusterData)
+	if err != nil {
+		return err


same here, wrap the error?

xcoulon · 2018-12-17T13:14:12Z

cluster/service/cluster.go

+	}
+	res, err := remoteClusterService.RemoveIdentityToClusterLinkClusters(goasupport.ForwardContextRequestID(ctx), clusterclient.RemoveIdentityToClusterLinkClustersPath(), identityToClusterData)
+	if err != nil {
+		return err


xcoulon · 2018-12-17T13:16:35Z

cluster/service/cluster.go

@@ -93,10 +155,10 @@ func Start(ctx context.Context, factory service.ClusterCacheFactory, options ...
 			} else {
 				clusterCache = nil
 			}
-			return (clusterCache != nil && started == uint32(1)), err
+			return clusterCache != nil && started == uint32(1), err


if this code tries to start the cache only once, then maybe https://golang.org/pkg/sync/#Once.Do would be more appropriate ?

This code can start the cache multiple times. It should initialize the cache successfully only once. But if it failed then it should keep trying. This is why we can't use once.Do() here.
And initialization can fail during auth service startup if cluster service is not ready yet (since it may be waiting for Auth service to start).

xcoulon · 2018-12-17T13:16:59Z

cluster/service/cluster.go

@@ -109,9 +171,9 @@ func Clusters(clusters map[string]cluster.Cluster) []cluster.Cluster {
 }

 func ClusterByURL(clusters map[string]cluster.Cluster, url string) *cluster.Cluster {
-	for apiURL, cluster := range clusters {
+	for apiURL, c := range clusters {


good work in avoiding var names with package names 👍

xcoulon · 2018-12-17T13:19:03Z

controller/users.go

+			"identity_id": identityID,
+			"cluster_url": clusterURL,
+		}, "failed to link identity to cluster in cluster service")
+		// Not a blocker. Log the error and proceed.


so what happens next if this error occurs? is there a way to retry the Identity/cluster link creation ?

This error is critical enough to fail here instead of proceed I think.
We should roll back user creation here (hard delete user&identity from DB) and return an error, so, the reg app can repeat provisioning later.
But this is not that easy to implement and will complicate that PR. So, let's add a TODO here and create a new issue to address this in a separate PR.

Created #747

alexeykazakov · 2018-12-17T22:53:06Z

application/service/services.go

@@ -62,6 +62,8 @@ type ClusterService interface {
 	Clusters(ctx context.Context, options ...rest.HTTPClientOption) ([]cluster.Cluster, error)
 	ClusterByURL(ctx context.Context, url string, options ...rest.HTTPClientOption) (*cluster.Cluster, error)
 	Status(ctx context.Context, options ...rest.HTTPClientOption) (bool, error)
+	RemoveIdentityToClusterLink(ctx context.Context, identityID uuid.UUID, clusterURL string, options ...rest.HTTPClientOption) error
+	AddIdentityToClusterLink(ctx context.Context, identityID uuid.UUID, clusterURL string, options ...rest.HTTPClientOption) error


Maybe

UnlinkIdentityFromCluster(...) LinkIdentityToCluster(...)

?
@sbryzak WDYT?

@alexeykazakov yes, I like your naming suggestions better

Addressed in ebaf3d2

alexeykazakov · 2018-12-17T22:58:41Z

cluster/cache.go

@@ -100,7 +98,8 @@ func (c *cache) refreshCache(ctx context.Context) error {

 // fetchClusters fetches a new list of clusters from Cluster Management Service
 func (c *cache) fetchClusters(ctx context.Context) (map[string]Cluster, error) {
-	cln, err := c.createClientWithServiceAccountSigner(ctx)
+	signer := NewJWTSASigner(ctx, c.config, c.options...)


Instead of exporting that NewJWTSASigner() function let's create a new method in the Cluster Service - AuthClientClusters(). Basically let's move all the business logic from fetchClusters() method of cache to AuthClientClusters() method of the service.
JWT signers and clients are an implementation details of the service.

But I see that it would create cycle dependency between cache (cluster) and service (cluster/service)...
We could solve this problem by moving cache.go to cluster/service.

We should get rid of this cache anyway btw. We should just load cluster by URL from cluster service every time and do not rely on the full list of clusters at all. But it's worth creating a separate issue and PR for that.

but how do we get a list of all cluster URL to fetch it once? Or are you saying when there is a request for any cluster explicitly from any client, then we should get it from cluster service each time?

External clients (such as tenant service, oso-proxy, jenkins-idler, etc) are proxied to cluster service. The cache is not used.
Internal clients (different auth services, like account linking) should not ask for the list of all clusters at all. They should ask for the particular cluster with particular URL. And they should call our cluster service every time.

But as I said let's just create an issue for that and do it in a separate PR.

Created #750

alexeykazakov · 2018-12-17T23:01:54Z

cluster/cluster.go

+	return cln, nil
+}
+
+func (c JWTSASigner) createClient(ctx context.Context) (*cluster.Client, error) {


As I mentioned above all these changes in cluster/cluster.go should be implementation details of the service package.

alexeykazakov · 2018-12-17T23:02:36Z

cluster/service/cluster.go

@@ -77,6 +83,62 @@ func (s *clusterService) Stop() {
 	}
 }

+func (s *clusterService) AddIdentityToClusterLink(ctx context.Context, identityID uuid.UUID, clusterURL string, options ...rest.HTTPClientOption) error {


Add docs please explaining what that method does.

Addressed in ebaf3d2

alexeykazakov · 2018-12-17T23:04:37Z

cluster/service/cluster.go

+	return nil
+}
+
+func (s *clusterService) RemoveIdentityToClusterLink(ctx context.Context, identityID uuid.UUID, clusterURL string, options ...rest.HTTPClientOption) error {


Addressed in ebaf3d2

alexeykazakov · 2018-12-17T23:25:20Z

cluster/service/cluster.go

@@ -93,10 +155,10 @@ func Start(ctx context.Context, factory service.ClusterCacheFactory, options ...
 			} else {
 				clusterCache = nil
 			}
-			return (clusterCache != nil && started == uint32(1)), err
+			return clusterCache != nil && started == uint32(1), err


This code can start the cache multiple times. It should initialize the cache successfully only once. But if it failed then it should keep trying. This is why we can't use once.Do() here.
And initialization can fail during auth service startup if cluster service is not ready yet (since it may be waiting for Auth service to start).

alexeykazakov · 2018-12-17T23:40:06Z

test/recorder/cluster.go

+	}
+}
+
+// WithLinkIdentityToClusterRequestPayloadMatcher an option to specify the RequestPayload matcher for the recorder


Fix the description. It's WithUnlink...

Addressed in ebaf3d2

alexeykazakov · 2018-12-17T23:40:50Z

test/recorder/recorder.go

-		if NotifyRequestPayloadMatcher(messageID)(httpRequest, cassetteRequest) {
-			authorization := httpRequest.Header.Get("Authorization")
-			reqID := httpRequest.Header.Get("X-Request-Id")
+// WithLinkIdentityToClusterRequestPayloadMatcher an option to specify the RequestPayload matcher for the recorder


Fix the description.

Addressed in ebaf3d2

alexeykazakov · 2018-12-17T23:59:00Z

test/recorder/cluster.go

+)
+
+type RequestParamMatcher interface {
+	MatchClusterURL(r cassette.Request, url string) bool


We also should match identityID. Or I missed something?

But actually I like https://github.com/h2non/gock more than go-vcr. It's IMO less verbose and code looks cleaner and easier to understand.

@MatousJobanek has been using it in tenant service for awhile.

@dipak-pawar would you mind trying to switch your test to gock instead? If you have concerns then we can discuss it. We could do it in a separate PR though.

Ideally yes we should match identityID, but I am using same identityID and different clusterURL, to match request from yaml file

Created #748

Sure but with IdentityID marcher you would also make sure that the identityID is set properly in the out-coming request. Right now if you don't pass the identity ID from the service to cluster the test won't fail.

I will improve it when introducing ginko

alexeykazakov · 2018-12-18T00:39:15Z

controller/users.go

+			"identity_id": identityID,
+			"cluster_url": clusterURL,
+		}, "failed to link identity to cluster in cluster service")
+		// Not a blocker. Log the error and proceed.


This error is critical enough to fail here instead of proceed I think.
We should roll back user creation here (hard delete user&identity from DB) and return an error, so, the reg app can repeat provisioning later.
But this is not that easy to implement and will complicate that PR. So, let's add a TODO here and create a new issue to address this in a separate PR.

xcoulon

PR looks good to me. Good job, @dipak-pawar !

xcoulon · 2018-12-20T13:25:36Z

cluster/service/cluster.go

@@ -93,10 +155,10 @@ func Start(ctx context.Context, factory service.ClusterCacheFactory, options ...
 			} else {
 				clusterCache = nil
 			}
-			return (clusterCache != nil && started == uint32(1)), err
+			return clusterCache != nil && started == uint32(1), err


xcoulon · 2018-12-20T13:30:11Z

cluster/service/cluster_blackbox_test.go

+		require.EqualError(t, err, "failed to unlink identity to cluster in cluster management service. Response status: 401 Unauthorized. Response body: unauthorized")
+	})
+
+	s.T().Run("404", func(t *testing.T) {


in its current state, the POST /api/clusters/identities and DELETE /api/clusters/identities endpoints cannot return 404 Not Found responses (https://github.com/fabric8-services/fabric8-cluster/blob/master/design/clusters.go#L125-L149), so this test may be necessary (but I guess there's nothing wrong in supporting this case at the client level anyways...)

it returns for DELETE look at https://github.com/fabric8-services/fabric8-cluster/blob/master/design/clusters.go#L146

oh thanks, I missed that :/ But it's not used in the controller (https://github.com/fabric8-services/fabric8-cluster/blob/master/controller/clusters.go#L175-L198) and IMO, we shouldn't return a 404 Not Found, but a 400 Bad Request if the parameters in the payload are invalid, IMO

Hmm, but it might be possible parameters in the payload are valid and there is no cluster available in DB. So, in that case, we should return 404 right?

you mean, returning a 400 Bad Request if the identity ID is not a valid UUID or if the cluster URL is not a parseable URL, but returning a 404 Not Found response if there's no record matching the identityID/clusterURL ? Yeah, I guess we could do that, indeed.

yes exactly, but we can do that only for clusterURL, because we try to find cluster using url. We don't try to find identity using identityID as we don't have any identity info in cluster. And I think it would be too much complext to check existence of identity by talking to auth.

but for unlinking, the 404 response would mean that the identity/cluster link record was not found, right? No need to call auth to check that there's actually an identify matching the ID?

btw we are already covering this. Look at https://github.com/fabric8-services/fabric8-cluster/blob/master/controller/clusters_blackbox_test.go#L354

ok, cool then ;)

alexeykazakov

Looks good. But IdentityID marcher would still improve the tests :)

feat: call cluster service to create identity cluster relation while …

547ba4e

…creating user

dipak-pawar requested review from xcoulon and alexeykazakov December 17, 2018 11:58

Merge branch 'master' of https://github.com/fabric8-services/fabric8-…

c3bacac

…auth into cluster_svc

dipak-pawar requested a review from sbryzak December 17, 2018 11:59

xcoulon reviewed Dec 17, 2018

View reviewed changes

alexeykazakov reviewed Dec 18, 2018

View reviewed changes

address review comments

ebaf3d2

xcoulon approved these changes Dec 20, 2018

View reviewed changes

alexeykazakov approved these changes Dec 21, 2018

View reviewed changes

dipak-pawar merged commit 7f83446 into fabric8-services:master Dec 21, 2018

pmacik mentioned this pull request Jan 29, 2019

Unable to create/register user in Auth #762

Closed

feat: call cluster service to create identity cluster relation while creating user #745

feat: call cluster service to create identity cluster relation while creating user #745

Conversation

dipak-pawar commented Dec 17, 2018

codecov bot commented Dec 17, 2018 • edited Loading

Codecov Report

xcoulon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2018 •

edited

Loading