-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: submodule for cloud memorystore (memcache) #22
feat: submodule for cloud memorystore (memcache) #22
Conversation
Hi @pkatsovich -- thanks for your contribution! Regarding (1), can you tell us more about the setup you're executing the module in when you see that behavior ( |
Hi @Jberlinsky thanks for taking a look at this. |
@pkatsovich Is the Cloud Memorystore for Memcached API enabled in the project that houses the service account? If not, please enable that and try again. |
@Jberlinsky thanks for the guidance. Enabling the cloudmemstore memcache api in the SA host project has fixed the 403 response. Are we expecting this behavior to remain, or will the API enablement only be required for the project to which cloudmemstore belongs? Are you able to verify my assumption re (2)? |
@pkatsovich API enablement is required on both the project the service account resides in, and the project targeted by the API action. The error message that you provided RE: (2) seems to exhibit the API enablement issue identified in (1). |
@Jberlinsky sorry copy/paste error in my original description, this is the actual error when try to use the
|
@pkatsovich Could you provide an example resource call that reproduces the issue? |
@Jberlinsky I'm getting this when running a terraform apply, using this branch. Below is the full output, including the plan
|
@pkatsovich It does indeed look like the issue you're pointing out and the pull request in the beta provider are related. I can't speak to when that might be released. |
af4d1bf
to
a9e096f
Compare
google-beta 3.31.0 has been released with the referenced fix. I've updated this PR to include an example and tests which I have passing locally. @Jberlinsky. would you be able to review/merge or provide the errors from google's CI? |
b7b6513
to
0c9e81f
Compare
@morgante or @Jberlinsky could I please get some feedback on why the int check is failing? |
@pkatsovich This is the error from CI:
Unfortunately this isn't terribly useful. My recommendation would be trying to upgrade the version of project factory used in the test/setup to the latest. |
@pkatsovich I would also bump the provider in setup to 3.x. I have noticed this in some other modules with 2.x |
@bharathkkb @morgante Thanks for the guidance. I bumped the provider in setup to 3.x, bumped project factory to latest (8.1.0) and set |
Yes, much clearer error now. Setup/converge succeeds; it fails on verify:
|
thanks @morgante, This is an error because of the gcloud sdk installed on the test instance. I was able to fix that error locally by updating the Makefile to use the latest |
The image version is specified here: https://github.com/terraform-google-modules/terraform-google-memorystore/blob/master/build/int.cloudbuild.yaml |
Thanks for the pointers @morgante. That looks to have been the last bit and the PR is passing all checks. Hoping this can be merged and included in the next module version! |
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.
Thanks for the contribution and patience with our CI. Just a few notes.
modules/memcache/main.tf
Outdated
region = var.region | ||
authorized_network = var.authorized_network | ||
node_count = var.node_count | ||
display_name = var.display_name |
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.
Can this default to var.name if not provided?
test/fixtures/memcache/main.tf
Outdated
*/ | ||
|
||
module "memcache" { | ||
source = "../../../modules/memcache" |
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.
Fixtures should call examples, not the module directly.
00915b9
to
3013377
Compare
3013377
to
af2b67b
Compare
Tests are failing with this error:
|
hi @morgante thanks. I think this might be a race condition. I attempted to handle the private service access using the same method that's use in the cloudsql module . https://github.com/terraform-google-modules/terraform-google-memorystore/pull/22/files#diff-c635286e9cc2991efe0732e02ba0d268R21 This was the reason I had the private-service-access in the setup originally. Any guidance is appreciated. |
@pkatsovich if it is a race condition issue, for Recently when I had a similar issue, I ended up using the module "private-service-access" {
source = "GoogleCloudPlatform/sql-db/google//modules/private_service_access"
version = "3.2.0"
project_id = var.project
vpc_network = "default"
}
module "memcache" {
source = "../../modules/memcache"
name = var.name
project = can(module.private-service-access.peering_completed) : var.project ? ""
memory_size_mb = var.memory_size_mb
enable_apis = var.enable_apis
cpu_count = var.cpu_count
region = var.region
} This is hacky but again native support is coming with 0.13. |
The other way to do this is with an implicit dependency. Specifically we should update the |
@bharathkkb thanks for the suggestion! @morgante please let me know if there's anything else on this. |
This PR is mostly working with 2 exceptions:
Error: Error waiting for Deleting Instance: error while retrieving operation: googleapi: Error 403: Cloud Memorystore for Memcached API has not been used in project ######## before or it is disabled
However, I assure you the API is enabled, and the GSA creating the resource has permissions.. because the instance actually gets created! (or destroyed if I need be). it's just the API seems to respond with a 403 no matter what.
memcache_parameters
attribute, it plans fine, but then fails to apply w/ a different error from the API:I suspect exception 2 is going to be resolved w/ the next release of the google-beta terraform provider, because I see a fix was merged into master but is not in the latest available release tag.
@morgante hoping you can provide some guidance here, thanks!