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

proxy unstable routing behavior for routes that match multiple rules #4497

Closed
rhafer opened this issue Sep 1, 2022 · 2 comments · Fixed by #4514
Closed

proxy unstable routing behavior for routes that match multiple rules #4497

rhafer opened this issue Sep 1, 2022 · 2 comments · Fixed by #4514
Assignees

Comments

@rhafer
Copy link
Contributor

rhafer commented Sep 1, 2022

Turns out that the config changes in #4461 cause some unstable behavior for routes that match multiple rules 😱. E.g. we have:

                {                                 
                    Endpoint:    "/app/list",
                    Backend:     "http://localhost:9140",
                    Unprotected: true,
                },
                {
                    Endpoint: "/app/", // /app or /apps? ocdav only handles /apps
                    Backend:  "http://localhost:9140",
                },

And now requests on /app/list/ sometimes go through and sometimes fail. (Not this is not exactly a new bug in the code, it just seems that we didn't have overlapping rules of the same type before. I'll open a new issue for that and try to come up with a quick fix. (We might also temporary revert this PR).

Originally posted by @rhafer in #4461 (comment)

@rhafer
Copy link
Contributor Author

rhafer commented Sep 1, 2022

Turns out that this is cause by the route iterating of the endpoints of the rules for a specific type in random order. So sometimes the /app rule is evaluated first and sometimes /app/list (See: https://github.com/owncloud/ocis/blob/master/services/proxy/pkg/router/router.go#L208)

rhafer added a commit to rhafer/ocis that referenced this issue Sep 1, 2022
This is a quickfix for owncloud#4497. Before evaluating we no sort the rules
of a specific type by the length of the endpoints. And start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.
rhafer added a commit to rhafer/ocis that referenced this issue Sep 1, 2022
This is a quickfix for owncloud#4497. Before evaluating, we now sort the rules
of a specific type by the length of the endpoints and start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.
rhafer added a commit that referenced this issue Sep 1, 2022
This is a quickfix for #4497. Before evaluating, we now sort the rules
of a specific type by the length of the endpoints and start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.
@rhafer
Copy link
Contributor Author

rhafer commented Sep 1, 2022

Even though the quick fix is merged. I'd like to keep this issue open for now. As the fix is somewhat inefficient. E.g. it is sorting the endpoints over and over again (ideally this would only happen once, at startup.
Also it might need adjustments for other types of rules than PrefixRoute

ownclouders pushed a commit that referenced this issue Sep 1, 2022
Author: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date:   Thu Sep 1 16:06:27 2022 +0200

    Evaluate routing rules ordered by path-length

    This is a quickfix for #4497. Before evaluating, we now sort the rules
    of a specific type by the length of the endpoints and start evaluation
    with the most specific endpoint first. There's obviously quite a bit
    room for optimization here and this will only fix the issue for routes
    of type `PrefixRoute`. But it should solve the immediate issue.
ownclouders pushed a commit that referenced this issue Sep 2, 2022
Author: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date:   Thu Sep 1 16:06:27 2022 +0200

    Evaluate routing rules ordered by path-length

    This is a quickfix for #4497. Before evaluating, we now sort the rules
    of a specific type by the length of the endpoints and start evaluation
    with the most specific endpoint first. There's obviously quite a bit
    room for optimization here and this will only fix the issue for routes
    of type `PrefixRoute`. But it should solve the immediate issue.
rhafer added a commit to rhafer/ocis that referenced this issue Sep 5, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. There are not store in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 5, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. There are not store in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 5, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 5, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 6, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 6, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 6, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 6, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit to rhafer/ocis that referenced this issue Sep 7, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
rhafer added a commit that referenced this issue Sep 7, 2022
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: #4497
ownclouders pushed a commit that referenced this issue Sep 7, 2022
Author: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date:   Mon Sep 5 17:03:10 2022 +0200

    proxy: Avoid sorting endpoints for every single request

    The endpoints are no longer hashed by path name in the directors map since
    that made iterating over the endpoints unstable. They are now stored in a
    slice in the order in which the are defined in the configuration.

    Closes: #4497
butonic added a commit that referenced this issue Sep 8, 2022
* Add support for the jsoncs3 share manager

* WIP: point to the wip-reva with the jsoncs3 share manager

* migration fixes

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use common config for jsoncs3 defaults

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* lint

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* add home space deletion on user delete

Signed-off-by: Christian Richter <crichter@owncloud.com>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* add delete home space permission to admin role

Signed-off-by: Christian Richter <crichter@owncloud.com>

* Bump github.com/tus/tusd from 1.9.0 to 1.9.1

Bumps [github.com/tus/tusd](https://github.com/tus/tusd) from 1.9.0 to 1.9.1.
- [Release notes](https://github.com/tus/tusd/releases)
- [Commits](tus/tusd@v1.9.0...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/tus/tusd
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/gomega from 1.20.0 to 1.20.1

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.0 to 1.20.1.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.0...v1.20.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* part1

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* [test-only] apiTest. change own password (#4480)

* apiTest. change own password

* fix

* Bump github.com/gookit/config/v2 from 2.1.2 to 2.1.4

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.2...v2.1.4)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* move graph api methods from Space context to GraphHelper

* fix after rebase

* Bump github.com/rs/zerolog from 1.27.0 to 1.28.0

Bumps [github.com/rs/zerolog](https://github.com/rs/zerolog) from 1.27.0 to 1.28.0.
- [Release notes](https://github.com/rs/zerolog/releases)
- [Commits](rs/zerolog@v1.27.0...v1.28.0)

---
updated-dependencies:
- dependency-name: github.com/rs/zerolog
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/ginkgo/v2 from 2.1.4 to 2.1.6

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.1.4 to 2.1.6.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.1.4...v2.1.6)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/coreos/go-oidc/v3 from 3.2.0 to 3.3.0

Bumps [github.com/coreos/go-oidc/v3](https://github.com/coreos/go-oidc) from 3.2.0 to 3.3.0.
- [Release notes](https://github.com/coreos/go-oidc/releases)
- [Commits](coreos/go-oidc@v3.2.0...v3.3.0)

---
updated-dependencies:
- dependency-name: github.com/coreos/go-oidc/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Added /Shares related tests for lock properties on ocis

* Update expected to failure for local api for copy

* Bump github.com/grpc-ecosystem/grpc-gateway/v2 from 2.11.2 to 2.11.3

Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.11.2 to 2.11.3.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/master/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.11.2...v2.11.3)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* refactor proxy code

I refactored the proxy so that we execute the routing before the
authentication middleware. This is necessary so that we can determine
which routes are considered unprotected i.e. which routes don't need
authentication.

* add unprotected flag to the proxy routes

I added an unprotected flag to the proxy routes which is evaluated by
the authentication middleware. This way we won't have to maintain a
hardcoded list of unprotected paths and path prefixes and we will
hopefully reduce the times we encounter the basic auth prompt by web
browsers.

* add missing unprotected flag and fix proxy test

* fix linting issues

* fix default policy and add changelog

* update tests

* Automated changelog update [skip ci]

* Bump google.golang.org/grpc from 1.48.0 to 1.49.0

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.48.0 to 1.49.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.48.0...v1.49.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/gomega from 1.20.1 to 1.20.2

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.1 to 1.20.2.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.1...v1.20.2)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/urfave/cli/v2 from 2.11.2 to 2.14.0

Bumps [github.com/urfave/cli/v2](https://github.com/urfave/cli) from 2.11.2 to 2.14.0.
- [Release notes](https://github.com/urfave/cli/releases)
- [Changelog](https://github.com/urfave/cli/blob/main/docs/CHANGELOG.md)
- [Commits](urfave/cli@v2.11.2...v2.14.0)

---
updated-dependencies:
- dependency-name: github.com/urfave/cli/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/owncloud/libre-graph-api-go from 0.16.0 to 0.17.0

Bumps [github.com/owncloud/libre-graph-api-go](https://github.com/owncloud/libre-graph-api-go) from 0.16.0 to 0.17.0.
- [Release notes](https://github.com/owncloud/libre-graph-api-go/releases)
- [Commits](owncloud/libre-graph-api-go@v0.16.0...v0.17.0)

---
updated-dependencies:
- dependency-name: github.com/owncloud/libre-graph-api-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Evaluate routing rules ordered by path-length

This is a quickfix for #4497. Before evaluating, we now sort the rules
of a specific type by the length of the endpoints and start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.

* Bump commit id for tests

* Improve login screen design

* Move background-size after the background css prop

* add returns after rendering errors and simplify loop condition

* Hardcode header in GraphHelper::deleteSpace

* Add 'Inter' font, change placeholder color to grey

* Use cv11 as font feature setting

* Automated changelog update [skip ci]

* adjust REPORT to PROPFIND endpoint

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* regenerate protogen

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* adjust expected failures

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* Automated changelog update [skip ci]

* Fix translations on login page

* Automated changelog update [skip ci]

* removed search test from expected failures

* Reuse code of code from core at most

* Add commit and branch related to this PR to work

* Reuse locking method from core

* fix search test

* Add api tests for tus upload when quota is set

* Add tests removed by core pr-40276

* graph: Fix Status code when updating the password

Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See #4480

* Bump github.com/gookit/config/v2 from 2.1.4 to 2.1.5

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.4 to 2.1.5.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.4...v2.1.5)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* auto orientate pictures for thumbnails

* Automated changelog update [skip ci]

* update buf lock

* graph: purge home space when deleting a user

previously the homespace was just marked as trashed

Fixes: #4195

* proxy: Initialize logger for router

* proxy: Fix archiver for public links

Allows /archiver to be used the "public-token" auth middleware. The
archiver is a bit of a special case, because it can be uses in several
ways: using 'normal' authentication (basic, oidc), using signed-urls or
using sharetokens. As only the "sharetoken" part is handled by the
"PublicShareAuth" middleware, we needed to special-case it a bit.

* proxy: Clarify comment

* proxy: Avoid sorting endpoints for every single request

The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: #4497

* Automated changelog update [skip ci]

* update reva to latest edge

* remove personal spaces as admin in the graph test suite

* use id to delete project spaces

* Add context in behat.yml

* Fix home space deletion when deleting user by name

DELETE requess on /graph/v1.0/users also work when specifing a user by
name. For deleting the home space in that case we need to get the User's
id from the backend first.

Fixes: #4195

* Add new "delete-all-spaces" permission

This is assigned to the Admin role by default and allows to cleanup
orphaned spaces (e.g. where the owner as been deleted)

Fixes: #4196

* Automated changelog update [skip ci]

* Automated changelog update [skip ci]

* update reva to latest edge

* Automated changelog update [skip ci]

* delete users with their personal spaces

* update reva to latest edge

* delete method

* Remove tests from unrelated issue and add actual one

* REPORT: add permissions to personal space

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* changelog and bump reva

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* Automated changelog update [skip ci]

* update reva and jsoncs3 share manager config

* use experimental reva

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* fix search service after merge

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* praise the linting god

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* use cs3 share manager

Signed-off-by: jkoberg <jkoberg@owncloud.com>

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Christian Richter <crichter@owncloud.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: André Duffeck <andre.duffeck@firondu.de>
Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Co-authored-by: Christian Richter <crichter@owncloud.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Viktor Scharf <scharf.vi@gmail.com>
Co-authored-by: David Christofas <dchristofas@owncloud.com>
Co-authored-by: sagargurung1001@gmail.com <sagargurung1001@gmail.com>
Co-authored-by: Ralf Haferkamp <rhaferkamp@owncloud.com>
Co-authored-by: Swikriti Tripathi <swikriti808@gmail.com>
Co-authored-by: Jannik Stehle <jannik.stehle@gmail.com>
Co-authored-by: Phil Davis <phil@jankaritech.com>
Co-authored-by: Swikriti Tripathi <41103328+SwikritiT@users.noreply.github.com>
Co-authored-by: Jannik Stehle <50302941+JammingBen@users.noreply.github.com>
Co-authored-by: Sagar Gurung <46086950+SagarGi@users.noreply.github.com>
Co-authored-by: Amrita <54478846+amrita-shrestha@users.noreply.github.com>
Co-authored-by: Michael Barz <mbarz@owncloud.com>
Co-authored-by: amrita <anukanxi05@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant