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

Fix libraries in filter.go to use the correct operators supported by GCE #24

Merged
merged 2 commits into from
Aug 22, 2019

Conversation

prameshj
Copy link
Contributor

Reverts #6

Most GCE APIs don't support regexp filter, hence reverting this.

Picked this up in k/k and Routes Controller started failing when listing routes with:

GCERoutes.List(context.Background.WithDeadline(2019-08-19 23:56:59.880823189 +0000 UTC m=+5923.032348891 [59m59.8650081s]), ..., (name ~ e2e-test-.) (network ~ https://www.googleapis.com/compute/v1/projects/pavithrar-k8s-dev/global/networks/e2e-test) (description ~ k8s-node-route)) = , googleapi: Error 400: Invalid value for field 'filter': '(name ~ e2e-test-pavithrar-.) (network ~ https://www.googleapis.com/compute/v1/projects/pavithrar-k8s-dev/global/networks/e2e-test-pavithrar) (description ~ k8s-node-route)'. Invalid list filter expression., invalid

The GCE APIs only support filter of the form "name = value" and not regexp. gcloud seems to be implementing regex filter and invoking API to get the full list.
ingress-gce uses composite types for most resources and no filter so far, which is probably why it has not broken any ingress tests.

@prameshj
Copy link
Contributor Author

/assign @bowei @rramkumar1
@spencerhance

@prameshj
Copy link
Contributor Author

/hold

Actually we need a part of the changes in this PR. Equals needs to translate to "=" and NotEquals needs to translate to " != ".
From the GCE API docs:

"A filter expression that filters resources listed in the response. The expression must specify the field name, a comparison operator, and the value that you want to use for filtering. The value must be a string, a number, or a boolean. The comparison operator must be either =, !=, >, or <."

This is the same for Instances, Routes, Addresses, ForwardingRules..
We can keep RegexpEquals as an option still, but not the default. I will work on the changes and submit for review.

@prameshj
Copy link
Contributor Author

/hold

Actually we need a part of the changes in this PR. Equals needs to translate to "=" and NotEquals needs to translate to " != ".
From the GCE API docs:

"A filter expression that filters resources listed in the response. The expression must specify the field name, a comparison operator, and the value that you want to use for filtering. The value must be a string, a number, or a boolean. The comparison operator must be either =, !=, >, or <."

This is the same for Instances, Routes, Addresses, ForwardingRules..
We can keep RegexpEquals as an option still, but not the default. I will work on the changes and submit for review.

UPDATE:

Turns out the GCE documentation is a bit confusing here, I tried this out using the "Try this API" section here: https://cloud.google.com/compute/docs/reference/rest/v1/routes/list

  1. Using "eq" for string equality/regex works, The following filter returned the correct matches:
    '(name eq e2e-test-pavithrar-.*) (network eq https://www.googleapis.com/compute/v1/projects/pavithrar-gke-dev/global/networks/e2e-test-pavithrar) (description eq k8s-node-route)'

  2. Using "=" for works only for exact match. The following filter returned a 400 "Invalid value for filter ..."
    '(name = e2e-test-pavithrar-.*) (network = https://www.googleapis.com/compute/v1/projects/pavithrar-gke-dev/global/networks/e2e-test-pavithrar) (description = k8s-node-route)'

  3. Using "=" for exact string match works, but not for regex. The following filter returns the correct matches:
    '(network = "https://www.googleapis.com/compute/v1/projects/pavithrar-gke-dev/global/networks/e2e-test-pavithrar") (description = "k8s-node-route")'

  4. Using "eq" works for int and bool matches.
    a) '(network eq https://www.googleapis.com/compute/v1/projects/pavithrar-gke-dev/global/networks/e2e-test-pavithrar) (description eq k8s-node-route) (priority eq 1000)'
    b) '(autoCreateSubnetworks eq false)' for networks api works.

So, for exact string match, the docs are correct. We need to use "=". For regex match, the operator to use is "eq". "eq" also works for int and bool equality. Finally, "~" is not supported for regex yet.

Reverting this PR will replace all equality filters with "eq", which (based on 1 and 4 above) should work.

@prameshj
Copy link
Contributor Author

Added a new test and verified that the filters work after the revert(and did not work before)

/assign @bowei

Copy link
Contributor

@rramkumar1 rramkumar1 left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/cloud/filter/filter_resource_test.go Outdated Show resolved Hide resolved
pkg/cloud/filter/filter_resource_test.go Outdated Show resolved Hide resolved
pkg/cloud/filter/filter_resource_test.go Outdated Show resolved Hide resolved
@prameshj prameshj force-pushed the revert-6-fix-filters branch 2 times, most recently from 200235d to 95b6687 Compare August 21, 2019 20:08
@bowei
Copy link
Member

bowei commented Aug 22, 2019

It would be better if we are keeping the upper layer Regexp() functions to keep the semantic meaning until the translation into GCE API.

@prameshj
Copy link
Contributor Author

It would be better if we are keeping the upper layer Regexp() functions to keep the semantic meaning until the translation into GCE API.

I just tried making the change so we keep Regexp functions, but use 'eq' operator and use '=' for exact matches. Turns out we cannot do a mix of 2operators in the API query:
'(name eq e2e-test-.) (description eq k8s-node-route) (priority = 1000)' fails, but '(name eq e2e-test-.) (description eq k8s-node-route) (priority eq 1000)' works.

We could keep Regexp and equals but fill in the same operator 'eq' always.

@prameshj prameshj force-pushed the revert-6-fix-filters branch from 95b6687 to 7c9f156 Compare August 22, 2019 18:11
@prameshj prameshj changed the title Revert "Fix libraries in filter.go to reflect how GCE API list filtering works currently" Fix libraries in filter.go to use the correct operators supported by GCE Aug 22, 2019
@@ -199,13 +199,15 @@ func (fp *filterPredicate) String() string {
var op string
switch fp.op {
case regexpEquals:
op = "~"
op = "eq"
Copy link
Member

Choose a reason for hiding this comment

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

add comment "GCE API maps regexp comparison to eq"

GCE supports using 'eq' for regex matches and '=' for
exact matches. However, when using multiple filters, they need to
all use 'eq' or all '='. This commit changes the code to always use
'eq'.
@prameshj prameshj force-pushed the revert-6-fix-filters branch from 7c9f156 to 381822c Compare August 22, 2019 18:20
@bowei bowei merged commit 27a4ced into GoogleCloudPlatform:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants