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

added optional dbName in header for tls #641

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

manishkuri
Copy link
Contributor

@manishkuri manishkuri commented Sep 10, 2024

In the case of a TLS-enabled environment, our Istio service needs a dbName in the header to route the request. In all our environments, we don't necessarily keep a "default" database, so we want to send something from the UI to it.

Without it, the Attu connection breaks on milvusClient.healthCheck().
With this code change it works fine.

@@ -55,6 +55,10 @@ export class MilvusService {
if (process.env.SERVER_NAME) {
clientConfig.tls.serverName = process.env.SERVER_NAME;
}

if (process.env.DB_NAME) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this here. you can set the process.env.DATABASE. it will be passed from the client.

But still, you need to pass the database to the MilvusClient.

like this:

const clientConfig: ClientConfig = {
        address: milvusAddress,
        token,
        username,
        password,
        logLevel: process.env.ATTU_LOG_LEVEL || 'info',
        database: database || this.DEFAULT_DATABASE,
      };

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this here. you can set the process.env.DATABASE. it will be passed from the client.

But still, you need to pass the database to the MilvusClient.

like this:

const clientConfig: ClientConfig = {
        address: milvusAddress,
        token,
        username,
        password,
        logLevel: process.env.ATTU_LOG_LEVEL || 'info',
        database: database || this.DEFAULT_DATABASE,
      };

hi @shanghaikid , didn't get this.
i am assigning this to clientConfig.database inside if statement. i see in SDK that clientConfig has a optional field database. it is also working this way.
I did not understood your comment here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

database should be passed from the client to the server side.
The client will read process.env.DATABASE .

Yes, you can setup new DB_NAME env here, but it will be a conflict with process.env.DATABASE

Copy link
Contributor Author

@manishkuri manishkuri Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, updated.
thanks @shanghaikid

@shanghaikid shanghaikid merged commit 8824986 into zilliztech:main Sep 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants