Disable auth gracefully without impacting existing watchers #13577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This attempts to fix a special case of the problem described in etcd-io#12385, where trying to do `clientv3.Watch` with an expired token would result in `ErrGRPCPermissionDenied`, due to the failing authorization check in `isWatchPermitted`. Furthermore, the client can't auto recover, since `shouldRefreshToken` rightly returns false for the permission denied error. In this case, we would like to have a runbook to dynamically disable auth, without causing any disruption. Doing so would immediately expire all existing tokens, which would then cause the behavior described above. This means existing watchers would still work for a period of time after disabling auth, until they have to reconnect, e.g. due to a rolling restart of server nodes. This commit adds a client-side fix and a server-side fix, either of which is sufficient to get the added test case to pass. Note that it is an e2e test case instead of an integration one, as the reconnect only happens if the server node is stopped via SIGINT or SIGTERM. A generic fix for the problem described in etcd-io#12385 would be better, as that shall also fix this special case. However, the fix would likely be a lot more involved, as some untangling of authn/authz is required.
|
@sayap thanks for the PR, let me check. |
spzala
approved these changes
Jan 4, 2022
liangyuanpeng
added a commit
to liangyuanpeng/etcd
that referenced
this pull request
Jul 14, 2023
Disable auth gracefully without impacting existing watchers. Signed-off-by: Lan Liang <[email protected]>
liangyuanpeng
added a commit
to liangyuanpeng/etcd
that referenced
this pull request
Jul 14, 2023
Disable auth gracefully without impacting existing watchers. Signed-off-by: Lan Liang <[email protected]>
liangyuanpeng
added a commit
to liangyuanpeng/etcd
that referenced
this pull request
Jul 14, 2023
Disable auth gracefully without impacting existing watchers. Signed-off-by: Lan Liang <[email protected]>
liangyuanpeng
added a commit
to liangyuanpeng/etcd
that referenced
this pull request
Jul 14, 2023
Disable auth gracefully without impacting existing watchers. Signed-off-by: Lan Liang <[email protected]>
This was referenced Jul 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This attempts to fix a special case of the problem described in #12385,
where trying to do
clientv3.Watchwith an expired token would resultin
ErrGRPCPermissionDenied, due to the failing authorization check inisWatchPermitted. Furthermore, the client can't auto recover, sinceshouldRefreshTokenrightly returns false for the permission deniederror.
In this case, we would like to have a runbook to dynamically disable
auth, without causing any disruption. Doing so would immediately expire
all existing tokens, which would then cause the behavior described
above. This means existing watchers would still work for a period of
time after disabling auth, until they have to reconnect, e.g. due to a
rolling restart of server nodes.
This commit adds a client-side fix and a server-side fix, either of
which is sufficient to get the added test case to pass. Note that it is
an e2e test case instead of an integration one, as the reconnect only
happens if the server node is stopped via SIGINT or SIGTERM.
A generic fix for the problem described in #12385 would be better, as
that shall also fix this special case. However, the fix would likely be
a lot more involved, as some untangling of authn/authz is required.