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

Change vault_namespace ID to path #570

Merged

Conversation

petems
Copy link
Contributor

@petems petems commented Oct 21, 2019

  • The ID isn’t super useful, it’s only used by Vault internally
  • We should use the path as it allows imports of existing namespaces
  • Move the namespace ID to its own computed attribute namespace_id

@ghost ghost added the size/XS label Oct 21, 2019
@petems
Copy link
Contributor Author

petems commented Oct 21, 2019

After:

$ vault namespace create import_tester
Key     Value
---     -----
id      ghA9v
path    import_tester/
$ terraform import vault_namespace.import_tester import_tester
vault_namespace.import_tester: Importing from ID "import_tester"...
vault_namespace.import_tester: Import prepared!
  Prepared vault_namespace for import
vault_namespace.import_tester: Refreshing state... [id=import_tester]

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.
$ cat terraform.tfstate | jq .'resources'
[
  {
    "mode": "managed",
    "type": "vault_namespace",
    "name": "import_tester",
    "provider": "provider.vault",
    "instances": [
      {
        "schema_version": 0,
        "attributes": {
          "id": "import_tester",
          "namespace_id": "ghA9v",
          "path": "import_tester"
        },
        "private": "eyJzY2hlbWFfdmVyc2lvbiI6IjAifQ=="
      }
    ]
  }
]

@petems
Copy link
Contributor Author

petems commented Oct 21, 2019

Before:

$ terraform import vault_namespace.import_tester import_tester
vault_namespace.import_tester: Importing from ID "import_tester"...
vault_namespace.import_tester: Import prepared!
  Prepared vault_namespace for import
vault_namespace.import_tester: Refreshing state... [id=import_tester]

Error: error reading from Vault: Error making API request.

URL: GET http://127.0.0.1:8200/v1/sys/namespaces
Code: 405. Errors:

* 1 error occurred:
	* unsupported operation

@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Oct 29, 2019
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

@petems - I had earlier marked this as approved, but I've been thinking about it more and also discussed a change in an ID in a subsequent PR. I realized that actually, changing the ID isn't backwards compatible, and I was wondering if we could add a migration path for the ID instead?

@petems
Copy link
Contributor Author

petems commented Nov 5, 2019

@tyrannosaurus-becks Happy to do so, but I'll need a hand helping with that.

Are there examples of doing this in TF providers? Have you got time for a quick pairing session on it as well? 😄

@tyrannosaurus-becks
Copy link
Contributor

@petems actually, I have a sample! We went through the same discussion on this PR, here's a direct link to the convo: #581 (comment). You should be able to check out the subsequent commits to see how it was implemented.

@petems petems force-pushed the change_to_namespace_id branch 2 times, most recently from d0d1183 to a3ef21b Compare November 7, 2019 01:49
@petems
Copy link
Contributor Author

petems commented Nov 7, 2019

@tyrannosaurus-becks Perfect! I'm not sure how to add a test for this new scenario... is there a way to create a special resource to run tests on in the old state?

@petems
Copy link
Contributor Author

petems commented Nov 7, 2019

I can see it working in the logs with a manual test of manually editing the statefile:

2019-11-07T02:06:18.453Z [DEBUG] plugin.terraform-provider-vault: 2019/11/07 02:06:18 [INFO] Using Vault token with the following policies: root
2019-11-07T02:06:18.455Z [DEBUG] plugin.terraform-provider-vault: 2019/11/07 02:06:18 [DEBUG] Upgrading old ID to path - abcd123 to import_tester
2019-11-07T02:06:18.455Z [DEBUG] plugin.terraform-provider-vault: 2019/11/07 02:06:18 [DEBUG] Setting namespace_id to old ID - abcd123
2019-11-07T02:06:18.455Z [DEBUG] plugin.terraform-provider-vault: 2019/11/07 02:06:18 [DEBUG] Vault API Request Details:

* The ID isn’t super useful, it’s only used by Vault internally
* We should use the path as it allows imports of existing namespaces
* Move the namespace ID to its own computed attribute `namespace_id`
* Add logic detecting if the ID isnt the same as the path, then upgrade
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

This looks awesome. Thank you!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 162df3f into hashicorp:master Nov 7, 2019
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
tmatilai added a commit to tmatilai/terraform-provider-vault that referenced this pull request Feb 11, 2023
The `id` of the namespace resource was changed to be the path in hashicorp#570,
and the ID of the namespace is now in the `namespace_id` attribute, but
the documentation was not updated.
tmatilai added a commit to tmatilai/terraform-provider-vault that referenced this pull request Feb 13, 2023
The `id` of the namespace resource was changed to be the full path in hashicorp#570,
and the ID of the namespace is now in the `namespace_id` attribute, but
the documentation was not updated.
fairclothjm pushed a commit that referenced this pull request May 20, 2023
* Fix `namespace_id` attribute name in the `vault_namespace` resource docs

The `id` of the namespace resource was changed to be the full path in #570,
and the ID of the namespace is now in the `namespace_id` attribute, but
the documentation was not updated.

* Document that `path_fq` is relative to the provider's namespace

* Fix and update the import example

* Refactor the nested namespace example

Drop extraneous local variable, and iterate over previous resources.

* Add back the `id` attribute, and rephrase the `namespace_id` description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants