Skip to content

Commit c98ce56

Browse files
footfoot
authored
Wait until cluster is ready before adding to MC-query client + move client creation out of middleware (#887)
- Bring in changes from core Move kube Client creation into the handlers and out of the middleware weave-gitops#2252 that moves client creation out of middleware, so routes that don't use the MC-querier (like /v1/clusters), will still work even if there is an issue w/ the MC-querier - Check for Ready before adding GitopsCluster to the MC-querier: - Refactor tests a bit as we have to pass through clientsFactory everywhere
1 parent 71da48f commit c98ce56

File tree

14 files changed

+175
-42
lines changed

14 files changed

+175
-42
lines changed

‎cmd/clusters-service/app/server.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,7 @@ func RunInProcessGateway(ctx context.Context, addr string, setters ...Option) er
435435
args.Log,
436436
args.ClustersLibrary,
437437
args.TemplateLibrary,
438+
args.CoreServerConfig.ClientsFactory,
438439
args.GitProvider,
439440
args.ClientGetter,
440441
args.DiscoveryClient,
@@ -469,8 +470,6 @@ func RunInProcessGateway(ctx context.Context, addr string, setters ...Option) er
469470
return fmt.Errorf("could not register new app server: %w", err)
470471
}
471472

472-
grpcHttpHandler = clustersmngr.WithClustersClient(args.CoreServerConfig.ClientsFactory, grpcHttpHandler)
473-
474473
// UI
475474
args.Log.Info("Attaching FileServer", "HtmlRootPath", args.HtmlRootPath)
476475
staticAssets := http.StripPrefix("/", http.FileServer(&spaFileSystem{http.Dir(args.HtmlRootPath)}))

‎cmd/clusters-service/pkg/server/clusters_test.go

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,10 @@ func TestListGitopsClusters(t *testing.T) {
204204
viper.SetDefault("runtime-namespace", "default")
205205

206206
// setup
207-
gp := NewFakeGitProvider("", nil, nil)
208-
s := createServer(t, tt.clusterState, "capi-templates", "default", gp, "", nil)
207+
s := createServer(t, serverOptions{
208+
clusterState: tt.clusterState,
209+
namespace: "default",
210+
})
209211

210212
// request
211213
listGitopsClustersRequest := new(capiv1_protos.ListGitopsClustersRequest)
@@ -419,7 +421,13 @@ status: {}
419421
hr.Namespace = "default"
420422
})
421423
tt.clusterState = append(tt.clusterState, hr)
422-
s := createServer(t, tt.clusterState, "capi-templates", "default", tt.provider, "", hr)
424+
s := createServer(t, serverOptions{
425+
clusterState: tt.clusterState,
426+
configMapName: "capi-templates",
427+
namespace: "default",
428+
provider: tt.provider,
429+
hr: hr,
430+
})
423431

424432
// request
425433
createPullRequestResponse, err := s.CreatePullRequest(context.Background(), tt.req)
@@ -527,8 +535,12 @@ func TestGetKubeconfig(t *testing.T) {
527535
t.Run(tt.name, func(t *testing.T) {
528536
viper.SetDefault("capi-clusters-namespace", tt.clusterObjectsNamespace)
529537

530-
gp := NewFakeGitProvider("", nil, nil)
531-
s := createServer(t, tt.clusterState, "capi-templates", "default", gp, tt.clusterObjectsNamespace, nil)
538+
s := createServer(t, serverOptions{
539+
clusterState: tt.clusterState,
540+
configMapName: "capi-templates",
541+
namespace: "default",
542+
ns: tt.clusterObjectsNamespace,
543+
})
532544

533545
res, err := s.GetKubeconfig(tt.ctx, tt.req)
534546

@@ -597,7 +609,11 @@ func TestDeleteClustersPullRequest(t *testing.T) {
597609
for _, tt := range testCases {
598610
t.Run(tt.name, func(t *testing.T) {
599611
// setup
600-
s := createServer(t, []runtime.Object{}, "capi-templates", "default", tt.provider, "", nil)
612+
s := createServer(t, serverOptions{
613+
configMapName: "capi-templates",
614+
namespace: "default",
615+
provider: tt.provider,
616+
})
601617

602618
// delete request
603619
deletePullRequestResponse, err := s.DeleteClustersPullRequest(context.Background(), tt.req)

‎cmd/clusters-service/pkg/server/common_test.go

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"sigs.k8s.io/controller-runtime/pkg/client/fake"
1717
"sigs.k8s.io/yaml"
1818

19+
"github.com/weaveworks/weave-gitops/core/clustersmngr"
1920
"github.com/weaveworks/weave-gitops/pkg/kube/kubefakes"
2021

2122
pacv1 "github.com/weaveworks/policy-agent/api/v1"
@@ -55,27 +56,38 @@ func createClient(t *testing.T, clusterState ...runtime.Object) client.Client {
5556
return c
5657
}
5758

58-
func createServer(t *testing.T, clusterState []runtime.Object, configMapName, namespace string, provider git.Provider, ns string, hr *sourcev1.HelmRepository) capiv1_protos.ClustersServiceServer {
59-
c := createClient(t, clusterState...)
59+
type serverOptions struct {
60+
clusterState []runtime.Object
61+
configMapName string
62+
namespace string
63+
provider git.Provider
64+
ns string
65+
hr *sourcev1.HelmRepository
66+
clientsFactory clustersmngr.ClientsFactory
67+
}
68+
69+
func createServer(t *testing.T, o serverOptions) capiv1_protos.ClustersServiceServer {
70+
c := createClient(t, o.clusterState...)
6071
dc := discovery.NewDiscoveryClient(fakeclientset.NewSimpleClientset().Discovery().RESTClient())
6172

6273
return NewClusterServer(
6374
logr.Discard(),
6475
&clusters.CRDLibrary{
6576
Log: logr.Discard(),
6677
ClientGetter: kubefakes.NewFakeClientGetter(c),
67-
Namespace: namespace,
78+
Namespace: o.namespace,
6879
},
6980
&templates.ConfigMapLibrary{
7081
Log: logr.Discard(),
7182
Client: c,
72-
ConfigMapName: configMapName,
73-
CAPINamespace: namespace,
83+
ConfigMapName: o.configMapName,
84+
CAPINamespace: o.namespace,
7485
},
75-
provider,
86+
o.clientsFactory,
87+
o.provider,
7688
kubefakes.NewFakeClientGetter(c),
7789
dc,
78-
ns,
90+
o.ns,
7991
"weaveworks-charts", t.TempDir(),
8092
)
8193
}

‎cmd/clusters-service/pkg/server/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestGetConfig(t *testing.T) {
3030
t.Run(tt.name, func(t *testing.T) {
3131
viper.SetDefault("capi-templates-repository-url", tt.value)
3232

33-
s := createServer(t, nil, "", "", nil, "", nil)
33+
s := createServer(t, serverOptions{})
3434

3535
res, _ := s.GetConfig(context.Background(), &capiv1_protos.GetConfigRequest{})
3636

‎cmd/clusters-service/pkg/server/policies.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
pacv1 "github.com/weaveworks/policy-agent/api/v1"
1313
capiv1_proto "github.com/weaveworks/weave-gitops-enterprise/cmd/clusters-service/pkg/protos"
1414
"github.com/weaveworks/weave-gitops/core/clustersmngr"
15+
"github.com/weaveworks/weave-gitops/pkg/server/auth"
1516
"google.golang.org/protobuf/types/known/anypb"
1617
"google.golang.org/protobuf/types/known/wrapperspb"
1718
"k8s.io/apimachinery/pkg/types"
@@ -127,7 +128,10 @@ func toPolicyResponse(policyCRD pacv1.Policy, clusterName string) (*capiv1_proto
127128
}
128129

129130
func (s *server) ListPolicies(ctx context.Context, m *capiv1_proto.ListPoliciesRequest) (*capiv1_proto.ListPoliciesResponse, error) {
130-
clustersClient := clustersmngr.ClientFromCtx(ctx)
131+
clustersClient, err := s.clientsFactory.GetImpersonatedClient(ctx, auth.Principal(ctx))
132+
if err != nil {
133+
return nil, fmt.Errorf("error getting impersonating client: %s", err)
134+
}
131135

132136
clist := clustersmngr.NewClusteredList(func() client.ObjectList {
133137
return &pacv1.PolicyList{}
@@ -178,13 +182,16 @@ func (s *server) ListPolicies(ctx context.Context, m *capiv1_proto.ListPoliciesR
178182
}
179183

180184
func (s *server) GetPolicy(ctx context.Context, m *capiv1_proto.GetPolicyRequest) (*capiv1_proto.GetPolicyResponse, error) {
181-
clustersClient := clustersmngr.ClientFromCtx(ctx)
185+
clustersClient, err := s.clientsFactory.GetImpersonatedClient(ctx, auth.Principal(ctx))
186+
if err != nil {
187+
return nil, fmt.Errorf("error getting impersonating client: %s", err)
188+
}
182189

183190
if m.ClusterName == "" {
184191
return nil, requiredClusterNameErr
185192
}
186193
policyCR := pacv1.Policy{}
187-
err := clustersClient.Get(ctx, m.ClusterName, types.NamespacedName{Name: m.PolicyName}, &policyCR)
194+
err = clustersClient.Get(ctx, m.ClusterName, types.NamespacedName{Name: m.PolicyName}, &policyCR)
188195
if err != nil {
189196
return nil, fmt.Errorf("error while getting policy %s from cluster %s: %w", m.PolicyName, m.ClusterName, err)
190197
}

‎cmd/clusters-service/pkg/server/policies_test.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -270,18 +270,23 @@ func TestListPolicies(t *testing.T) {
270270
err: errors.New("found unsupported policy parameter type invalid in policy "),
271271
},
272272
}
273-
s := createServer(t, []runtime.Object{}, "policies", "default", nil, "", nil)
274273
for _, tt := range tests {
275274
t.Run(tt.name, func(t *testing.T) {
276275
clientsPool := &clustersmngrfakes.FakeClientsPool{}
277276
fakeCl := createClient(t, tt.clusterState...)
278277
clientsPool.ClientsReturns(map[string]client.Client{"Default": fakeCl})
279278
clientsPool.ClientReturns(fakeCl, nil)
280279
clustersClient := clustersmngr.NewClient(clientsPool, map[string][]v1.Namespace{})
281-
ctx := context.WithValue(context.Background(), clustersmngr.ClustersClientCtxKey, clustersClient)
280+
281+
fakeFactory := &clustersmngrfakes.FakeClientsFactory{}
282+
fakeFactory.GetImpersonatedClientReturns(clustersClient, nil)
283+
284+
s := createServer(t, serverOptions{
285+
clientsFactory: fakeFactory,
286+
})
282287

283288
req := capiv1_proto.ListPoliciesRequest{ClusterName: tt.clusterName}
284-
gotResponse, err := s.ListPolicies(ctx, &req)
289+
gotResponse, err := s.ListPolicies(context.Background(), &req)
285290
if err != nil {
286291
if tt.err == nil {
287292
t.Fatalf("failed to list policies:\n%s", err)
@@ -394,17 +399,22 @@ func TestGetPolicy(t *testing.T) {
394399
err: requiredClusterNameErr,
395400
},
396401
}
397-
s := createServer(t, []runtime.Object{}, "policies", "default", nil, "", nil)
398402
for _, tt := range tests {
399403
t.Run(tt.name, func(t *testing.T) {
400404
clientsPool := &clustersmngrfakes.FakeClientsPool{}
401405
fakeCl := createClient(t, tt.clusterState...)
402406
clientsPool.ClientsReturns(map[string]client.Client{tt.clusterName: fakeCl})
403407
clientsPool.ClientReturns(fakeCl, nil)
404408
clustersClient := clustersmngr.NewClient(clientsPool, map[string][]v1.Namespace{})
405-
ctx := context.WithValue(context.Background(), clustersmngr.ClustersClientCtxKey, clustersClient)
406409

407-
gotResponse, err := s.GetPolicy(ctx, &capiv1_proto.GetPolicyRequest{
410+
fakeFactory := &clustersmngrfakes.FakeClientsFactory{}
411+
fakeFactory.GetImpersonatedClientReturns(clustersClient, nil)
412+
413+
s := createServer(t, serverOptions{
414+
clientsFactory: fakeFactory,
415+
})
416+
417+
gotResponse, err := s.GetPolicy(context.Background(), &capiv1_proto.GetPolicyRequest{
408418
PolicyName: tt.policyName,
409419
ClusterName: tt.clusterName})
410420
if err != nil {

‎cmd/clusters-service/pkg/server/policy_violations_test.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ func TestGetPolicyViolation(t *testing.T) {
7373
for _, tt := range tests {
7474
t.Run(tt.name, func(t *testing.T) {
7575

76-
s := createServer(t, tt.clusterState, "policies", "default", nil, "", nil)
76+
s := createServer(t, serverOptions{
77+
clusterState: tt.clusterState,
78+
})
7779

7880
policyViolation, err := s.GetPolicyValidation(context.Background(), &capiv1_proto.GetPolicyValidationRequest{
7981
ViolationId: tt.ViolationId,
@@ -146,7 +148,9 @@ func TestListPolicyValidations(t *testing.T) {
146148

147149
for _, tt := range tests {
148150
t.Run(tt.name, func(t *testing.T) {
149-
s := createServer(t, tt.clusterState, "policies", "default", nil, "", nil)
151+
s := createServer(t, serverOptions{
152+
clusterState: tt.clusterState,
153+
})
150154
policyViolation, err := s.ListPolicyValidations(context.Background(), &capiv1_proto.ListPolicyValidationsRequest{})
151155
if err != nil {
152156
if tt.err == nil {

‎cmd/clusters-service/pkg/server/server.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package server
22

33
import (
44
"github.com/go-logr/logr"
5+
"github.com/weaveworks/weave-gitops/core/clustersmngr"
56
"github.com/weaveworks/weave-gitops/pkg/kube"
67
"k8s.io/client-go/discovery"
78

@@ -29,6 +30,7 @@ type server struct {
2930
log logr.Logger
3031
templatesLibrary templates.Library
3132
clustersLibrary clusters.Library
33+
clientsFactory clustersmngr.ClientsFactory
3234
provider git.Provider
3335
clientGetter kube.ClientGetter
3436
discoveryClient discovery.DiscoveryInterface
@@ -38,11 +40,12 @@ type server struct {
3840
helmRepositoryCacheDir string
3941
}
4042

41-
func NewClusterServer(log logr.Logger, clustersLibrary clusters.Library, templatesLibrary templates.Library, provider git.Provider, clientGetter kube.ClientGetter, discoveryClient discovery.DiscoveryInterface, ns string, profileHelmRepositoryName string, helmRepositoryCacheDir string) capiv1_proto.ClustersServiceServer {
43+
func NewClusterServer(log logr.Logger, clustersLibrary clusters.Library, templatesLibrary templates.Library, clientsFactory clustersmngr.ClientsFactory, provider git.Provider, clientGetter kube.ClientGetter, discoveryClient discovery.DiscoveryInterface, ns string, profileHelmRepositoryName string, helmRepositoryCacheDir string) capiv1_proto.ClustersServiceServer {
4244
return &server{
4345
log: log,
4446
clustersLibrary: clustersLibrary,
4547
templatesLibrary: templatesLibrary,
48+
clientsFactory: clientsFactory,
4649
provider: provider,
4750
clientGetter: clientGetter,
4851
discoveryClient: discoveryClient,

‎cmd/clusters-service/pkg/server/templates_test.go

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,11 @@ func TestListTemplates(t *testing.T) {
122122

123123
for _, tt := range testCases {
124124
t.Run(tt.name, func(t *testing.T) {
125-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
125+
s := createServer(t, serverOptions{
126+
clusterState: tt.clusterState,
127+
configMapName: "capi-templates",
128+
namespace: "default",
129+
})
126130

127131
listTemplatesRequest := &capiv1_protos.ListTemplatesRequest{
128132
TemplateKind: capiv1.Kind,
@@ -247,7 +251,11 @@ func TestListClusterTemplates(t *testing.T) {
247251

248252
for _, tt := range testCases {
249253
t.Run(tt.name, func(t *testing.T) {
250-
s := createServer(t, tt.clusterState, "cluster-templates", "default", nil, "", nil)
254+
s := createServer(t, serverOptions{
255+
clusterState: tt.clusterState,
256+
configMapName: "cluster-templates",
257+
namespace: "default",
258+
})
251259

252260
listTemplatesRequest := &capiv1_protos.ListTemplatesRequest{
253261
TemplateKind: gapiv1.Kind,
@@ -366,7 +374,11 @@ func TestListTemplates_FilterByProvider(t *testing.T) {
366374

367375
for _, tt := range testCases {
368376
t.Run(tt.name, func(t *testing.T) {
369-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
377+
s := createServer(t, serverOptions{
378+
clusterState: tt.clusterState,
379+
configMapName: "capi-templates",
380+
namespace: "default",
381+
})
370382

371383
listTemplatesRequest := new(capiv1_protos.ListTemplatesRequest)
372384
listTemplatesRequest.Provider = tt.provider
@@ -433,7 +445,11 @@ func TestGetTemplate(t *testing.T) {
433445

434446
for _, tt := range testCases {
435447
t.Run(tt.name, func(t *testing.T) {
436-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
448+
s := createServer(t, serverOptions{
449+
clusterState: tt.clusterState,
450+
configMapName: "capi-templates",
451+
namespace: "default",
452+
})
437453
getTemplateRes, err := s.GetTemplate(context.Background(), &capiv1_protos.GetTemplateRequest{TemplateName: "cluster-template-1"})
438454
if err != nil && tt.err == nil {
439455
t.Fatalf("failed to read the templates:\n%s", err)
@@ -478,7 +494,11 @@ func TestListTemplateParams(t *testing.T) {
478494

479495
for _, tt := range testCases {
480496
t.Run(tt.name, func(t *testing.T) {
481-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
497+
s := createServer(t, serverOptions{
498+
clusterState: tt.clusterState,
499+
configMapName: "capi-templates",
500+
namespace: "default",
501+
})
482502

483503
listTemplateParamsRequest := new(capiv1_protos.ListTemplateParamsRequest)
484504
listTemplateParamsRequest.TemplateName = "cluster-template-1"
@@ -532,7 +552,11 @@ func TestListTemplateProfiles(t *testing.T) {
532552

533553
for _, tt := range testCases {
534554
t.Run(tt.name, func(t *testing.T) {
535-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
555+
s := createServer(t, serverOptions{
556+
clusterState: tt.clusterState,
557+
configMapName: "capi-templates",
558+
namespace: "default",
559+
})
536560

537561
listTemplateProfilesRequest := new(capiv1_protos.ListTemplateProfilesRequest)
538562
listTemplateProfilesRequest.TemplateName = "cluster-template-1"
@@ -652,7 +676,11 @@ func TestRenderTemplate(t *testing.T) {
652676
viper.SetDefault("inject-prune-annotation", tt.pruneEnvVar)
653677
viper.SetDefault("capi-clusters-namespace", tt.clusterNamespace)
654678

655-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
679+
s := createServer(t, serverOptions{
680+
clusterState: tt.clusterState,
681+
configMapName: "capi-templates",
682+
namespace: "default",
683+
})
656684

657685
renderTemplateRequest := &capiv1_protos.RenderTemplateRequest{
658686
TemplateName: "cluster-template-1",
@@ -683,7 +711,11 @@ func TestRenderTemplate_MissingVariables(t *testing.T) {
683711
clusterState := []runtime.Object{
684712
makeTemplateConfigMap("capi-templates", "template1", makeCAPITemplate(t)),
685713
}
686-
s := createServer(t, clusterState, "capi-templates", "default", nil, "", nil)
714+
s := createServer(t, serverOptions{
715+
clusterState: clusterState,
716+
configMapName: "capi-templates",
717+
namespace: "default",
718+
})
687719

688720
renderTemplateRequest := &capiv1_protos.RenderTemplateRequest{
689721
TemplateName: "cluster-template-1",
@@ -748,7 +780,11 @@ func TestRenderTemplate_ValidateVariables(t *testing.T) {
748780
t.Run(tt.name, func(t *testing.T) {
749781
viper.Reset()
750782

751-
s := createServer(t, tt.clusterState, "capi-templates", "default", nil, "", nil)
783+
s := createServer(t, serverOptions{
784+
clusterState: tt.clusterState,
785+
configMapName: "capi-templates",
786+
namespace: "default",
787+
})
752788

753789
renderTemplateRequest := &capiv1_protos.RenderTemplateRequest{
754790
TemplateName: "cluster-template-1",

0 commit comments

Comments
 (0)