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

init os-login sshkey #3221

Merged
merged 24 commits into from
Apr 1, 2020
Merged

init os-login sshkey #3221

merged 24 commits into from
Apr 1, 2020

Conversation

sebglon
Copy link
Contributor

@sebglon sebglon commented Mar 5, 2020

Create new terraform resource oslogin SSHPublicKey

hashicorp/terraform-provider-google#5389

  • Use-case 1: add the ssh public key on current the user defined on the provider
    User email is set by default with the current userinfO
  • Use-case 2: add the ssh public key on a specific user
    User email is provided with impersonated provider and google_client_openid_userinfo
    example here

Release Note Template for Downstream PRs (will be copied)

`google_os_login_ssh_public_key`

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@rileykarson, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 97 insertions(+))
TF OiCS: Diff ( 4 files changed, 115 insertions(+))

2 similar comments
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 97 insertions(+))
TF OiCS: Diff ( 4 files changed, 115 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 628 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 97 insertions(+))
TF OiCS: Diff ( 4 files changed, 115 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Looking good so far! I made a first pass at reviewing.

products/oslogin/api.yaml Outdated Show resolved Hide resolved
products/oslogin/api.yaml Outdated Show resolved Hide resolved
products/oslogin/api.yaml Outdated Show resolved Hide resolved
products/oslogin/api.yaml Outdated Show resolved Hide resolved
templates/terraform/examples/os_login_ssh_key_add.tf.erb Outdated Show resolved Hide resolved
templates/terraform/examples/os_login_ssh_key_add.tf.erb Outdated Show resolved Hide resolved
products/oslogin/terraform.yaml Show resolved Hide resolved
@sebglon
Copy link
Contributor Author

sebglon commented Mar 5, 2020

Hi today i have an issue.
We can upload a public key for a service account.
I am the owner of the project but i can't add an SSH Key for a service account on this project.
i have this message: End user credentials must match the user specified in the request.

Is it possible for this API to use something like "impersonate-service-account"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 616 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 616 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 87 insertions(+))
TF OiCS: Diff ( 4 files changed, 115 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 570 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 570 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 77 insertions(+))
TF OiCS: Diff ( 4 files changed, 105 insertions(+))

@rileykarson
Copy link
Member

rileykarson commented Mar 5, 2020

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 582 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 582 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 77 insertions(+))
TF OiCS: Diff ( 4 files changed, 105 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 597 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 597 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 82 insertions(+))
TF OiCS: Diff ( 4 files changed, 105 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 591 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 591 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 82 insertions(+))
TF OiCS: Diff ( 4 files changed, 105 insertions(+))

@sebglon
Copy link
Contributor Author

sebglon commented Mar 5, 2020

Hi,
is to possible to get the current user defined on the provuder to put them as default value on the user field?

@rileykarson
Copy link
Member

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 617 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 617 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 92 insertions(+))
TF OiCS: Diff ( 4 files changed, 104 insertions(+))

@sebglon
Copy link
Contributor Author

sebglon commented Mar 6, 2020

HI,
The defautl user email is now well setted.
But on the test i have this error Create response didn't contain critical fields. Create may not have succeeded.

I don't understand because the resp object is empty but not when i run the same API from here

And how can i decode properly the loginProfile json?

products/oslogin/api.yaml Show resolved Hide resolved
products/oslogin/terraform.yaml Outdated Show resolved Hide resolved
templates/terraform/post_create/sshkeyfingerprint.erb Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 72 insertions(+))
TF OiCS: Diff ( 4 files changed, 104 insertions(+))

1 similar comment
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 72 insertions(+))
TF OiCS: Diff ( 4 files changed, 104 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 623 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 623 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 92 insertions(+))
TF OiCS: Diff ( 4 files changed, 104 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 629 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 629 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 92 insertions(+))
TF OiCS: Diff ( 4 files changed, 104 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

@sebglon
Copy link
Contributor Author

sebglon commented Mar 9, 2020

Now i have a new error on import:

make testacc TEST=./google TESTARGS='-run=TestAccOSLogin*'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccOSLogin* -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
=== PAUSE TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
=== CONT  TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
panic: Invalid address to set: []string{"name"}

goroutine 482 [running]:
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*ResourceData).Set(0xc0000cc1c0, 0x2990f74, 0x4, 0x22f16c0, 0xc0008c25a0, 0x0, 0x0)
	/home/sebastien/go/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/resource_data.go:203 +0x2ee
github.com/terraform-providers/terraform-provider-google/google.parseImportId(0xc000936400, 0x2, 0x2, 0x2f95a20, 0xc0000cc1c0, 0xc000b1eb00, 0xc0005a6e70, 0x290f0a1)
	/home/sebastien/go/src/github.com/terraform-providers/terraform-provider-google/google/import.go:45 +0x4fa
github.com/terraform-providers/terraform-provider-google/google.resourceOSLoginSSHPublicKeyImport(0xc0000cc1c0, 0x250bf20, 0xc000b1eb00, 0x1e, 0xc0007ce1e8, 0x7ff42c23ce01, 0x0, 0x2)
	/home/sebastien/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_os_login_ssh_public_key.go:219 +0xae

I have no name properties on this object.
How can i skip this on import?

If i set the import_format: ["users/{{user}}/sshPublicKeys/{{fingerprint}}"] the parameters are bad:

TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample: testing.go:635: Step 1 error: Error reading OSLoginSSHPublicKey "users/users/sshPublicKeys/<My_EMAIL>/": googleapi: Error 403: End user credentials must match the user specified in the request.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 597 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 10 files changed, 603 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 10 files changed, 603 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

@sebglon
Copy link
Contributor Author

sebglon commented Mar 18, 2020

$ make testacc TEST=./google TESTARGS='-run=TestAccOSLogin*'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccOSLogin* -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
=== PAUSE TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
=== CONT  TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample
--- PASS: TestAccOSLoginSSHPublicKey_osLoginSshKeyProvidedUserExample (5.59s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	5.618s

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 10 files changed, 610 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 10 files changed, 610 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

I added a couple small nitpicks- I've just gotta verify the test in CI to merge, and the changes in sourcerepo are causing a compiler error stopping that from working.

@@ -2,4 +2,10 @@ if v, ok := d.GetOkExists("pubsub_configs"); !isEmptyValue(reflect.ValueOf(pubsu
log.Printf("[DEBUG] Calling update after create to patch in pubsub_configs")
Copy link
Member

Choose a reason for hiding this comment

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

I think this file inadvertently got modified, can you revert the changes?

products/oslogin/api.yaml Outdated Show resolved Hide resolved
products/oslogin/terraform.yaml Outdated Show resolved Hide resolved
provider/terraform/examples.rb Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

1 similar comment
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 9 files changed, 604 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

@sebglon
Copy link
Contributor Author

sebglon commented Apr 1, 2020

@rileykarson can i have a status of the review?
This lock a project on my firm.

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Sorry! I didn't seem to have gotten the notification from your last update. Looks good, just one small thing- can you add the resource to the sidebar? It'll be a single-resource product similar to https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/website-compiled/google.erb#L289-L295, with most of the values drawn from https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-3221-old..auto-pr-3221#diff-4781117125c62204e8a4e030cc2ef7f0R15-R20.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 10 files changed, 612 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 10 files changed, 612 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 2 files changed, 67 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@rileykarson rileykarson merged commit 03d8887 into GoogleCloudPlatform:master Apr 1, 2020
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
* init os-login sshkey

Signed-off-by: Sébastien GLON <[email protected]>

* Fix naming + example

* Fix endpoint

* clean build folder

* Apply suggestions from code review

Co-Authored-By: Riley Karson <[email protected]>

* Fix review

* Add post_create

* Add reference doc

* Try to put user on url request

* Fix user

* Get the default userinfo email

* Update products/oslogin/api.yaml

Co-Authored-By: Riley Karson <[email protected]>

* Apply suggestions from code review

* revert url_param_only

* creation fixed

* Work except import

* Add id_format and import_format

* Add Id

* Fix id

* Update source_repo_repository_update.go.erb

* Update source_repo_repository_update.go.erb

* revert

* Apply suggestions from code review

Co-Authored-By: Riley Karson <[email protected]>

* Add sidebar link

Co-authored-by: Sébastien GLON <[email protected]>
Co-authored-by: Riley Karson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants