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 the Audit Trail API #407

Merged
merged 3 commits into from Jun 10, 2022
Merged

Conversation

sebasslash
Copy link
Contributor

@sebasslash sebasslash commented May 18, 2022

Description

Adds support for the Audit Trail API. One thing to note is that this particular API requires an organization token and does not follow the JSON+API spec as most of our endpoints do. Thusly the implementation of the List() method will create a new retryable http client and send the request using that client -- I figured this would be better than altering the existing client that is shared across our interfaces.

Testing plan

To run the integration test:

go test -v ./... -run TestAuditTrailsList -tags=integration

Output from tests

$  go test -v ./... -run TestAuditTrailsList -tags=integration
=== RUN   TestAuditTrailRead
=== RUN   TestAuditTrailRead/with_no_specified_timeframe
=== RUN   TestAuditTrailRead/with_no_specified_timeframe/with_resource_deserialized_correctly
=== RUN   TestAuditTrailRead/with_no_specified_timeframe/with_auth_deserialized_correctly
=== RUN   TestAuditTrailRead/with_no_specified_timeframe/with_request_deserialized_correctly
=== RUN   TestAuditTrailRead/using_since_query_param
--- PASS: TestAuditTrailRead (2.11s)
    --- PASS: TestAuditTrailRead/with_no_specified_timeframe (0.17s)
        --- PASS: TestAuditTrailRead/with_no_specified_timeframe/with_resource_deserialized_correctly (0.00s)
        --- PASS: TestAuditTrailRead/with_no_specified_timeframe/with_auth_deserialized_correctly (0.00s)
        --- PASS: TestAuditTrailRead/with_no_specified_timeframe/with_request_deserialized_correctly (0.00s)
    --- PASS: TestAuditTrailRead/using_since_query_param (1.51s)
PASS
ok      github.com/hashicorp/go-tfe     2.872s
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]

audit_trail.go Outdated Show resolved Hide resolved
audit_trail.go Outdated Show resolved Hide resolved
@sebasslash sebasslash force-pushed the sebasslash/audit-trail branch 2 times, most recently from 7d7df59 to 8385afb Compare May 23, 2022 19:31
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

I would love to see all this implemented within a new type of Client, not as a part of existing type Client. Naming is tricky but perhaps something like JsonClient or AuditTrailsClient.

Rationale:

  1. The endpoint is incompatible with the existing auth and type headers
  2. The endpoint requires different token config (always an org token, which is rare)

What if the API was more like

auditConfig := DefaultConfig()
auditConfig.Token = orgToken
auditClient, err := NewAuditTrailClient(&auditConfig)

auditTrails, err := auditClient.AuditTrails.List(ctx)

one thing to look at to help with naming is if there is ANY OTHER public API that doesn't use json:api.

It just doesn't feel right to drop to retryablehttp, reusing client, within this method.

audit_trail.go Outdated Show resolved Hide resolved
audit_trail_integration_test.go Outdated Show resolved Hide resolved
client := testClient(t)
ctx := context.Background()

orgToken := os.Getenv("TFE_ORGANIZATION_TOKEN")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you decide that creating an org token would not work? I'm sorry I forgot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we currently have no way in go-tfe to upgrade an organization to the business tier. The integration tests in atlas do in fact upgrade the entitlement set for an org but it's an internal endpoint as I understand it.

I discussed with @taiidani and @annawinkler on potentially configuring tflocal to default any organization that's created to the business plan so we can run the entire test suite. This is non trivial and requires further internal discussion. 👍

Choose a reason for hiding this comment

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

@sebasslash sebasslash force-pushed the sebasslash/audit-trail branch 2 times, most recently from e40193f to 9958a1d Compare May 25, 2022 20:21
@brandonc
Copy link
Collaborator

Is this waiting on #415 ?

@sebasslash sebasslash force-pushed the sebasslash/audit-trail branch 2 times, most recently from 440c399 to 9c36e74 Compare June 2, 2022 16:56
audit_trail.go Outdated Show resolved Hide resolved
audit_trail.go Outdated Show resolved Hide resolved
// AuditTrailListOptions represents the options for listing audit trails.
type AuditTrailListOptions struct {
// Optional: Returns only audit trails created after this date
Since time.Time `url:"since,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand, this looks like it should be a pointer since it is an emptyable struct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The zero value for time.Time{} is 0001-01-01 00:00:00 +0000 UTC so it wouldn't be an issue. The only reason I wouldn't make it a pointer is time.Time isn't really ever used as a pointer.

CHANGELOG.md Outdated
Comment on lines 1 to 7
# v1.3.0

## Enhancements
* Add support for Audit Trail API by @sebasslash [#407](https://github.com/hashicorp/go-tfe/pull/407)

# v1.2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.2.0 is still called Unreleased in main 😱

Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

An orgToken parameter still doesn't sit right with me on the interface API. It's confusing because you already supplied an auth token when you created the client, and it feels like configuration as opposed to a list option.

Can we re-open the argument for having a second 'ComplianceClient' or 'PlatformClient' that consumes json instead of jsonapi and is configured with a separate token?

@@ -66,7 +66,7 @@ This API client covers most of the existing Terraform Cloud API calls and is upd
- [x] Agent Pools
- [x] Agent Tokens
- [x] Applies
- [ ] Audit Trails
- [x] Audit Trails
Copy link
Collaborator

Choose a reason for hiding this comment

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

😎

brandonc
brandonc previously approved these changes Jun 6, 2022
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

I've found that a user token will authenticate, returning an empty List and 200 response.


headers := make(http.Header)
headers.Set("Authorization", "Bearer "+s.client.token)
headers.Set("Content-Type", "application/json")
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should set the user-agent to go-tfe, similar to tfe.go

Copy link
Contributor

Choose a reason for hiding this comment

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

@sebasslash Is this something that still needs to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in L89: headers.Set("User-Agent", _userAgent) where _userAgent is go-tfe

@brandonc brandonc dismissed their stale review June 6, 2022 20:10

I think we should set the user agent on this before merging

brandonc
brandonc previously approved these changes Jun 7, 2022
@sebasslash sebasslash merged commit 599a87f into main Jun 10, 2022
@sebasslash sebasslash deleted the sebasslash/audit-trail branch June 10, 2022 18:57
@github-actions
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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

5 participants