-
Notifications
You must be signed in to change notification settings - Fork 107
fix: Allow users to set environment variable to connect to emulator running on docker #1164
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
Merged
danieljbruce
merged 11 commits into
googleapis:main
from
danieljbruce:1119-unable-to-connect-to-emulator
Sep 29, 2023
Merged
fix: Allow users to set environment variable to connect to emulator running on docker #1164
danieljbruce
merged 11 commits into
googleapis:main
from
danieljbruce:1119-unable-to-connect-to-emulator
Sep 29, 2023
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of checking to see if the endpoint is local, we should check to see if the datastore emulator variable is set. This is because there is an edge case where the user is using an emulator, but the emulator is not listening from local host and in this case we want to use insecure credentials.
Add unit tests for evaluating value of ssl credentials. Tests evaluate what happens when the user uses remote custom endpoints or local custom endpoints in combination with different values for the DATASTORE_EMULATOR_HOST variable.
This reverts commit dea1706.
This reverts commit a3b50df.
The tests should use a concise apiEndpoint variable and they should set the host for the remote case to be the same as the apiEndpoint variable.
More test cases are needed to describe what happens when the DATASTORE_EMULATOR_HOST variable is not set both in a remote and in a localhost environment because those behaviors should be different. Setting the apiEndpoint to localhost means the user wants to use the emulator without specifying the environment variable so insecure credentials should be provided. Setting the apiEndpoint to remote means the user likely wishes to use regional endpoints so should not be using insecure credentials to skip authentication.
If the user uses localhost as the endpoint or they provide DATASTORE_EMULATOR_HOST then it is assumed that the user is using the emulator and authentication is skipped. Otherwise, authentication is not skipped.
The old test title should be what it was before. setHost is now only needed in one place.
The comments need to be added to explain why the test is written the way that it is written so that we know the motivation behind each test.
alexander-fenster
approved these changes
Sep 27, 2023
…into 1119-unable-to-connect-to-emulator # Conflicts: # test/index.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
api: datastore
Issues related to the googleapis/nodejs-datastore API.
size: m
Pull request size is medium.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1119
Summary:
Originally, https://github.com/googleapis/nodejs-datastore/pull/1101/files was merged to support users who wanted to pass in a custom regional endpoint to use a datastore service hosted at a different address than the default endpoint. Before PR 1101 the code assumed that if a custom endpoint was provided then the emulator was being used so insecure credentials should be set so authentication can be skipped, but for users using regional endpoints this does not work. Users using regional endpoints still need authentication so a change in PR 1101 was introduced to set insecure credentials and skip authentication only if the provided endpoint was localhost.
After the PR was merged, users were affected who were using the emulator on a docker service. This is because after PR 1101 was merged the code is checking to see if the custom endpoint provided is localhost, realizing that it is not and so is not setting insecure authentication credentials like it was before and is not skipping authentication. If users are using the emulator on docker, they should at least be able to set DATASTORE_EMULATOR_HOST to indicate that they want to use the emulator so that authentication can be skipped and they can use the emulator.
Changes:
This PR allows the code to assume that the user is using the emulator if the endpoint provided is localhost or the DATASTORE_EMULATOR_HOST environment variable is set. The reason we cannot just check to see if the DATASTORE_EMULATOR_HOST variable is set is because the user might not be using this environment variable when using the emulator and instead might be passing in the emulator address in apiEndpoint as instructed by the following documentation:
nodejs-datastore/src/index.ts
Lines 153 to 155 in 6f6acfd
Test cases failing before source code changes:
checking ssl credentials are set correctly with custom endpoints
with DATASTORE_EMULATOR_HOST environment variable set
with DATASTORE_EMULATOR_HOST set to remote host
should use insecure ssl credentials when ssl credentials are not provided:
Remaining limitations:
With this change there are still two cases where users will not have their preferred experience.