Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -230,5 +230,5 @@ func NewLegacyCloudLoggingListLogTask(taskId taskid.TaskImplementationID[[]*log.
}

return []*log.Log{}, err
}, inspectioncore_contract.NewQueryTaskLabelOpt(logType, sampleQuery))
}, inspectioncore_contract.NewQueryTaskLabelOpt(logType, sampleQuery), coretask.WithLabelValue(RequestOptionalInputResourceNameTaskLabel, taskId.ReferenceIDString()))
}
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func NewListLogEntriesTask(taskSetting ListLogEntriesTaskSetting) coretask.Task[

return allLogs, nil
}, inspectioncore_contract.NewQueryTaskLabelOpt(description.DefaultLogType, description.ExampleQuery),
coretask.WithLabelValue(RequestOptionalInputResourceNameTaskLabel, taskID.ReferenceIDString()),
)
}

Expand Down Expand Up @@ -200,7 +201,7 @@ func setQueryInfo(ctx context.Context, taskID, baseLogFilter string, logFilterIn
}
finalFilter := fmt.Sprintf("%s\n%s", baseLogFilter, gcpqueryutil.TimeRangeQuerySection(startTime, endTime, true))
if len(finalFilter) > 20000 {
slog.WarnContext(ctx, fmt.Sprintf("Logging filter is exceeding Cloud Logging limitation 20000 charactors\n%s", finalFilter))
slog.WarnContext(ctx, fmt.Sprintf("Logging filter is exceeding Cloud Logging limitation 20000 characters\n%s", finalFilter))
}
queryInfo.SetQuery(taskID, logFilterName, finalFilter)
return nil
Expand Down
13 changes: 0 additions & 13 deletions pkg/task/inspection/googlecloudcommon/contract/resourcename.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,6 @@ func (r *ResourceNamesInput) GetResourceNamesForQuery(ctx context.Context, query
return queryNames
}

// GetQueryResourceNamePairs returns all query ID and resource name pairs.
func (r *ResourceNamesInput) GetQueryResourceNamePairs() []*QueryResourceNames {
queries := []*QueryResourceNames{}
for _, queryID := range r.resourceNames.Keys() {
resourceNames, found := typeddict.Get(r.resourceNames, queryID)
if !found {
continue
}
queries = append(queries, resourceNames)
}
return queries
}

func (r *ResourceNamesInput) ensureQueryID(queryID string) {
_, found := typeddict.Get(r.resourceNames, queryID)
if !found {
Expand Down
21 changes: 21 additions & 0 deletions pkg/task/inspection/googlecloudcommon/contract/tasklabel.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2025 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package googlecloudcommon_contract

import "github.com/GoogleCloudPlatform/khi/pkg/common/typedmap"

// RequestOptionalInputResourceNameTaskLabel is a label assigned to a task that requests the Cloud Logging resource name optionally.
// The value is the query ID.
var RequestOptionalInputResourceNameTaskLabel = typedmap.NewTypedKey[string]("request-optional-input-resource-name")
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/GoogleCloudPlatform/khi/pkg/common/typedmap"
inspectionmetadata "github.com/GoogleCloudPlatform/khi/pkg/core/inspection/metadata"
inspectiontaskbase "github.com/GoogleCloudPlatform/khi/pkg/core/inspection/taskbase"
coretask "github.com/GoogleCloudPlatform/khi/pkg/core/task"
"github.com/GoogleCloudPlatform/khi/pkg/core/task/taskid"
googlecloudcommon_contract "github.com/GoogleCloudPlatform/khi/pkg/task/inspection/googlecloudcommon/contract"
inspectioncore_contract "github.com/GoogleCloudPlatform/khi/pkg/task/inspection/inspectioncore/contract"
Expand All @@ -34,6 +35,10 @@ var resourceNamesInputKey = typedmap.NewTypedKey[*googlecloudcommon_contract.Res
// InputLoggingFilterResourceNameTask defines an inspection task that creates a form group
// for overriding log filter resource names for advanced users.
var InputLoggingFilterResourceNameTask = inspectiontaskbase.NewInspectionTask(googlecloudcommon_contract.InputLoggingFilterResourceNameTaskID, []taskid.UntypedTaskReference{}, func(ctx context.Context, taskMode inspectioncore_contract.InspectionTaskModeType) (*googlecloudcommon_contract.ResourceNamesInput, error) {
// Tasks requiring active resource names can change, so we always retrieve current tasks that need resource names from the task graph.
taskRunner := khictx.MustGetValue(ctx, inspectioncore_contract.TaskRunner)
currentActiveResourceNameInputRequests := getCurrentActiveQueryIDsForResourceName(taskRunner)
// Since the default resource names registered by the tasks actually used are not known until those tasks are executed, we store them in sharedMap and have the actual tasks update them.
sharedMap := khictx.MustGetValue(ctx, inspectioncore_contract.InspectionSharedMap)
resourceNamesInput := typedmap.GetOrSetFunc(sharedMap, resourceNamesInputKey, googlecloudcommon_contract.NewResourceNamesInput)

Expand All @@ -46,19 +51,20 @@ var InputLoggingFilterResourceNameTask = inspectiontaskbase.NewInspectionTask(go
requestInput := khictx.MustGetValue(ctx, inspectioncore_contract.InspectionTaskInput)

queryForms := []inspectionmetadata.ParameterFormField{}
for _, form := range resourceNamesInput.GetQueryResourceNamePairs() {
defaultValue := strings.Join(form.DefaultResourceNames, " ")
for _, request := range currentActiveResourceNameInputRequests {
queryInfo := resourceNamesInput.GetResourceNamesForQuery(ctx, request)
defaultValue := strings.Join(queryInfo.DefaultResourceNames, " ")
formFieldBase := inspectionmetadata.ParameterFormFieldBase{
Priority: 0,
ID: form.GetInputID(),
ID: queryInfo.GetInputID(),
Type: inspectionmetadata.Text,
Label: form.QueryID,
Label: queryInfo.QueryID,
Description: "",
HintType: inspectionmetadata.None,
Hint: "",
}
// This task validates the inputs only.
formInput, found := requestInput[form.GetInputID()]
formInput, found := requestInput[queryInfo.GetInputID()]
if found {
resourceNamesFromInput := strings.Split(formInput.(string), " ")
for i, resourceNameFromInput := range resourceNamesFromInput {
Expand All @@ -74,7 +80,7 @@ var InputLoggingFilterResourceNameTask = inspectiontaskbase.NewInspectionTask(go
queryForms = append(queryForms, &inspectionmetadata.TextParameterFormField{
ParameterFormFieldBase: formFieldBase,
Default: defaultValue,
Suggestions: form.DefaultResourceNames,
Suggestions: queryInfo.DefaultResourceNames,
ValidationTiming: inspectionmetadata.Change,
})
}
Expand All @@ -100,3 +106,17 @@ var InputLoggingFilterResourceNameTask = inspectiontaskbase.NewInspectionTask(go

return resourceNamesInput, nil
})

// getCurrentActiveQueryIDsForResourceName returns the query IDs that are currently active with retrieving them from the current task graph.
func getCurrentActiveQueryIDsForResourceName(runner coretask.TaskRunner) []string {
tasks := runner.Tasks()
result := []string{}
for _, t := range tasks {
requestInput, found := typedmap.Get(t.Labels(), googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel)
if !found {
continue
}
result = append(result, requestInput)
}
return result
Comment on lines +112 to +121

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation may return duplicate query IDs if multiple active tasks use the same ID. This could lead to duplicate form fields in the UI, which is confusing for the user.

To prevent this, it's better to use a map to collect unique query IDs. Sorting the result also ensures a deterministic order, which improves UI consistency and testability.

Please also add import "sort" to the file.

	tasks := runner.Tasks()
	queryIDSet := make(map[string]struct{})
	for _, t := range tasks {
		if requestInput, found := typedmap.Get(t.Labels(), googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel); found {
			queryIDSet[requestInput] = struct{}{}
		}
	}
	result := make([]string, 0, len(queryIDSet))
	for queryID := range queryIDSet {
		result = append(result, queryID)
	}
	sort.Strings(result)
	return result

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,68 @@
package googlecloudcommon_impl

import (
"context"
"testing"

"github.com/GoogleCloudPlatform/khi/pkg/common/khictx"
"github.com/GoogleCloudPlatform/khi/pkg/common/typedmap"
inspectionmetadata "github.com/GoogleCloudPlatform/khi/pkg/core/inspection/metadata"
inspectiontest "github.com/GoogleCloudPlatform/khi/pkg/core/inspection/test"
coretask "github.com/GoogleCloudPlatform/khi/pkg/core/task"
"github.com/GoogleCloudPlatform/khi/pkg/core/task/taskid"
googlecloudcommon_contract "github.com/GoogleCloudPlatform/khi/pkg/task/inspection/googlecloudcommon/contract"
inspectioncore_contract "github.com/GoogleCloudPlatform/khi/pkg/task/inspection/inspectioncore/contract"
"github.com/google/go-cmp/cmp"
)

type mockTaskRunner struct {
tasks []coretask.UntypedTask
}

// AddInterceptor implements coretask.TaskRunner.
func (m *mockTaskRunner) AddInterceptor(interceptor coretask.Interceptor) {
panic("unimplemented")
}

// Result implements coretask.TaskRunner.
func (m *mockTaskRunner) Result() (*typedmap.ReadonlyTypedMap, error) {
panic("unimplemented")
}

// Run implements coretask.TaskRunner.
func (m *mockTaskRunner) Run(ctx context.Context) error {
panic("unimplemented")
}

// Wait implements coretask.TaskRunner.
func (m *mockTaskRunner) Wait() <-chan interface{} {
panic("unimplemented")
}

func (m *mockTaskRunner) Tasks() []coretask.UntypedTask {
return m.tasks
}

var _ coretask.TaskRunner = (*mockTaskRunner)(nil)

func TestInputLoggingFilterResourceNameTask(t *testing.T) {
defaultNames := []string{"projects/foo"}
t1 := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t1"), nil, nil, coretask.WithLabelValue(
googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test",
))
nonRelatedTask := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("not-related"), nil, nil)
testCases := []struct {
desc string
taskMode inspectioncore_contract.InspectionTaskModeType
inputValue string
tasks []coretask.UntypedTask
wantForm inspectionmetadata.GroupParameterFormField
}{
{
desc: "basic input",
taskMode: inspectioncore_contract.TaskModeDryRun,
inputValue: "projects/foo",
tasks: []coretask.UntypedTask{t1, nonRelatedTask},
wantForm: inspectionmetadata.GroupParameterFormField{
ParameterFormFieldBase: inspectionmetadata.ParameterFormFieldBase{
Priority: -1000000,
Expand Down Expand Up @@ -68,6 +108,7 @@ func TestInputLoggingFilterResourceNameTask(t *testing.T) {
desc: "invalid input",
taskMode: inspectioncore_contract.TaskModeDryRun,
inputValue: "invalid-resource-name",
tasks: []coretask.UntypedTask{t1, nonRelatedTask},
wantForm: inspectionmetadata.GroupParameterFormField{
ParameterFormFieldBase: inspectionmetadata.ParameterFormFieldBase{
Priority: -1000000,
Expand Down Expand Up @@ -100,6 +141,7 @@ func TestInputLoggingFilterResourceNameTask(t *testing.T) {
desc: "basic input for run mode",
taskMode: inspectioncore_contract.TaskModeRun,
inputValue: "projects/foo",
tasks: []coretask.UntypedTask{t1, nonRelatedTask},
wantForm: inspectionmetadata.GroupParameterFormField{
ParameterFormFieldBase: inspectionmetadata.ParameterFormFieldBase{
Priority: -1000000,
Expand Down Expand Up @@ -127,10 +169,31 @@ func TestInputLoggingFilterResourceNameTask(t *testing.T) {
CollapsedByDefault: true,
},
},
{
desc: "shouldn't populate inputs when the task requesting the resource name wasn't included in the graph even it already has the default value updated",
taskMode: inspectioncore_contract.TaskModeRun,
inputValue: "projects/foo",
tasks: []coretask.UntypedTask{nonRelatedTask},
wantForm: inspectionmetadata.GroupParameterFormField{
ParameterFormFieldBase: inspectionmetadata.ParameterFormFieldBase{
Priority: -1000000,
ID: googlecloudcommon_contract.InputLoggingFilterResourceNameTaskID.ReferenceIDString(),
Type: inspectionmetadata.Group,
Label: "Logging filter resource names (advanced)",
Description: "Override these parameters when your logs are not on the same project of the cluster, or customize the log filter target resources.",
HintType: inspectionmetadata.None,
Hint: "",
},
Children: []inspectionmetadata.ParameterFormField{},
Collapsible: true,
CollapsedByDefault: true,
},
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
ctx := inspectiontest.WithDefaultTestInspectionTaskContext(t.Context())
ctx = khictx.WithValue[coretask.TaskRunner](ctx, inspectioncore_contract.TaskRunner, &mockTaskRunner{tasks: tc.tasks})
resourceNames, _, err := inspectiontest.RunInspectionTask(ctx, InputLoggingFilterResourceNameTask, inspectioncore_contract.TaskModeDryRun, map[string]any{})
if err != nil {
t.Fatalf("Failed to call InputLoggingFilterResourceNameTask at 1st time:%v", err)
Expand All @@ -139,6 +202,7 @@ func TestInputLoggingFilterResourceNameTask(t *testing.T) {
QueryID: "test",
}
newCtx := inspectiontest.NextRunTaskContext(t.Context(), ctx)
newCtx = khictx.WithValue[coretask.TaskRunner](newCtx, inspectioncore_contract.TaskRunner, &mockTaskRunner{tasks: tc.tasks})
resourceNames.UpdateDefaultResourceNamesForQuery("test", defaultNames)
_, metadata, err := inspectiontest.RunInspectionTask(newCtx, InputLoggingFilterResourceNameTask, tc.taskMode, map[string]any{
resourceName.GetInputID(): tc.inputValue,
Expand All @@ -157,3 +221,40 @@ func TestInputLoggingFilterResourceNameTask(t *testing.T) {
})
}
}

func TestGetCurrentActiveQueryIDsForResourceName(t *testing.T) {
t1 := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t1"), nil, nil, coretask.WithLabelValue(
googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test1",
))
t2 := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t2"), nil, nil, coretask.WithLabelValue(
googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test2",
))
nonRelatedTask := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("not-related"), nil, nil)
testCases := []struct {
desc string
tasks []coretask.UntypedTask
want []string
}{
{
desc: "with mixed tasks",
tasks: []coretask.UntypedTask{t1, t2, nonRelatedTask},
want: []string{
"test1",
"test2",
},
},
{
desc: "with no tasks",
tasks: []coretask.UntypedTask{},
want: []string{},
},
}
Comment on lines +226 to +251

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

It's good practice to also test the case where multiple tasks might have the same query ID. This would highlight the need for deduplication in getCurrentActiveQueryIDsForResourceName and verify the fix is working as expected.

	t1 := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t1"), nil, nil, coretask.WithLabelValue(
		googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test1",
	))
	t2 := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t2"), nil, nil, coretask.WithLabelValue(
		googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test2",
	))
	t1Dup := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("t1-dup"), nil, nil, coretask.WithLabelValue(
		googlecloudcommon_contract.RequestOptionalInputResourceNameTaskLabel, "test1",
	))
	nonRelatedTask := coretask.NewTask(taskid.NewDefaultImplementationID[struct{}]("not-related"), nil, nil)
	testCases := []struct {
		desc  string
		tasks []coretask.UntypedTask
		want  []string
	}{
		{
			desc:  "with mixed tasks",
			tasks: []coretask.UntypedTask{t1, t2, nonRelatedTask},
			want: []string{
				"test1",
				"test2",
			},
		},
		{
			desc:  "with duplicate query IDs",
			tasks: []coretask.UntypedTask{t1, t2, t1Dup, nonRelatedTask},
			want: []string{
				"test1",
				"test2",
			},
		},
		{
			desc:  "with no tasks",
			tasks: []coretask.UntypedTask{},
			want:  []string{},
		},
	}

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
got := getCurrentActiveQueryIDsForResourceName(&mockTaskRunner{tasks: tc.tasks})
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("getResourceNameInputRequests mismatch (-want,+got):\n%s", diff)
}
})
}
}
Loading