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.

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

feat(server): Add maxPodLogsToRender setting (#9131) #14617

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions docs/operator-manual/argocd-cm.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ data:
# cluster.inClusterEnabled indicates whether to allow in-cluster server address. This is enabled by default.
cluster.inClusterEnabled: "true"

# The maximum number of pod logs to render in UI. If the application has more than this number of pods, the logs will not be rendered.
# This is to prevent the UI from becoming unresponsive when rendering a large number of logs. Default is 10.
server.maxPodLogsToRender: 10

# Application pod logs RBAC enforcement enables control over who can and who can't view application pod logs.
# When you enable the switch, pod logs will be visible only to admin role by default. Other roles/users will not be able to view them via cli and UI.
# When you enable the switch, viewing pod logs for other roles/users will require explicit RBAC allow policies (allow get on logs subresource).
Expand Down
10 changes: 7 additions & 3 deletions server/application/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ import (
type AppResourceTreeFn func(ctx context.Context, app *appv1.Application) (*appv1.ApplicationTree, error)

const (
maxPodLogsToRender = 10
backgroundPropagationPolicy string = "background"
foregroundPropagationPolicy string = "foreground"
)
Expand Down Expand Up @@ -1579,8 +1578,13 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application.
return nil
}

if len(pods) > maxPodLogsToRender {
return errors.New("Max pods to view logs are reached. Please provide more granular query.")
Copy link
Member

Choose a reason for hiding this comment

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

this is the root error that ends up in the argocd throwing 500 error responses.

while I agree having a configurable limit may mitigate this to some extent, can we also solve the generic case and treat this error differently?

a 4xx response may be more fitted.

Copy link
Contributor Author

@lukaszgyg lukaszgyg Aug 29, 2023

Choose a reason for hiding this comment

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

As specifying the log query will fix the problem I took the codes.InvalidArgument which will be transformed to http.StatusBadRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the variable should be removed here also?

maxPodLogsToRender = 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Jellyfrog for a good catch! I removed unused const.

maxPodLogsToRender, err := s.settingsMgr.GetMaxPodLogsToRender()
if err != nil {
return fmt.Errorf("error getting MaxPodLogsToRender config: %w", err)
}

if int64(len(pods)) > maxPodLogsToRender {
return status.Error(codes.InvalidArgument, "max pods to view logs are reached. Please provide more granular query")
}

var streams []chan logEntry
Expand Down
111 changes: 107 additions & 4 deletions server/application/application_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@ func newTestAppServer(t *testing.T, objects ...runtime.Object) *Server {
_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enf.SetDefaultRole("role:admin")
}
return newTestAppServerWithEnforcerConfigure(f, t, objects...)
return newTestAppServerWithEnforcerConfigure(f, t, map[string]string{}, objects...)
}

func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T, objects ...runtime.Object) *Server {
func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T, additionalConfig map[string]string, objects ...runtime.Object) *Server {
kubeclientset := fake.NewSimpleClientset(&v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Expand All @@ -144,6 +144,7 @@ func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T,
"app.kubernetes.io/part-of": "argocd",
},
},
Data: additionalConfig,
}, &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "argocd-secret",
Expand Down Expand Up @@ -752,7 +753,7 @@ func TestNoAppEnumeration(t *testing.T) {
}
})
testDeployment := kube.MustToUnstructured(&deployment)
appServer := newTestAppServerWithEnforcerConfigure(f, t, testApp, testHelmApp, testDeployment)
appServer := newTestAppServerWithEnforcerConfigure(f, t, map[string]string{}, testApp, testHelmApp, testDeployment)

noRoleCtx := context.Background()
// nolint:staticcheck
Expand Down Expand Up @@ -1272,7 +1273,7 @@ g, group-49, role:test3
`
_ = enf.SetUserPolicy(policy)
}
appServer := newTestAppServerWithEnforcerConfigure(f, t, objects...)
appServer := newTestAppServerWithEnforcerConfigure(f, t, map[string]string{}, objects...)

res, err := appServer.List(ctx, &application.ApplicationQuery{})

Expand Down Expand Up @@ -1987,6 +1988,108 @@ func TestLogsGetSelectedPod(t *testing.T) {
})
}

func TestMaxPodLogsRender(t *testing.T) {
crenshaw-dev marked this conversation as resolved.
Show resolved Hide resolved

defaultMaxPodLogsToRender, _ := newTestAppServer(t).settingsMgr.GetMaxPodLogsToRender()

// Case: number of pods to view logs is less than defaultMaxPodLogsToRender
podNumber := int(defaultMaxPodLogsToRender - 1)
appServer, adminCtx := createAppServerWithMaxLodLogs(t, podNumber)

t.Run("PodLogs", func(t *testing.T) {
err := appServer.PodLogs(&application.ApplicationPodLogsQuery{Name: pointer.String("test")}, &TestPodLogsServer{ctx: adminCtx})
statusCode, _ := status.FromError(err)
assert.Equal(t, codes.OK, statusCode.Code())
})

// Case: number of pods higher than defaultMaxPodLogsToRender
podNumber = int(defaultMaxPodLogsToRender + 1)
appServer, adminCtx = createAppServerWithMaxLodLogs(t, podNumber)

t.Run("PodLogs", func(t *testing.T) {
err := appServer.PodLogs(&application.ApplicationPodLogsQuery{Name: pointer.String("test")}, &TestPodLogsServer{ctx: adminCtx})
assert.NotNil(t, err)
statusCode, _ := status.FromError(err)
assert.Equal(t, codes.InvalidArgument, statusCode.Code())
assert.Equal(t, "rpc error: code = InvalidArgument desc = max pods to view logs are reached. Please provide more granular query", err.Error())
})

// Case: number of pods to view logs is less than customMaxPodLogsToRender
customMaxPodLogsToRender := int64(15)
podNumber = int(customMaxPodLogsToRender - 1)
appServer, adminCtx = createAppServerWithMaxLodLogs(t, podNumber, customMaxPodLogsToRender)

t.Run("PodLogs", func(t *testing.T) {
err := appServer.PodLogs(&application.ApplicationPodLogsQuery{Name: pointer.String("test")}, &TestPodLogsServer{ctx: adminCtx})
statusCode, _ := status.FromError(err)
assert.Equal(t, codes.OK, statusCode.Code())
})

// Case: number of pods higher than customMaxPodLogsToRender
customMaxPodLogsToRender = int64(15)
podNumber = int(customMaxPodLogsToRender + 1)
appServer, adminCtx = createAppServerWithMaxLodLogs(t, podNumber, customMaxPodLogsToRender)

t.Run("PodLogs", func(t *testing.T) {
err := appServer.PodLogs(&application.ApplicationPodLogsQuery{Name: pointer.String("test")}, &TestPodLogsServer{ctx: adminCtx})
assert.NotNil(t, err)
statusCode, _ := status.FromError(err)
assert.Equal(t, codes.InvalidArgument, statusCode.Code())
assert.Equal(t, "rpc error: code = InvalidArgument desc = max pods to view logs are reached. Please provide more granular query", err.Error())
})
}

// createAppServerWithMaxLodLogs creates a new app server with given number of pods and resources
func createAppServerWithMaxLodLogs(t *testing.T, podNumber int, maxPodLogsToRender ...int64) (*Server, context.Context) {
runtimeObjects := make([]runtime.Object, podNumber+1)
resources := make([]appsv1.ResourceStatus, podNumber)

for i := 0; i < podNumber; i++ {
pod := v1.Pod{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Pod",
},
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("pod-%d", i),
Namespace: "test",
},
}
resources[i] = appsv1.ResourceStatus{
Group: pod.GroupVersionKind().Group,
Kind: pod.GroupVersionKind().Kind,
Version: pod.GroupVersionKind().Version,
Name: pod.Name,
Namespace: pod.Namespace,
Status: "Synced",
}
runtimeObjects[i] = kube.MustToUnstructured(&pod)
}

testApp := newTestApp(func(app *appsv1.Application) {
app.Name = "test"
app.Status.Resources = resources
})
runtimeObjects[podNumber] = testApp

noRoleCtx := context.Background()
// nolint:staticcheck
adminCtx := context.WithValue(noRoleCtx, "claims", &jwt.MapClaims{"groups": []string{"admin"}})

if len(maxPodLogsToRender) > 0 {
f := func(enf *rbac.Enforcer) {
_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enf.SetDefaultRole("role:admin")
}
formatInt := strconv.FormatInt(maxPodLogsToRender[0], 10)
appServer := newTestAppServerWithEnforcerConfigure(f, t, map[string]string{"server.maxPodLogsToRender": formatInt}, runtimeObjects...)
return appServer, adminCtx
} else {
appServer := newTestAppServer(t, runtimeObjects...)
return appServer, adminCtx
}
}

// refreshAnnotationRemover runs an infinite loop until it detects and removes refresh annotation or given context is done
func refreshAnnotationRemover(t *testing.T, ctx context.Context, patched *int32, appServer *Server, appName string, ch chan string) {
for ctx.Err() == nil {
Expand Down
24 changes: 24 additions & 0 deletions util/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ type ArgoCDSettings struct {
InClusterEnabled bool `json:"inClusterEnabled"`
// ServerRBACLogEnforceEnable temporary var indicates whether rbac will be enforced on logs
ServerRBACLogEnforceEnable bool `json:"serverRBACLogEnforceEnable"`
// MaxPodLogsToRender the maximum number of pod logs to render
MaxPodLogsToRender int64 `json:"maxPodLogsToRender"`
// ExecEnabled indicates whether the UI exec feature is enabled
ExecEnabled bool `json:"execEnabled"`
// ExecShells restricts which shells are allowed for `exec` and in which order they are tried
Expand Down Expand Up @@ -485,6 +487,8 @@ const (
inClusterEnabledKey = "cluster.inClusterEnabled"
// settingsServerRBACLogEnforceEnable is the key to configure whether logs RBAC enforcement is enabled
settingsServerRBACLogEnforceEnableKey = "server.rbac.log.enforce.enable"
// MaxPodLogsToRender the maximum number of pod logs to render
settingsMaxPodLogsToRender = "server.maxPodLogsToRender"
// helmValuesFileSchemesKey is the key to configure the list of supported helm values file schemas
helmValuesFileSchemesKey = "helm.valuesFileSchemes"
// execEnabledKey is the key to configure whether the UI exec feature is enabled
Expand Down Expand Up @@ -788,6 +792,19 @@ func (mgr *SettingsManager) GetServerRBACLogEnforceEnable() (bool, error) {
return strconv.ParseBool(argoCDCM.Data[settingsServerRBACLogEnforceEnableKey])
}

func (mgr *SettingsManager) GetMaxPodLogsToRender() (int64, error) {
argoCDCM, err := mgr.getConfigMap()
if err != nil {
return 10, err
}

if argoCDCM.Data[settingsMaxPodLogsToRender] == "" {
return 10, nil
}

return strconv.ParseInt(argoCDCM.Data[settingsMaxPodLogsToRender], 10, 64)
}

func (mgr *SettingsManager) GetDeepLinks(deeplinkType string) ([]DeepLink, error) {
argoCDCM, err := mgr.getConfigMap()
if err != nil {
Expand Down Expand Up @@ -1457,6 +1474,13 @@ func updateSettingsFromConfigMap(settings *ArgoCDSettings, argoCDCM *apiv1.Confi
if settings.PasswordPattern == "" {
settings.PasswordPattern = common.PasswordPatten
}
if maxPodLogsToRenderStr, ok := argoCDCM.Data[settingsMaxPodLogsToRender]; ok {
if val, err := strconv.ParseInt(maxPodLogsToRenderStr, 10, 64); err != nil {
log.Warnf("Failed to parse '%s' key: %v", settingsMaxPodLogsToRender, err)
} else {
settings.MaxPodLogsToRender = val
}
}
settings.InClusterEnabled = argoCDCM.Data[inClusterEnabledKey] != "false"
settings.ExecEnabled = argoCDCM.Data[execEnabledKey] == "true"
execShells := argoCDCM.Data[execShellsKey]
Expand Down
Loading