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

backend/remote: Swift Authentication Update #23510

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

kayrus
Copy link
Contributor

@kayrus kayrus commented Nov 27, 2019

This PR syncs OpenStack auth options with provider auth options.

See https://github.com/terraform-providers/terraform-provider-openstack/issues/686 for a reference.

/cc @jtopjian @radeksimko

@kayrus kayrus force-pushed the new-swift-opts branch 2 times, most recently from b8be44d to b2e8a71 Compare November 27, 2019 08:11
@kayrus
Copy link
Contributor Author

kayrus commented Nov 27, 2019

/cc @ozerovandrei

@SvenDowideit
Copy link

would this PR mean that

provider "openstack" {
 user_name = data.vault_generic_secret.nectarcloud_ereefs.data["openstack_user_name"]
 tenant_id = data.vault_generic_secret.nectarcloud_ereefs.data["openstack_tenant_id"]
 tenant_name = data.vault_generic_secret.nectarcloud_ereefs.data["openstack_tenant_name"]
 password = data.vault_generic_secret.nectarcloud_ereefs.data["openstack_password"]
 auth_url = data.vault_generic_secret.nectarcloud_ereefs.data["openstack_auth_url"]
}

terraform {
  backend "swift" {
    container         = "terraform-state"
    archive_container = "terraform-state-archive"
  }
}

would just work?

ie https://github.com/terraform-providers/terraform-provider-openstack/issues/939 and https://github.com/terraform-providers/terraform-provider-openstack/issues/373

@LorbusChris
Copy link

@kayrus do you mind rebasing?

@kayrus
Copy link
Contributor Author

kayrus commented Dec 10, 2019

@LorbusChris rebased

@LorbusChris
Copy link

LorbusChris commented Dec 10, 2019

@kayrus thanks! maybe sqash the last two commits, though? (fixup the rebase commit, so it doesnt appear in commit history)

@kayrus
Copy link
Contributor Author

kayrus commented Dec 10, 2019

@LorbusChris they can be squashed via github merge menu.

@kayrus
Copy link
Contributor Author

kayrus commented Dec 10, 2019

@SvenDowideit unfortunately no.

@LorbusChris
Copy link

@radeksimko friendly ping. If you could have a look at this soon (and subsequently at #23601), that would be great! :)

@LorbusChris
Copy link

any update on this?

@kayrus
Copy link
Contributor Author

kayrus commented Jan 23, 2020

@apparentlymart @radeksimko kindly ping

@radeksimko radeksimko removed their request for review January 29, 2020 16:33
@LorbusChris
Copy link

ping

@LorbusChris
Copy link

this would need another rebase :/ any chance this could go in soon?

@kayrus
Copy link
Contributor Author

kayrus commented Feb 24, 2020

rebased

@LorbusChris
Copy link

@apparentlymart ping

@kayrus kayrus force-pushed the new-swift-opts branch 2 times, most recently from b80c3bf to 4d973e5 Compare March 13, 2020 14:31
@LorbusChris
Copy link

@apparentlymart @radeksimko kind ping

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #23510 into master will decrease coverage by 0.01%.
The diff coverage is 8.10%.

Impacted Files Coverage Δ
backend/remote-state/swift/backend_state.go 0.00% <0.00%> (ø)
backend/remote-state/swift/backend.go 12.09% <8.18%> (+0.87%) ⬆️
terraform/node_resource_plan.go 93.27% <0.00%> (+1.68%) ⬆️

@kayrus
Copy link
Contributor Author

kayrus commented Apr 24, 2020

@apparentlymart @radeksimko kind ping
/cc @jtopjian @ozerovandrei could you please help somehow to pay attention to this PR? From a contributor perspective current situation looks bizarre. I rebased this PR already 6 times.

@danieldreier
Copy link
Contributor

@kayrus I apologize for the need to do multiple rebases here. I know that getting updates to backends merged has been tough, and @pkolyvas (the Terraform Core product manager) and I (the Terraform Core engineering manager) are trying to improve that. The root issue has been that nobody on the core team has deep domain knowledge of Swift, so doing really good reviews of PRs is fairly challenging. We've had to individually reach out and ask favors of engineers who have that knowledge, which slows down the process a lot.

I'm hopeful that we can significantly speed up the backend review process by allowing people who contribute to this codebase to review and approve PRs. We recently updated our contributor guide and set up a CODEOWNERS section to track who is maintaining what. Swift currently lacks a maintainer. Once we have a few maintainers listed for this backend, I'm hopeful that the community of Terraform OpenStack contributors can review, test, and approve changes, so that Core feels more confident merging these changes quickly.

If anyone on this thread would like to become a maintainer, please either make a PR to CODEOWNERS proposing adding yourself, or email me ([email protected]) and Petros ([email protected]) with any questions you have. I'm happy to email or video conference.

@kayrus
Copy link
Contributor Author

kayrus commented Apr 25, 2020

Dear @danieldreier, thank you for your reply and explanation. This PR doesn't change the Swift logic and reviewers don't need to have a Swift knowledge. I'd prefer to avoid decisions regarding a maintainer role if possible, since this PR just synchronizes auth options between upstream terraform-provider-openstack and resolves a https://github.com/terraform-providers/terraform-provider-openstack/issues/686 opened by @radeksimko. Please let me know what do you think.

@jtopjian
Copy link
Contributor

jtopjian commented Apr 25, 2020

@danieldreier

As a previous maintainer of the OpenStack plugin and as someone who used to work on and review this part of the code in this repository, I can confidently say that the Swift-specific changes being done in this PR are safe.

This PR is a result of a request that was made at https://github.com/terraform-providers/terraform-provider-openstack/issues/686. The requested wanted to decouple the authentication logic being shared between the openstack plugin and this Swift remote backend:

hashicorp/terraform also depends on terraform-providers/terraform-provider-openstack because it shares some auth logic in the Swift remote backend.

Sharing the auth logic was problematic because this caused hashicorp/terraform to require all of the same Go dependencies as terraform-provider-openstack.

Work was done to satisfy this decoupling by creating a small package located at github.com/gophercloud/utils/terraform/auth. This package shared no dependencies with terraform-provider-openstack and thus would help remove the unnecessary dependencies from hashicorp/terraform. This PR is large, yes, but that is due to the amount of dependencies no longer required as a result of decoupling

There are some changes being made to the actual Swift logic, but they are all amendments to enhance the current authentication to reach feature parity with the authentication work that has happened since https://github.com/terraform-providers/terraform-provider-openstack/issues/686 was made - if that makes sense?

Basically, https://github.com/terraform-providers/terraform-provider-openstack/issues/686 was opened, we did a bunch of work to resolve that request, this PR was opened, and then in the meantime, additional authentication amendments were made which has to also be done here in order to use the shared authentication package we created.

Again, from reviewing this PR, I feel the changes being made safe. However, if any issues are caused by this PR, I will work with @kayrus on resolving them. Unfortunately I don't have the time to become a full maintainer (I used to), but to help move this PR along, I can commit to ensuring no issues come out of this PR.

@mildwonkey mildwonkey requested a review from a team April 27, 2020 19:02
Copy link
Contributor

@mildwonkey mildwonkey 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 good to me, thanks for all your hard work @kayrus and thanks for the review @jtopjian .
I'd like one more +1 from someone in Terraform Core please!

@LorbusChris
Copy link

:shipit:

@danieldreier
Copy link
Contributor

@kayrus @jtopjian thank you for the extended review, thoughtful responses, and the work on contributing this and getting it merged. We'll merge this and include it in the upcoming 0.13 release. I think it makes sense to have discussions about maintainers elsewhere. We'll pin a thread in the terraform community forum and continue that discussion over there.

@danieldreier danieldreier merged commit 1170efb into hashicorp:master Apr 27, 2020
@danieldreier danieldreier added this to the v0.13.0 milestone Apr 27, 2020
@kayrus kayrus deleted the new-swift-opts branch April 28, 2020 05:06
@pedrokiefer
Copy link

pedrokiefer commented May 14, 2020

Any chance this gets cherry-picked for 0.12.XX release?

@ghost
Copy link

ghost commented May 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 28, 2020
@danieldreier
Copy link
Contributor

Terraform 0.13.0 beta 1 launched today with this update! If you have time it would be awesome if you could try it out and make sure it works.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants