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

cv3/mirror: Fetch the most recent revision from the prefix #13923

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

endocrimes
Copy link
Contributor

When a user sets up a Mirror with a restricted user that doesn't have
access to the foo path, we will fail to get the most recent revision
due to permissions issues.

With this change, when a prefix is provided we will get the initial
revision from the prefix rather than /foo. This allows restricted users
to setup sync.

Fixes #13846

When a user sets up a Mirror with a restricted user that doesn't have
access to the `foo` path, we will fail to get the most recent revision
due to permissions issues.

With this change, when a prefix is provided we will get the initial
revision from the prefix rather than /foo. This allows restricted users
to setup sync.
@endocrimes endocrimes changed the title cv3/mirror: Fetch the most recent prefix revision cv3/mirror: Fetch the most recent revision from the prefix Apr 11, 2022
@endocrimes
Copy link
Contributor Author

@serathius I can't think of an obvious reason why this shouldn't be a reasonable approach here.

These tests should move to the common framework eventually, but I'll follow up with that seperately (as that's a larger change than is necessary to make an improvement here).

@endocrimes endocrimes mentioned this pull request Apr 11, 2022
28 tasks
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

Makes sense

@serathius
Copy link
Member

cc @ptabor

@serathius
Copy link
Member

TestMirrorSync_Authenticated, test added in this PR, consistently fails on grpc proxy tests. Please consider fixing it or disabling the proxy case.

@endocrimes
Copy link
Contributor Author

Oh exciting - I'll take a look

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

@serathius
Copy link
Member

I expect that grpc proxy doesn't support the prefix argument correctly. If you can't find the problem I would suggest disabling this test from proxy tests (moving to separate file and adding a !proxy build flag). If we cannot fix the tests soon this will not be included in v3.5.3.

@ahrtr
Copy link
Member

ahrtr commented Apr 12, 2022

lt seems the pipeline failure is caused by this PR.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

Please fix the pipeline failures.

@endocrimes
Copy link
Contributor Author

Moved and disabled proxy tests for now - not sure what the issue is after a quick look and I'm heads down on jepsen testing. Will follow up with figuring out whatever is weird.

@ahrtr
Copy link
Member

ahrtr commented Apr 12, 2022

Moved and disabled proxy tests for now - not sure what the issue is after a quick look and I'm heads down on jepsen testing. Will follow up with figuring out whatever is weird.

Looks good for now.

@serathius serathius merged commit f341b95 into etcd-io:main Apr 13, 2022
@serathius
Copy link
Member

@endocrimes Please send a backport to release-3.5 if you have time.

@endocrimes endocrimes deleted the dani/syncer branch April 13, 2022 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

etcdctl make-mirror doesn't work when RBAC enabled and user doesn't have read permissions of key "foo"
3 participants