Resources for Custom TLS and Platform TLS products #364

bengesoff · 2021-02-08T15:24:17Z

4 new resources:

fastly_tls_private_key
fastly_tls_certificate
fastly_tls_platform_certificate
fastly_tls_activation

10 new data sources

fastly_tls_activation
fastly_tls_activation_ids
fastly_tls_certificate
fastly_tls_certificate_ids
fastly_tls_configuration
fastly_tls_configuration_ids
fastly_tls_platform_certificate
fastly_tls_platform_certificate_ids
fastly_tls_private_key
fastly_tls_private_key_ids

This PR depends on the related go-fastly SDK PR (fastly/go-fastly#259) and will need its go.mod updating once it gets merged. (Currently uses a replace directive to point to the OC fork).

The documentation has been updated to use the new format, thanks to the upstream PRs #356 and #362. This carries with it the same caveats as noted elsewhere with regards to the generated documentation for nested schemas.

There were a few tweaks added on top of the docs generation script. In particular I added some logic to the Makefile to install tfplugindocs to a project-local ./bin directory due to the volatility in the generated documentation. I also removed the module caching from the github workflow because the tool was installed from the vendor directory which was already inside the git repo.

The Makefile had a command added for sweepers as well.

Also add sweepers for TLS certificates and private keys to easily clean up resources leaked during any failed tests.

Terraform testing style guide seems to suggest camel case is used for the main test name then an underscore separates different variations of it.

Main changes were moving docs generation to tfplugindocs, and updating the go-fastly SDK to v3. I added some changes to the upstream docs generation to avoid having to globally install tfplugindocs. This was also done upstream so I had to do some large merge conflict resolution in this commit to combine the similar but different updates. One commit message related to vendoring tfplugindocs was: > Don't cache dependencies in github PR workflow, instead rely on /vendor > > Including the tfplugindocs module in vendor means it's updating with `go > mod vendor` along with the other libraries used. When running `go > install`, this vendored copy is used, and installed to a project-local > /bin directory. This enables the version of tfplugindocs used to be > independent of other go projects installed on one's system. > > This change means `make dependencies` is no longer used, and isn't > needed in the github PR workflow. Furthermore, the source code for the > tool is included in the /vendor already so the caching of ~/go/* isn't > required either.

A couple naming/structure things resulting from different people writing the code. Have just tidied them up before PRing.

…#353) * Add TLSCLientCert and TLSClientKey options for splunk logging * Add some comments to clarify the usage splunk test tls cert values * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go * Update fastly/block_fastly_service_v1_splunk_test.go Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>

…custom

fastly/validators.go

go.mod

bengesoff · 2021-02-08T15:34:16Z

docs/data-sources/tls_configuration.md

+- **updated_at** (String) Timestamp (GMT) when the configuration was last updated.
+
+<a id="nestedatt--dns_records"></a>
+### Nested Schema for `dns_records`


Same issue as @Integralist raised previously (hashicorp/terraform-plugin-docs#28) - no description on nested schema. I've left it autogenerated because I didn't think the descriptions were too critical, it is reasonably self-explanatory what the fields are for, and it seemed easier to maintain by leaving it autogenerated with the rest of the docs.

The intermediates_blob field of the fastly_tls_platform_certificate resource can contain PEM blocks representing an arbitrary length chain of certificates. The validation function for this field has been updated to reflect this. It now loops through the provided string and checks that each block it finds matches the expected block type until it reaches the end of the string. Similarly the validation function for one single block has been updated to fail if the string contains more than one PEM block.

Was only used in creation function but should have also been used in update too.

Also removed the `replace` directive in the go.mod to remove dependency on opencredo fork.

Integralist

@bengesoff this PR looks great! I've left some questions and minor suggestions. Let me know your thoughts when you get a chance.

scripts/generate-docs.go

Integralist · 2021-02-10T17:57:54Z

scripts/generate-docs.go

@@ -74,6 +74,46 @@ func main() {
 			name: "ip_ranges",
 			path: tempDir + "/data-sources/ip_ranges.md.tmpl",
 		},
+		{


It would be nice now that I think about it for these slices to be dynamically populated rather than having to remember to update the script every time a new resource is added.

But I wouldn't worry too much about it for this PR.

Yeah that could be a nice enhancement - scanning the directories or something

Integralist · 2021-02-15T12:29:02Z

.github/workflows/pr.yml

-    - name: Restore cached binaries
-      id: cache
-      uses: actions/cache@v2
-      with:
-        path: ~/go/bin
-        key: ${{ runner.os }}-go-bin-${{ hashFiles('**/go.sum') }}
-        restore-keys: |
-          ${{ runner.os }}-go-bin-
-    - name: Restore cached modules dependencies
-      uses: actions/cache@v2
-      with:
-        path: ~/go/pkg/mod
-        key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }}
-        restore-keys: |
-          ${{ runner.os }}-go-mod-
-    - name: Install dependencies
-      if: steps.cache.outputs.cache-hit != 'true'
-      run: make dependencies
-      shell: bash


Any explanation you can give me for why these segments of the workflow have been deleted?

Yes this change tied in with some of the other changes relating to the way tfplugindocs was being installed. I think previously the makefile was installing the package to GOROOT (i.e. ~/go/pkg and ~/go/bin), which would explain the steps in the workflow caching these directories.

However when adapting the upstream docs generation changes I found that the output of the tfplugindocs tool was quite volatile, given its alpha status, and thought it made sense to use the vendor style dependency management to pin in the exact version of the source code used to generate the docs and reduce disparity between all of the contributor's versions of it. In a similar vein I changed it to install the binary to a project-local ./bin directory to isolate it in case other terraform plugin projects were using different versions of it on anyone's machine. (I think you also made some very similar changes in parallel, e.g. adding a tools.go to allow go mod to find it, so I ended up merging my tweaks into that as well).

Anyway long story short, with the source code being installed to ./vendor and the compiled tfplugindocs binary being installed to ./bin, I didn't see a need to manually cache the external global GOROOT directories as the GitHub workflow does. (Also it had stopped working for me when I removed the make dependencies target, so I had to fix it somehow! 😉)

Does this make sense do you think? Happy to jump on a call and discuss it further if you think that would be helpful

No that's fine. Thanks for the background information 👍🏻

.gitignore

Integralist · 2021-02-15T12:48:54Z

GNUmakefile

-
-install-tools: dependencies
+BIN=$(CURDIR)/bin
+$(BIN)/%:


Does this need to be a dynamic target? Could the target still be named install-tools and generate-docs to reference it? My thinking is that in the updated generate-docs we're still adding $BIN to $PATH so the go install'ed tools will still be located there.

I feel like the dynamic target name (and specifically setting $(BIN)/tfplugindocs) is too much generalization without any real value? Although I could be mis-understanding the purpose for this refactor entirely.

This was related to the same change as explained in the comment on .github/workflows/pr.yml. As part of moving the installation of tfplugindocs to be inside the terraform-provider-fastly directory, I used the non-PHONY target to avoid needing to re-run it every time, if the binary already exists. By extension, the dynamic target seemed useful in case other dependencies were added, given that the grep/awk command was already capable of installing the whole tools.go file.

That's the rationale anyway - I'm happy to change it to use a PHONY install-tools target if you'd prefer, and if I'm not mistaken that would rebuild the tool on each invocation, which could be helpful if it were upgraded? That's assuming that you agree installing to a project-local terraform-provider-fastly/bin directory is a good idea anyway, which you might not - also ok!

OK that makes sense. I think avoiding reinstalling the tool every time is better any way and we can suffer not being on the bleeding edge all the time.

Integralist · 2021-02-16T09:54:36Z

fastly/data_source_fastly_tls_configuration_ids_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: testAccFastlyAccDataSourceTLSConfigurationIDs,
+				Check:  resource.TestCheckResourceAttrSet("data.fastly_tls_configuration_ids.subject", "ids.#"),


I presume this is just validating that there are zero elements within the ids list, is my understanding correct?

I think it's just validating that the ids field has an attribute for its length and therefore loaded without error, not that it is specifically zero. There's not really anything interesting to test here because we don't know what configurations, if any, might be present and we can't create one and look for it, as we do in the other resources (eg activations)

fastly/provider.go

fastly/data_source_fastly_tls_private_key_ids.go

fastly/data_source_fastly_tls_private_key.go

fastly/resource_fastly_tls_activation_test.go

- removal of unneeded .gitignore entry - removal of superfluous whitespace in docs example block - conversion of TypeList to TypeSet in plural data sources' `ids` field - a couple typo fixes here and there - removal of Set function for controlling set hashing, unneeded - consolidation of function naming to include "Fastly" before resource name - fix some acctest.RandomWithPrefix with duplicate prefix - clarify some comments - add some checks in testAcc.*Exists functions when accessing the map of resources in state to avoid a panic if resource not found

bengesoff · 2021-02-16T17:42:14Z

@Integralist I have fixed&resolved some comments, and replied to the rest. Thanks for the feedback, caught some things that had slipped through and raised some good points!

Add a -tfplugindocsPath command line argument to the parsing script to make it a bit more robust than dynamically setting the PATH variable in the Makefile. Defaults to local bin, as the Makefile expects, but I still set the argument in the Makefile in case someone modifies the BIN variable.

Integralist

@bengesoff LGTM.

I'll get this merged and then if you could let me know once you've rebased the latest master into your #365 PR.

Thanks!

bengesoff and others added 24 commits February 8, 2021 12:30

TLS Custom Private Key Resource

94a4df6

TLS Configuration Data Source

1f75279

TLS Custom Certificate Resource

bfc483f

TLS Custom Private Key Data Source

4f3f9a6

TLS Custom Certificate Data Source

8a4ba5f

Also add sweepers for TLS certificates and private keys to easily clean up resources leaked during any failed tests.

TLS Activation Resource

f47fa90

TLS Activation Data Source

1b74e16

TLS Private Key IDs Data Source

444c610

TLS Custom Certificate IDs Data Source

d2dcebb

TLS Configuration IDs Data Source

73deb71

TLS Activation IDs Data Source

7594f3a

Change test names to use underscore

f9dc6d2

Terraform testing style guide seems to suggest camel case is used for the main test name then an underscore separates different variations of it.

Add dns_records attribute to TLS Configuration Data Source

67499dd

TLS Platform Certificate Resource

b88fca1

Update go-fastly reference based on merged PR

31acfc6

TLS Platform Certificate Data Source

1eb8f3a

TLS Platform Certificate IDs Data Source

e94768b

Platform TLS Data Sources documentation

a917559

Remove redundant id filter function

23f082d

Stylistic tweaks to make resources more aligned

0d68aec

A couple naming/structure things resulting from different people writing the code. Have just tidied them up before PRing.

Update go-fastly reference

a6a117a

Merge remote-tracking branch 'upstream/master' into tls-platform-and-…

12afcea

…custom

bengesoff commented Feb 8, 2021

View reviewed changes

fastly/validators.go Outdated Show resolved Hide resolved

bengesoff commented Feb 8, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

bengesoff commented Feb 8, 2021

View reviewed changes

bengesoff mentioned this pull request Feb 8, 2021

Managed TLS Subscriptions Resources #365

Merged

bengesoff added 2 commits February 9, 2021 10:42

Use allow_untrusted_root in platform certificate update function

b14967f

Was only used in creation function but should have also been used in update too.

Update go-fastly reference to v3.3.0

9ff3b8f

Also removed the `replace` directive in the go.mod to remove dependency on opencredo fork.

Integralist reviewed Feb 16, 2021

View reviewed changes

bengesoff force-pushed the tls-platform-and-custom branch from 9290935 to bd3fa5c Compare February 17, 2021 09:14

bengesoff force-pushed the tls-platform-and-custom branch from bd3fa5c to 255b34b Compare February 17, 2021 09:41

Integralist approved these changes Feb 17, 2021

View reviewed changes

Integralist merged commit 748a103 into fastly:master Feb 17, 2021

Integralist added the enhancement New feature or request label Feb 18, 2021

trentrosenbaum deleted the tls-platform-and-custom branch March 15, 2021 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resources for Custom TLS and Platform TLS products #364

Resources for Custom TLS and Platform TLS products #364

bengesoff commented Feb 8, 2021

bengesoff Feb 8, 2021

Integralist left a comment

Integralist Feb 10, 2021

Integralist Feb 10, 2021

bengesoff Feb 16, 2021

Integralist Feb 15, 2021

bengesoff Feb 16, 2021

Integralist Feb 16, 2021

Integralist Feb 15, 2021

bengesoff Feb 16, 2021

Integralist Feb 16, 2021

Integralist Feb 16, 2021

bengesoff Feb 16, 2021

bengesoff commented Feb 16, 2021

Integralist left a comment

Resources for Custom TLS and Platform TLS products #364

Resources for Custom TLS and Platform TLS products #364

Conversation

bengesoff commented Feb 8, 2021

Choose a reason for hiding this comment

Integralist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengesoff commented Feb 16, 2021

Integralist left a comment

Choose a reason for hiding this comment