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(server): don't use cluster scope list + watch in namespaced mode. Fixes #13177 #13189

Merged
merged 3 commits into from
Jun 16, 2024
Merged

fix(server): don't use cluster scope list + watch in namespaced mode. Fixes #13177 #13189

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a148a36
fix: list and watch permission missing for server in namespaced mode
jiachengxu Jun 14, 2024
56e8e45
make the namespace a pointer
jiachengxu Jun 16, 2024
ccec14a
fix the workflow server test
jiachengxu Jun 16, 2024
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
2 changes: 1 addition & 1 deletion pkg/apiclient/argo-kube-client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func newArgoKubeClient(ctx context.Context, clientConfig clientcmd.ClientConfig,
func (a *argoKubeClient) NewWorkflowServiceClient() workflowpkg.WorkflowServiceClient {
wfArchive := sqldb.NullWorkflowArchive
wfLister := store.NewKubeLister(a.wfClient)
return &errorTranslatingWorkflowServiceClient{&argoKubeWorkflowServiceClient{workflowserver.NewWorkflowServer(a.instanceIDService, argoKubeOffloadNodeStatusRepo, wfArchive, a.wfClient, wfLister, nil)}}
return &errorTranslatingWorkflowServiceClient{&argoKubeWorkflowServiceClient{workflowserver.NewWorkflowServer(a.instanceIDService, argoKubeOffloadNodeStatusRepo, wfArchive, a.wfClient, wfLister, nil, nil)}}
}

func (a *argoKubeClient) NewCronWorkflowServiceClient() (cronworkflow.CronWorkflowServiceClient, error) {
Expand Down
11 changes: 6 additions & 5 deletions server/apiserver/argoserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ func init() {
}
}

func getResourceCacheNamespace(opts ArgoServerOpts) string {
if opts.ManagedNamespace != "" {
return opts.ManagedNamespace
func getResourceCacheNamespace(managedNamespace string) string {
if managedNamespace != "" {
return managedNamespace
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
}
return v1.NamespaceAll
}
Expand All @@ -146,7 +146,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error
}
if ssoIf.IsRBACEnabled() {
// resourceCache is only used for SSO RBAC
resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, getResourceCacheNamespace(opts))
resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, getResourceCacheNamespace(opts.ManagedNamespace))
resourceCache.Run(ctx.Done())
}
log.Info("SSO enabled")
Expand Down Expand Up @@ -236,7 +236,8 @@ func (as *argoServer) Run(ctx context.Context, port int, browserOpenFunc func(st
if err != nil {
log.Fatal(err)
}
workflowServer := workflow.NewWorkflowServer(instanceIDService, offloadRepo, wfArchive, as.clients.Workflow, wfStore, wfStore)
resourceCacheNamespace := getResourceCacheNamespace(as.managedNamespace)
workflowServer := workflow.NewWorkflowServer(instanceIDService, offloadRepo, wfArchive, as.clients.Workflow, wfStore, wfStore, &resourceCacheNamespace)
grpcServer := as.newGRPCServer(instanceIDService, workflowServer, wfArchiveServer, eventServer, config.Links, config.Columns, config.NavColor)
httpServer := as.newHTTPServer(ctx, port, artifactServer)

Expand Down
8 changes: 4 additions & 4 deletions server/workflow/workflow_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,21 @@ type workflowServer struct {
var _ workflowpkg.WorkflowServiceServer = &workflowServer{}

// NewWorkflowServer returns a new WorkflowServer
func NewWorkflowServer(instanceIDService instanceid.Service, offloadNodeStatusRepo sqldb.OffloadNodeStatusRepo, wfArchive sqldb.WorkflowArchive, wfClientSet versioned.Interface, wfLister store.WorkflowLister, wfStore store.WorkflowStore) *workflowServer {
func NewWorkflowServer(instanceIDService instanceid.Service, offloadNodeStatusRepo sqldb.OffloadNodeStatusRepo, wfArchive sqldb.WorkflowArchive, wfClientSet versioned.Interface, wfLister store.WorkflowLister, wfStore store.WorkflowStore, namespace *string) *workflowServer {
ws := &workflowServer{
instanceIDService: instanceIDService,
offloadNodeStatusRepo: offloadNodeStatusRepo,
hydrator: hydrator.New(offloadNodeStatusRepo),
wfArchive: wfArchive,
wfLister: wfLister,
}
if wfStore != nil {
if wfStore != nil && namespace != nil {
lw := &cache.ListWatch{
ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
return wfClientSet.ArgoprojV1alpha1().Workflows(metav1.NamespaceAll).List(context.Background(), options)
return wfClientSet.ArgoprojV1alpha1().Workflows(*namespace).List(context.Background(), options)
},
WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
return wfClientSet.ArgoprojV1alpha1().Workflows(metav1.NamespaceAll).Watch(context.Background(), options)
return wfClientSet.ArgoprojV1alpha1().Workflows(*namespace).Watch(context.Background(), options)
},
}
wfReflector := cache.NewReflector(lw, &wfv1.Workflow{}, wfStore, reSyncDuration)
Expand Down
2 changes: 1 addition & 1 deletion server/workflow/workflow_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ func getWorkflowServer() (workflowpkg.WorkflowServiceServer, context.Context) {
if err = wfStore.Add(&wfObj5); err != nil {
panic(err)
}
server := NewWorkflowServer(instanceIdSvc, offloadNodeStatusRepo, archivedRepo, wfClientset, wfStore, wfStore)
server := NewWorkflowServer(instanceIdSvc, offloadNodeStatusRepo, archivedRepo, wfClientset, wfStore, wfStore, nil)
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this one previously correct with metav1.NamespaceAll?

With the new && namespace != nil check, this now takes a different path (but still passes apparently?)

Copy link
Member Author

@jiachengxu jiachengxu Jun 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh, I think I changed it by accident and I change it back to the metav1.NamespaceAll.

With the new && namespace != nil check, this now takes a different path (but still passes apparently?)

The value of the namespace doesn't matter regarding the test because we don't start the reflector to perform list and watch in the test and we just manually add objects to the store.

Copy link
Member

Choose a reason for hiding this comment

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

But the reflector is set since the store is not nil in this case, so it would start wouldn't it? It just isn't used actively in this test

Copy link
Member Author

Choose a reason for hiding this comment

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

the Run method is not called in the test, so the reflector is not started.

return server, ctx
}

Expand Down
Loading