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

Add support for Consul Peering #309

Merged
merged 8 commits into from Sep 23, 2022
Merged

Conversation

remilapeyre
Copy link
Collaborator

No description provided.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Thanks for putting this PR up. There have been a few changes since the alpha which will necessitate some changes but overall, almost everythingin the PR looks correct to me.

.github/workflows/test.yaml Outdated Show resolved Hide resolved
consul/data_source_consul_peering.go Outdated Show resolved Hide resolved
consul/data_source_consul_peering.go Show resolved Hide resolved
vendor/github.com/google/go-cmp/cmp/compare.go Outdated Show resolved Hide resolved
consul/data_source_consul_peering_test.go Show resolved Hide resolved
consul/data_source_consul_peerings_test.go Show resolved Hide resolved
consul/resource_consul_peering.go Outdated Show resolved Hide resolved
consul/resource_consul_peering.go Show resolved Hide resolved
consul/resource_consul_peering_token.go Outdated Show resolved Hide resolved
Comment on lines 24 to 29
Read: func(*schema.ResourceData, interface{}) error {
return nil
},
Delete: func(*schema.ResourceData, interface{}) error {
return nil
},
Copy link
Member

Choose a reason for hiding this comment

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

hmm, so peering token create will behind the scenes create a peering that can be read or deleted. The read and delete functions should be able to reuse what you created for the consul_peering resource. Without support for deletions, you could not use the tf provider to manage peerings without leaking the accepting side of the peering relationship.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @mkeeler, I'm not sure what else could do here, there's support for deleting a peering connection at https://github.com/remilapeyre/terraform-provider-consul/blob/peering/consul/resource_consul_peering.go#L163-L173 but the Consul API does not provide a way to invalid a Peering token that has not yet been used to established a connection.

Support for doing this with the Consul API would be needed if we want to implement a meaningful Delete() for this resource.

Copy link
Member

Choose a reason for hiding this comment

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

It does work but the peering APIs are a bit asymmetrical:

➜  curl -X POST localhost:8500/v1/peering/token -d '{"PeerName": "foo"}'
{
    "PeeringToken": "eyJDQSI6bnVsbCwiU2VydmVyQWRkcmVzc2VzIjpbIjEyNy4wLjAuMTo4NTAyIl0sIlNlcnZlck5hbWUiOiJzZXJ2ZXIuZGMxLmNvbnN1bCIsIlBlZXJJRCI6IjRmZmJkMzljLWZkYzYtOThkOC1kMWE3LWU3MjM3ZTEyMjllOSIsIkVzdGFibGlzaG1lbnRTZWNyZXQiOiJmYTE0M2Y1MS1kNDU1LThiYWQtNWM5Yi0yMDFhZmY2NTk4YzQifQ=="
}

~
➜  curl localhost:8500/v1/peerings
[
    {
        "ID": "4ffbd39c-fdc6-98d8-d1a7-e7237e1229e9",
        "Name": "foo",
        "Partition": "default",
        "State": "PENDING",
        "ImportedServiceCount": 0,
        "ExportedServiceCount": 0,
        "CreateIndex": 18,
        "ModifyIndex": 18
    }
]

~
➜  curl -X DELETE localhost:8500/v1/peering/foo

~
➜  curl localhost:8500/v1/peerings
[]

Basically issuing a request to either POST /v1/peering/token or POST /v1/peering/establish will create a peering which can be deleted with a DELETE /v1/peering/<peer name>.

Copy link
Member

@mkeeler mkeeler Sep 21, 2022

Choose a reason for hiding this comment

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

Similarly you can still read an individual peering that has a token generated for it but the stream is not yet established:

➜  curl localhost:8500/v1/peering/foo
{
    "ID": "947b29ca-8dcf-1d61-1da8-0defeda6e0f0",
    "Name": "foo",
    "Partition": "default",
    "State": "PENDING",
    "ImportedServiceCount": 0,
    "ExportedServiceCount": 0,
    "CreateIndex": 27,
    "ModifyIndex": 27
}

The PENDING state here indicates that the stream is not established.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @mkeeler, all should be good now.

remilapeyre added a commit that referenced this pull request Aug 28, 2022
I'm not updating the docs in this pull request as I will switch the
whole provider to use terraform-plugin-docs in
#309. I will
update the documentation after merging this PR.

Closes #317
@remilapeyre remilapeyre merged commit 3f2a7e4 into hashicorp:master Sep 23, 2022
@remilapeyre remilapeyre deleted the peering branch September 23, 2022 21:42
remilapeyre added a commit that referenced this pull request Dec 13, 2022
I'm not updating the docs in this pull request as I will switch the
whole provider to use terraform-plugin-docs in
#309. I will
update the documentation after merging this PR.

Closes #317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants