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: support LabelSelector type for NamespaceSelectors #2494

Merged
merged 4 commits into from
Jan 26, 2024

Conversation

deszhou
Copy link
Contributor

@deszhou deszhou commented Jan 24, 2024

What type of PR is this?

What this PR does / why we need it:

In the current configuration, the namespaceSelectors field under EnvoyGateway.Provider.Kubernetes.Wach only supports label selection using the key, similar to the Exists operator in LabelSelectorOperator.

The namespaceSelectors field can support a more comprehensive label-based selection, allowing you to specify both matchLabels and matchExpressions

namespaceSelectors:
  matchLabels:
    component: redis
  matchExpressions:
    - { key: tier, operator: In, values: [cache] }
    - { key: environment, operator: NotIn, values: [dev] }

https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#LabelSelector

Which issue(s) this PR fixes:

Fixes #2481

@deszhou deszhou requested a review from a team as a code owner January 24, 2024 07:53
@deszhou deszhou closed this Jan 24, 2024
// NamespaceSelectors holds the label selector used to dynamically select namespaces.
// Envoy Gateway will watch for namespaces matching the specified label selector.
// Precisely one of Namespaces and NamespaceSelectors must be set.
NamespaceSelectors *metav1.LabelSelector `json:"namespaceSelectors,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

should this field be renamed to namespaceSelector ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done

@deszhou deszhou reopened this Jan 25, 2024
@deszhou deszhou force-pushed the namespaceSelectors branch 4 times, most recently from 2fb447c to 2a574f0 Compare January 25, 2024 09:50
@deszhou deszhou force-pushed the namespaceSelectors branch from 2a574f0 to bff6416 Compare January 25, 2024 11:10
@zirain
Copy link
Contributor

zirain commented Jan 25, 2024

EG use squash merge model, please kindly don't force push everytime for better review.

@deszhou
Copy link
Contributor Author

deszhou commented Jan 25, 2024

EG use squash merge model, please kindly don't force push everytime for better review.

I will pay attention to this. Can you help me look at the coverage-test? thank

@zirain
Copy link
Contributor

zirain commented Jan 25, 2024

--- FAIL: TestNamespaceSelectorProvider (67.82s)
    kubernetes_test.go:1372: 
        	Error Trace:	/home/runner/work/gateway/gateway/internal/provider/kubernetes/kubernetes_test.go:1372
        	Error:      	Condition never satisfied
        	Test:       	TestNamespaceSelectorProvider
2024-01-25T11:14:20.404Z	INFO	kubernetes/predicates.go:39	gatewayclass has matching controller name, processing	{"name": "test-gc"}
2024-01-25T11:14:20.404Z	INFO	kubernetes/predicates.go:43	bypassing reconciliation due to controller name	{"controller": "not.configured/controller"}
2024-01-25T11:14:20.406Z	DPANIC	kubernetes/predicates.go:123	non-string key argument passed to logging, ignoring all later arguments	{"invalid key": "not.configured/controller"}
2024-01-25T11:14:20.406Z	INFO	kubernetes/predicates.go:123	gatewayclass controller name
2024-01-25T11:14:20.406Z	INFO	kubernetes/predicates.go:124	gatewayclass name for gateway doesn't match configured name	{"namespace": "", "name": "scheduled-status-test"}
2024-01-25T11:14:20.407Z	DPANIC	kubernetes/predicates.go:123	non-string key argument passed to logging, ignoring all later arguments	{"invalid key": "not.configured/controller"}
2024-01-25T11:14:20.407Z	INFO	kubernetes/predicates.go:123	gatewayclass controller name
2024-01-25T11:14:20.407Z	INFO	kubernetes/predicates.go:124	gatewayclass name for gateway doesn't match configured name	{"namespace": "", "name": "scheduled-status-test"}
2024-01-25T11:14:20.417Z	INFO	kubernetes/routes.go:266	processing HTTPRoute	{"namespace": "test", "name": "test"}
2024-01-25T11:14:20.418Z	INFO	kubernetes/routes.go:266	processing HTTPRoute	{"namespace": "test", "name": "test"}
2024-01-25T11:14:20.418Z	INFO	kubernetes/routes.go:266	processing HTTPRoute	{"namespace": "test", "name": "test"}
2024-01-25T11:14:20.419Z	INFO	kubernetes/routes.go:137	processing GRPCRoute	{"namespace": "test", "name": "test"}

@arkodg arkodg requested a review from shawnh2 January 25, 2024 22:57
@shawnh2
Copy link
Contributor

shawnh2 commented Jan 26, 2024

/retest

Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

thanks for adding this, generally lgtm, just some nits.

internal/provider/kubernetes/kubernetes_test.go Outdated Show resolved Hide resolved
internal/provider/kubernetes/predicates.go Show resolved Hide resolved
internal/envoygateway/config/decoder_test.go Show resolved Hide resolved
Signed-off-by: yeedove <[email protected]>
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (c86681c) 64.68% compared to head (b48a8cd) 64.62%.
Report is 6 commits behind head on main.

Files Patch % Lines
internal/provider/kubernetes/predicates.go 50.00% 4 Missing and 2 partials ⚠️
internal/provider/kubernetes/controller.go 96.15% 1 Missing ⚠️
internal/provider/kubernetes/filters.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2494      +/-   ##
==========================================
- Coverage   64.68%   64.62%   -0.07%     
==========================================
  Files         115      115              
  Lines       17472    17513      +41     
==========================================
+ Hits        11302    11317      +15     
- Misses       5449     5471      +22     
- Partials      721      725       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deszhou
Copy link
Contributor Author

deszhou commented Jan 26, 2024

@shawnh2 done, thank!

Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg merged commit e168ef9 into envoyproxy:main Jan 26, 2024
20 checks passed
@arkodg arkodg added the release-note Indicates a required release note label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Indicates a required release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support complete label in namespaceSelectorwatch
4 participants