Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB #14988

huberty89 · 2022-11-10T15:12:13Z

Description

Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB

Fixes

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# MongoDB
* Fix failure when an unauthorized exception happens while listing schemas or tables. ({issue}`1398`)

huberty89 · 2022-11-10T15:16:27Z

I need to update the docs

huberty89 · 2022-11-10T20:53:03Z

Looks like TestMetadataPrivilegesRequired test added by me is flaky - Docker container with MongoDB cannot starts successfully.
I will take a look on Monday.

ebyhr · 2022-11-11T00:09:44Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

@@ -44,6 +44,7 @@
    private Optional<String> connectionUrl = Optional.empty();
    private List<ServerAddress> seeds = ImmutableList.of();
    private List<MongoCredential> credentials = ImmutableList.of();
+    private boolean metadataPrivilegesRequired = true;


Could you share why we want to introduce a config property? Other connectors hide unauthorized databases and tables. Always enabling authorizedDatabasesOnly & authorizedCollections properties looks fine to me.

But older versions behave differently with this config enabled so unless we confirm that we don’t support older versions of Mongo (i.e 4.0) - enabling it by default might cause some issue. @huberty89 can you share how it works in the latest and older version ?

Now I also agree to not introduce new property.
In older version of MongoDB returned list of databases and collection was filtered out, in newer versions it's required to have specific privilege to list those. So this behavior has changed between versions only for authorizedDatabases=true and authorizedCollections=true

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/MongoServer.java

ebyhr · 2022-11-11T00:10:00Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMetadataPrivilegesRequired.java

+
+    private static MongoServer setupMongoServer()
+    {
+        MongoServer server = new MongoServer("4.0.6", ImmutableMap.of(


MongoDB 4.0 already reached EOL on Apr 30. Let's use a new version. Upcoming FTE on MongoDB will upgrade the minimum supported version to 4.2.

I set version to 4.4.17 - it seams docker init flakiness (when I set root user and password via env) got fixed

Hmm, it's still failing in my environment. It looks relating to testcontainers/testcontainers-java#4695

Looks like org.testcontainers.containers.MongoDBContainer contains a bug.

When I set MONGO_INITDB_ROOT_USERNAME and MONGO_INITDB_ROOT_PASSWORD this cause a restart MongoDB process.
At the same time db.runCommand( { isMaster: 1 } ).ismaster==false is executed inside Docker container in a loop (60 times) and during process restart this throw network error - that's the main issue.

I create AuthenticatedMongoServer which use GenericContainer directly. Default enabled replication is not needed in current scenario.

skrzypo987

Looks ok, ping me for another round.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMetadataPrivilegesRequired.java

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoClientConfig.java

Praveen2112 · 2022-11-14T10:40:07Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

@@ -44,6 +44,7 @@
    private Optional<String> connectionUrl = Optional.empty();
    private List<ServerAddress> seeds = ImmutableList.of();
    private List<MongoCredential> credentials = ImmutableList.of();
+    private boolean metadataPrivilegesRequired = true;


But older versions behave differently with this config enabled so unless we confirm that we don’t support older versions of Mongo (i.e 4.0) - enabling it by default might cause some issue. @huberty89 can you share how it works in the latest and older version ?

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMetadataPrivilegesRequired.java

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/MongoServer.java

huberty89 · 2022-11-16T14:21:15Z

@skrzypo987 @ebyhr @Praveen2112 I make an update, please take a look

ebyhr

Looks good except for the flaky container initialization & a minor comment.

ebyhr · 2022-11-17T01:08:34Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoPrivileges.java

+
+    private static void createTestRole(MongoDatabase database)
+    {
+        Double commandStatus = database.runCommand(new BsonDocument("createRole", new BsonString(TEST_ROLE))


Is it possible to use Document class to improve readability?

Got it, I also try to improve readability by spiting it into few methods

s2lomon

Please elaborate on why these changes are required in commit message. Right now it doesn't seem important and it is. In addition please describe what is the current behaviour etc.

s2lomon · 2022-11-18T11:37:41Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

            if (schemaName.equals(remoteSchemaName.toLowerCase(ENGLISH))) {
                return remoteSchemaName;
            }
        }
        return schemaName;
    }

+    private Iterable<String> listDatabaseNames()


Maybe instead returning Iterable from here and later turning it to Stream through ImmutableList.of(listDatabaseNames()).stream(), please return Iterable from here, for example by Streams.stream(...).

If the name of the method starts with list maybe just return a list?

We discuss this offline, because listDatabaseNames is private I return now MongoIterable

s2lomon · 2022-11-18T11:46:08Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

@@ -178,7 +183,7 @@ public List<HostAddress> getAddresses()

    public List<String> getAllSchemas()
    {
-        return ImmutableList.copyOf(client.listDatabaseNames()).stream()
+        return ImmutableList.copyOf(listDatabaseNames()).stream()


please look below - change the return type of listDatabaseNames to make it simpler to read. I'm kind of lost on what is transformed into what.

Praveen2112 · 2022-11-21T07:38:16Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

+        MongoDatabase database = client.getDatabase(databaseName);
+        Document cursor = database.runCommand(new Document(AUTHORIZED_LIST_COLLECTIONS_COMMAND)).get("cursor", Document.class);
+
+        List<Document> firstBatch = cursor.get("firstBatch", List.class);


Would there be another batch ?

Or a next batch ?

I checked in a MongoDB Java driver source and by just creating 10,000 collections in MongoDB and by default MongoDB returns everything in a single response, so this implementation should be sufficient.

Does MongoDB takes too long to create 10,000 collections ? Can we add it as a test to ensure that if the newer version returns in some batch we could handle the same ?

Listing 10,000 collections took for me 1.14 second. Still agree a test for this case is a good idea, I will add it

Praveen2112 · 2022-11-21T07:49:40Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoPrivileges.java

+    {
+        assertThat(getQueryRunner().execute("SHOW TABLES FROM mongodb." + TEST_DATABASE)
+                .getOnlyColumnAsSet())
+                .containsExactly(TEST_COLLECTION.toLowerCase(Locale.ENGLISH));


Do we need a redundant conversion ? Can we use assertQuery where we could provide a result as VALUES ...

Praveen2112 · 2022-11-21T07:50:07Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoPrivileges.java

+        }
+    }
+
+    private static void createTestRole(MongoDatabase database)


Can we move this to MongoServer

I moved into AuthenticatedMongoServer

skrzypo987

lgtm % minor comment and other's comments

skrzypo987 · 2022-11-21T11:01:26Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

            if (schemaName.equals(remoteSchemaName.toLowerCase(ENGLISH))) {
                return remoteSchemaName;
            }
        }
        return schemaName;
    }

+    private Iterable<String> listDatabaseNames()


If the name of the method starts with list maybe just return a list?

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoPrivileges.java

Send `authorizedDatabases=true` parameter while listing databases/schemas Send `authorizedCollections=true` parameter while listing collections/tables Without those listing databases & collections require `listDatabases` & `listCollections` actions.

huberty89 · 2022-12-04T23:30:44Z

@Praveen2112 @skrzypo987 @ebyhr Please take a look if everything looks good and it's good to be merged, thanks

ebyhr · 2022-12-05T00:13:17Z

Merged, thanks!

findepi · 2023-01-10T09:10:28Z

Follow up: #15652

cla-bot bot added the cla-signed label Nov 10, 2022

huberty89 requested review from s2lomon, skrzypo987, kokosing, Praveen2112 and ebyhr November 10, 2022 15:12

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 0db337e to 0dfe3d0 Compare November 10, 2022 20:37

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 0dfe3d0 to 0c8ad0a Compare November 10, 2022 22:01

ebyhr reviewed Nov 11, 2022

View reviewed changes

skrzypo987 reviewed Nov 11, 2022

View reviewed changes

Praveen2112 reviewed Nov 14, 2022

View reviewed changes

Praveen2112 reviewed Nov 15, 2022

View reviewed changes

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java Outdated Show resolved Hide resolved

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/MongoServer.java Outdated Show resolved Hide resolved

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 0c8ad0a to 5e159c8 Compare November 16, 2022 14:15

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 5e159c8 to 32b1889 Compare November 16, 2022 14:30

huberty89 changed the title ~~Add property to set requirement of privileges for fetching metadata from MongoDB~~ Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB Nov 16, 2022

martint force-pushed the master branch from d011a28 to d002d09 Compare November 16, 2022 15:35

ebyhr reviewed Nov 17, 2022

View reviewed changes

s2lomon reviewed Nov 18, 2022

View reviewed changes

Praveen2112 reviewed Nov 21, 2022

View reviewed changes

skrzypo987 reviewed Nov 21, 2022

View reviewed changes

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch 2 times, most recently from 5857836 to 963b174 Compare November 22, 2022 13:06

ebyhr reviewed Nov 23, 2022

View reviewed changes

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoPrivileges.java Outdated Show resolved Hide resolved

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 963b174 to 47fcc6b Compare December 4, 2022 22:35

huberty89 added 2 commits December 4, 2022 23:46

Test schemas visibility for MongoDB with authorization

87bf835

huberty89 force-pushed the huberty89/mongo-metadata-privileges branch from 47fcc6b to 87bf835 Compare December 4, 2022 22:47

ebyhr approved these changes Dec 4, 2022

View reviewed changes

ebyhr merged commit a53d472 into trinodb:master Dec 5, 2022

ebyhr mentioned this pull request Dec 5, 2022

Release notes for 405 #15058

Closed

github-actions bot added this to the 404 milestone Dec 5, 2022

ebyhr mentioned this pull request Dec 6, 2022

Use username/password authentication in MongoDB docker tests #7854

Closed

colebow mentioned this pull request Dec 14, 2022

Add Trino 405 release notes #15139

Merged

This was referenced Jan 10, 2023

Fix Mongo connector Atlas integration #15652

Closed

Enable authorized databases option in MongoDB connector #1398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB #14988

Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB #14988

huberty89 commented Nov 10, 2022 •

edited by ebyhr

Loading

huberty89 commented Nov 10, 2022

huberty89 commented Nov 10, 2022

ebyhr Nov 11, 2022

Praveen2112 Nov 14, 2022

huberty89 Nov 16, 2022 •

edited

Loading

ebyhr Nov 11, 2022

huberty89 Nov 16, 2022

ebyhr Nov 17, 2022 •

edited

Loading

huberty89 Nov 22, 2022

skrzypo987 left a comment

Praveen2112 Nov 14, 2022

huberty89 commented Nov 16, 2022

ebyhr left a comment

ebyhr Nov 17, 2022

huberty89 Nov 22, 2022

s2lomon left a comment

s2lomon Nov 18, 2022

skrzypo987 Nov 21, 2022

huberty89 Nov 22, 2022

s2lomon Nov 18, 2022

Praveen2112 Nov 21, 2022

Praveen2112 Nov 21, 2022

huberty89 Nov 28, 2022

Praveen2112 Nov 29, 2022

huberty89 Nov 29, 2022

Praveen2112 Nov 21, 2022

huberty89 Dec 4, 2022

Praveen2112 Nov 21, 2022

huberty89 Nov 22, 2022

skrzypo987 left a comment

skrzypo987 Nov 21, 2022

huberty89 commented Dec 4, 2022

ebyhr commented Dec 5, 2022

findepi commented Jan 10, 2023

Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB #14988

Enable authorizedDatabases & authorizedCollections for fetching metadata in MongoDB #14988

Conversation

huberty89 commented Nov 10, 2022 • edited by ebyhr Loading

Description

Fixes

Release notes

huberty89 commented Nov 10, 2022

huberty89 commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huberty89 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huberty89 commented Nov 16, 2022

ebyhr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s2lomon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huberty89 commented Dec 4, 2022

ebyhr commented Dec 5, 2022

findepi commented Jan 10, 2023

huberty89 commented Nov 10, 2022 •

edited by ebyhr

Loading

huberty89 Nov 16, 2022 •

edited

Loading

ebyhr Nov 17, 2022 •

edited

Loading