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

Dir problem/518 #2147

Closed
wants to merge 4 commits into from
Closed

Conversation

iankronquist
Copy link

Addresses issue #518
I think this fixes the issue, but I'm not sure if it's the right approach. After I dived in to the issue I realized why it sat open for a year.
I'm very open for alternate approaches.

Before adding IsKeyDir we should add tests.
IsKeyDir is necessary to address etcd-io#518 on github
IsKeyDir is needed so we can check if a given key is a directory. This
should be a public method because we need to check whether a request
includes Dir=true in etcdserver.

Addressed issue etcd-io#518 on github.
Test that if dir=true and prevExist=true PUT only if the target node is a
directory. Otherwise return "Not a directory".
Addresses issue etcd-io#518 on github
If dir=true and prevExist=true PUT only if the target node is a
directory. Otherwise return "Not a directory".

Addresses etcd-io#518 on github.
@iankronquist
Copy link
Author

Can someone take a look at this?

@iankronquist
Copy link
Author

@philips I know you're busy, but can you take a look at this when you get a chance?

@xiang90
Copy link
Contributor

xiang90 commented Feb 7, 2015

@iankronquist The checking logic should happen in the store not in etcd server. So can you try to make the change?

@iankronquist
Copy link
Author

So the interesting thing about this bug, as I understand it, is that the store doesn't know some key information in order to make that check, and the etcd server doesn't know whether the key is a directory. Both have really clean interfaces which prevent you from knowing both that a key is a directory and that prevExist=true at the same point in the code. I suspect this is why the bug hasn't been fixed for a year.
I chose to leak whether a given key is a directory to the etcd server, as opposed to leaking part of the content of the request to the store. I understand that the other option may be preferable, and I will prepare an implementation of what you suggest.
Unfortunately I have a midterm next week and am leading a bug bounty hunt tomorrow, so I may not get to it until later next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants