You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/09/08 13:58:03 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
dlmarion opened a new pull request, #2919:
URL: https://github.com/apache/accumulo/pull/2919
Closes #2890
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966033593
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
Review Comment:
I can do that, should I point them to the release notes for how to correct this situation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #2919:
URL: https://github.com/apache/accumulo/pull/2919
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966023665
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
Review Comment:
Could strengthen this message to indicate that upgrade failed and that the permissions need to be corrected so that the reason and action required are clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966039696
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
Review Comment:
Release notes - or maybe in the docs under troubleshooting / zookeeper section? The docs may be more permanent and I'm assuming that this check would run on versions after this release, so they would always be accessible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966972548
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,42 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean aclErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
Review Comment:
```suggestion
private void validateACLs(ServerContext context) {
final AtomicBoolean aclErrorOccurred = new AtomicBoolean(false);
```
Seems like this only used in the method. Wondering if could be declared in the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#issuecomment-1240987208
> It may also be desirable to check all paths, keeping a list of any nodes that are incorrect and then failing if the list size is > 0 and providing the complete list. That way they may be able to correct things once and the re-try the upgrade instead of needing to "find" each node individually.
That's what should happen. It should log all of them, then throw the exception at the end after the visit method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966217587
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
+ asyncErrorOccurred.set(true);
+ } else {
+ log.info("ZNode at {} has expected ACL.", path);
Review Comment:
Should this be at trace level - other than development it does not seem that I would care the path had the correct ACls
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966255496
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
Review Comment:
Added troubleshooting doc at https://github.com/apache/accumulo-website/pull/338. Once / if that is merged, then I will update this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2919: Validate ZNode ACLs prior to upgrade, log unexpected values and fail
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2919:
URL: https://github.com/apache/accumulo/pull/2919#discussion_r966051430
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -133,6 +141,40 @@ public void upgradeZookeeper(ServerContext context) {
createScanServerNodes(context);
}
+ private static final AtomicBoolean asyncErrorOccurred = new AtomicBoolean(false);
+
+ private void validateACLs(ServerContext context) {
+
+ final ZooReaderWriter zrw = context.getZooReaderWriter();
+ final ZooKeeper zk = zrw.getZooKeeper();
+ final String rootPath = context.getZooKeeperRoot();
+ try {
+ ZKUtil.visitSubTreeDFS(zk, rootPath, false, (rc, path, ctx, name) -> {
+ try {
+ final Stat stat = new Stat();
+ final List<ACL> acls = zk.getACL(path, stat);
+
+ if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
+ && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
+ || (!ZooUtil.PRIVATE.equals(acls) && !ZooUtil.PUBLIC.equals(acls))) {
+ log.error("ZNode at {} has unexpected ACL: {}", path, acls);
Review Comment:
Or could just recommend that they check the ZooKeeper documentation for `DigestAuthenticationProvider.superDigest` - but that was a not straight forward to put together what was needed. The text in #2890 provides more help in both getting the provider added and the way to set the Accumulo permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org