feat: add !=, == support for namespace field selector #15098
Conversation
miltalex
force-pushed
the
fix/add-namespace-operators
branch
3 times, most recently
from
8874b3c to
d79bf8d
Compare
miltalex
force-pushed
the
fix/add-namespace-operators
branch
from
d79bf8d to
25bd9d2
Compare
Signed-off-by: Miltiadis Alexis <[email protected]>
miltalex
force-pushed
the
fix/add-namespace-operators
branch
from
25bd9d2 to
03388db
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughImplements support for namespace field selector operators ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/workflow/workflow_server.go (1)
191-201: The error message may be confusing forNotEqualsnamespace filter.When
NamespaceFilter == "NotEquals", thetargetNamespacebecomes an empty string. The error message at line 201 will then display:Permission denied, you are not allowed to list workflows in namespace "". Maybe you want to specify a namespace with query parameter `.namespace=`?This message is confusing because the user did specify a namespace (with
!=), and they need cluster-wide permissions, not a specific namespace. Consider providing a more descriptive message for this case:if !allowed { + if options.NamespaceFilter == "NotEquals" { + return nil, status.Error(codes.PermissionDenied, "Permission denied, listing workflows with namespace exclusion (!=) requires cluster-wide list permissions") + } return nil, status.Error(codes.PermissionDenied, fmt.Sprintf("Permission denied, you are not allowed to list workflows in namespace \"%s\". Maybe you want to specify a namespace with query parameter `.namespace=%s`?", targetNamespace, targetNamespace)) }persist/sqldb/selector.go (1)
13-17: LGTM on the conditional namespace filtering logic.The implementation correctly branches on
NamespaceFilter == "NotEquals"to generate eithernamespace != ?ornamespace = ?SQL conditions. The logic is consistent betweenBuildArchivedWorkflowSelector(lines 13-17) andBuildWorkflowSelector(lines 67-71), and both helper functions (namespaceNotEqualandnamespaceEqual) are properly implemented inworkflow_archive.go.Consider defining a constant for
"NotEquals"to avoid the magic string that appears throughout the codebase:const NamespaceFilterNotEquals = "NotEquals"persist/sqldb/workflow_archive.go (1)
291-297: Suggest using a constant for the "NotEquals" filter value.The conditional logic correctly implements namespace filtering based on
NamespaceFilter. However, the string literal"NotEquals"appears multiple times across this file (lines 293, 320, 355, 382, 419, 446) and likely in other files as well.Consider defining a constant to improve maintainability and prevent typos:
const ( FilterNotEquals = "NotEquals" FilterExact = "Exact" FilterContains = "Contains" FilterPrefix = "Prefix" )Then use it consistently:
-if options.NamespaceFilter == "NotEquals" { +if options.NamespaceFilter == FilterNotEquals {This pattern should be applied across all filter comparisons in the codebase for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.features/pending/namespace-field-selectors-operators.md(1 hunks)persist/sqldb/selector.go(2 hunks)persist/sqldb/workflow_archive.go(4 hunks)server/utils/list_options.go(4 hunks)server/utils/list_options_test.go(1 hunks)server/workflow/workflow_server.go(1 hunks)server/workflowarchive/archived_workflow_server_test.go(2 hunks)test/e2e/argo_server_test.go(3 hunks)test/e2e/testdata/argo-server-test-clusterrole.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
persist/sqldb/workflow_archive.go (1)
test/e2e/fixtures/e2e_suite.go (1)
Namespace(42-42)
persist/sqldb/selector.go (1)
test/e2e/fixtures/e2e_suite.go (1)
Namespace(42-42)
test/e2e/argo_server_test.go (1)
test/e2e/fixtures/util.go (1)
LoadObject(65-84)
server/utils/list_options_test.go (1)
server/utils/list_options.go (1)
ListOptions(15-25)
server/workflowarchive/archived_workflow_server_test.go (2)
server/utils/list_options.go (1)
ListOptions(15-25)pkg/apiclient/workflowarchive/workflow-archive.pb.go (3)
ListArchivedWorkflowsRequest(32-41)ListArchivedWorkflowsRequest(45-45)ListArchivedWorkflowsRequest(46-48)
🪛 LanguageTool
.features/pending/namespace-field-selectors-operators.md
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 561 characters long)
Context: ...eries. For example, you can filter with namespace!=kube-system to exclude system namespac...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (12)
server/utils/list_options.go (2)
101-114: The!=operator unconditionally overwrites namespace without conflict detection.When
metadata.namespace!=is parsed, it setsnamespaceandnamespaceFilterwithout checking for conflicts with an existingnsquery parameter. This is asymmetric with the==and=handlers which perform conflict checks.For example, if a user specifies
?namespace=foo&listOptions.fieldSelector=metadata.namespace!=bar, thenamespacewill be silently overwritten tobarwithNotEqualsfilter, which may not be the intended behavior.Consider whether conflict detection should also apply here, or if this behavior is intentional (e.g.,
!=always takes precedence).
18-18: LGTM on the NamespaceFilter field addition and propagation.The new
NamespaceFilterfield is properly declared in the struct, initialized in the function, and correctly propagated to the returnedListOptions.Also applies to: 77-77, 166-166
test/e2e/testdata/argo-server-test-clusterrole.yaml (1)
1-16: LGTM on the ClusterRole definition.The ClusterRole correctly defines cluster-scoped permissions for Argo workflows, enabling the e2e tests to validate that users with cluster-wide
listpermissions can use themetadata.namespace!=filter.server/utils/list_options_test.go (1)
133-193: LGTM on the new namespace field selector test cases.The test cases comprehensively cover:
metadata.namespace!=(NotEquals) handlingmetadata.namespace==(Equals) handling- Conflict detection between
!=and==in different orders- Conflict detection between field selectors and
nsquery parameter- Valid matching scenarios
The inline comments clearly document the expected behavior for each case.
test/e2e/argo_server_test.go (3)
511-560: LGTM on the cluster-scoped RBAC setup for e2e tests.The test setup correctly:
- Creates a cluster-scoped ServiceAccount
- Loads and creates the ClusterRole from testdata
- Creates a ClusterRoleBinding linking the SA to the ClusterRole
- Retrieves the SA token for use in subsequent tests
- Properly cleans up resources with deferred deletions
608-652: Well-structured test cases for namespace field selector operators.The tests effectively validate:
metadata.namespace!=argoreturns empty results when excluding the only namespace with workflowsmetadata.namespace!=argo-excludedreturns workflows fromargonamespace (exclusion doesn't match)metadata.namespace==argocorrectly matches and returns workflowsThe token swapping pattern with deferred restoration is consistent with the existing test patterns.
687-692: LGTM on the authorization test forNotEqualswith insufficient permissions.This test correctly validates that using
metadata.namespace!=without cluster-wide permissions results in a 403 Forbidden response, enforcing the security requirement that namespace exclusion queries require elevated permissions.server/workflowarchive/archived_workflow_server_test.go (2)
71-72: LGTM! Mock expectations align with the new namespace filtering feature.The mock expectations correctly set up test data for the namespace NotEquals filter (line 71) and regular namespace filtering (line 72). These expectations match the test cases added later in the file.
203-213: LGTM! Test coverage validates namespace operator parsing and propagation.The test cases verify that both
!=and==operators are correctly parsed from the field selector and propagate through to the repository layer. The assertions confirm expected behavior for item counts and pagination.persist/sqldb/workflow_archive.go (3)
513-518: LGTM! Helper function follows established patterns.The
namespaceNotEqualfunction correctly mirrors the existingnameNotEqualhelper (lines 527-532), maintaining consistency in the codebase. The conditional logic ensures that an empty namespace doesn't add a filter clause.
353-359: LGTM! Consistent implementation across counting methods.The namespace filtering logic matches the pattern used in
CountWorkflows(lines 291-297), ensuring consistent behavior. The conditional correctly appliesnamespaceNotEqualwhen the filter is set to"NotEquals".
417-423: LGTM! Consistent namespace filtering across all query methods.The namespace filtering logic is correctly applied in
HasMoreWorkflows, maintaining consistency withCountWorkflows(lines 291-297) andcountWorkflowsOptimized(lines 353-359). This ensures uniform behavior across all workflow listing and counting operations.
| You can now use the `!=` and `==` operators when filtering workflows by namespace field. | ||
| This provides more flexible query capabilities, allowing you to easily exclude specific namespaces or match exact namespace values in your workflow queries. | ||
| For example, you can filter with `namespace!=kube-system` to exclude system namespaces or `namespace==production` to target only production environments. |
There was a problem hiding this comment.
Field selector syntax in examples is inconsistent with the implementation.
The examples use namespace!=kube-system and namespace==production, but the actual field selector syntax requires the full path: metadata.namespace!=kube-system and metadata.namespace==production.
-For example, you can filter with `namespace!=kube-system` to exclude system namespaces or `namespace==production` to target only production environments.
+For example, you can filter with `metadata.namespace!=kube-system` to exclude system namespaces or `metadata.namespace==production` to target only production environments.🧰 Tools
🪛 LanguageTool
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 561 characters long)
Context: ...eries. For example, you can filter with namespace!=kube-system to exclude system namespac...
(EN_EXCESSIVE_EXCLAMATION)
🤖 Prompt for AI Agents
In .features/pending/namespace-field-selectors-operators.md around lines 6 to 8,
the examples use shorthand field selectors like `namespace!=kube-system` and
`namespace==production` which are inconsistent with the implementation that
expects the full field path; update the examples to use the full selector path
`metadata.namespace!=kube-system` and `metadata.namespace==production` (and any
other occurrences in this file) so they match the actual field selector syntax
used by the code.
2 participants
Fixes #13468
Motivation
Currently, argo-workflows only supports the '=' operator for the field selector
metadata.namespacewhen listing workflows. This limitation restricts the flexibility of our queries, particularly when users wish to exclude specific namespaces from their results. The goal of this PR is to expand support to include the '==' and '!=' operators formetadata.namespace, aligning our capabilities with native Kubernetes functionality. This enhancement allows users to perform exclusion queries (e.g., "list all workflows except those in the 'test' namespace"), improving overall system usability.Modifications
ListOptions Expansion:
ListOptionsstruct inserver/utilsto include a newNamespaceFilterfield to store the operator type.BuildListOptions Enhancement:
BuildListOptionsfunction to parsemetadata.namespace==andmetadata.namespace!=.!=sets the correct filter type and==behaves consistent with=.Database Selector Update:
BuildArchivedWorkflowSelectorandBuildWorkflowSelectorinpersist/sqldbto handle theNamespaceFilter.namespace != ?) to support theNotEqualsfilter in the archive.Workflow Server Authorization:
ListWorkflowsinserver/workflow/workflow_server.goto handle authorization for namespace exclusion.metadata.namespace!=is used, the user must have cluster-wide list permissions. If they do not, the request is denied immediately. This mirrorskubectlbehavior and prevents potential security issues where excluding a namespace might imply access to all others.Verification
Unit Testing:
server/utils/list_options_test.goto cover parsing of the new operators and conflict detection.server/workflowarchive/archived_workflow_server_test.goto verify that the new options are correctly passed to the repository layer.E2E Testing:
test/e2e/argo_server_test.go.argo list --field-selector metadata.namespace!=<ns>returns the correct workflows for authorized users.Example audit log from local testing:
{ "kind": "Event", "apiVersion": "audit.k8s.io/v1", "level": "Metadata", "auditID": "f8a139b4-9201-4dee-b57b-15e095887f79", "stage": "ResponseComplete", "requestURI": "/apis/argoproj.io/v1alpha1/workflows?fieldSelector=metadata.namespace%21%3Dargo-test-2\u0026labelSelector=%21workflows.argoproj.io%2Fcontroller-instanceid", "verb": "list", "user": { "username": "system:admin", "groups": [ "system:masters", "system:authenticated" ] }, "sourceIPs": [ "192.168.107.3" ], "userAgent": "argo/v0.0.0 (darwin/amd64) kubernetes/$Format/argo-workflows/latest+6fb2b9f.dirty argo-api-client", "objectRef": { "resource": "workflows", "apiGroup": "argoproj.io", "apiVersion": "v1alpha1" }, "responseStatus": { "metadata": {}, "code": 200 }, "requestReceivedTimestamp": "2025-12-03T10:34:35.475997Z", "stageTimestamp": "2025-12-03T10:34:35.483591Z", "annotations": { "authorization.k8s.io/decision": "allow", "authorization.k8s.io/reason": "" } }Summary by CodeRabbit
New Features
!=for excluding namespaces and==for exact namespace matching. Users can now filter workflows using expressions likenamespace!=kube-systemornamespace==production.Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.