-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add -topo_zk_auth_file
flag
#4733
Conversation
Signed-off-by: Adam Saponara <[email protected]>
go/vt/topo/zk2topo/zk_conn.go
Outdated
@@ -108,7 +109,6 @@ func (c *ZkConn) Get(ctx context.Context, path string) (data []byte, stat *zk.St | |||
return | |||
} | |||
|
|||
// GetW is part of the Conn interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these comments obviously add no value but I'm pretty sure some linter or the other (maybe gofmt) will complain about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted the comments because, afaict, they are not part of the Conn
interface from go/vt/topo/conn.go
although I may be missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should have generated lint errors. These comments are required by go coding standards: https://github.com/golang/go/wiki/CodeReviewComments#doc-comments. I know it's silly, but we'll need to add them back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back in
return fmt.Errorf("addAuth: expected args <scheme> <auth>") | ||
} | ||
scheme, auth := subFlags.Arg(0), subFlags.Arg(1) | ||
return zconn.AddAuth(ctx, scheme, []byte(auth)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any assumptions about the args that should be validated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can only validate if we limit the schemes -- e.g., digest
scheme should have format user:pass
, ip
scheme should have format addr/mask
. However, ZooKeeper's auth system is pluggable, so someone could write scheme foobar
with some unknown format which we cannot validate.
Signed-off-by: Adam Saponara <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@sougou can you take a look one more time? if it looks good, I can merge it. |
If the flag is specified, Vitess clients will send an auth packet at connect time. This allows for the use of ACLs on the ZooKeeper side. We are using this for basic
digest:user:pass
authentication. Relevant ZooKeeper docs:https://zookeeper.apache.org/doc/r3.4.13/zookeeperProgrammers.html#sc_BuiltinACLSchemes