Add Splunk HEC p0 Integration #55
Conversation
…nto dominickhing/eng-3409-terraform-install-splunk-hec-p0-integration
| @@ -28,14 +23,6 @@ const ( | |||
| // Order matters here; components installed in this order. | |||
| var InstallSteps = []string{Verify, Config} | |||
|
|
|||
| func OperationPath(base_path string, step string) string { | |||
There was a problem hiding this comment.
These were currently being unused
| @@ -14,10 +13,6 @@ import ( | |||
| const ( | |||
| Config = "configure" | |||
| Verify = "verify" | |||
| IamAssessment = "iam-assessment" | |||
There was a problem hiding this comment.
Moved these to internal/provider/resources/install/resources.go under the installresources package.
| @@ -1,8 +1,7 @@ | |||
| package installresources | |||
| package common | |||
There was a problem hiding this comment.
package common contains general Install, RootInstall, and reporting utilities (ReportConversionError), as well as some constants that are generally used throughout
There was a problem hiding this comment.
I don't think I fully understand why we had to move the package. Outside of lining up with our backend structure. What is this refactor trying to accomplish?
There was a problem hiding this comment.
I had originally thought that was a good enough reason. I guess in part it was probably a little easier for me to understand, because i interpreted "resources" in this folder structure to refer to p0 "integration resource" as it appears on our integrations app page, rather than the concept of a "terraform resource".
Another thing - I think between when I started this and finished this PR, the Okta stuff got merged in, so with this change it wouldn't really align with our backend structure unless I additionally did another refactor to separate directory integrations into a separate package. So maybe it's just to just undo these specific changes and put everything back into one folder?
There was a problem hiding this comment.
I think if there aren't any other objections, we can keep this for now and revisit it later if we find that it isn't working out for some reason.
There was a problem hiding this comment.
I think the new structure makes sense. But yes, we should do the Okta refactor.
There was a problem hiding this comment.
I don't have any objection, thank you for the clarity. I'd personally prefer to separate these types of refactors from enhancements so that there's less to review but i'm okay with moving forward with this for now.
| Attributes: map[string]schema.Attribute{ | ||
| "state": stateAttribute, | ||
| "token": schema.StringAttribute{ | ||
| Computed: true, |
There was a problem hiding this comment.
The provider automatically generates a random UUID for the token ID that does not need to be states in the .tf file.
|
|
||
| var currentTokenId types.String | ||
| resp.Diagnostics.Append(resp.State.GetAttribute(ctx, path.Root("token"), ¤tTokenId)...) | ||
| if currentTokenId.ValueString() != "" { |
There was a problem hiding this comment.
This condition solves some edge cases around refresh. Specifically if the resource is managed by terraform and the user deletes the token in the UI, then running terraform refresh without this would result in an inconsistent state.
There was a problem hiding this comment.
@varunalla Why is anything returned here if the token is deleted in the UI? Shouldn't this return a 404 or something?
@dhing1024 I forget, what is the state when we do a refresh if there is no token in the UI? Does it delete the resource? Or what state do we end up with?
There was a problem hiding this comment.
I think these conditions + the other conditions may not be necessary anymore when the token is managed by the user instead, especially after adding the token ID via user input and adding the RequiresReplace to that property.
Prior terraform would get some consistency error related to the HEC token clear text (don't remember if it was exactly that or some other issue) if you ran those commands. Now when I tested removing these conditions, I haven't been seen those issues come up again.
| stringvalidator.RegexMatches(UuidRegex, "Token must be a valid UUID"), | ||
| }, | ||
| PlanModifiers: []planmodifier.String{ | ||
| stringplanmodifier.RequiresReplace(), |
There was a problem hiding this comment.
This indicates that the resource should be completely destroyed and replaced if this field changes. I think it does seem to do the right thing even without it, but adding this makes it behave a little bit more like how our UI works.
There was a problem hiding this comment.
@varunalla Thoughts on this?
| if currentTokenId.ValueString() != "" { | ||
| // manually set the token cleartext attribute | ||
| // this is needed because the cleartext token is not returned by the API | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root(HecTokenClearTextKey), plan.HecTokenClearText)...) |
There was a problem hiding this comment.
This value needs to be explicitly put back into the statefile because it is not returned by the API
dhing1024
changed the title
internal/provider/event_collectors/install/splunk/audit_logs.go
Outdated
Show resolved
Hide resolved
…nto dominickhing/eng-3409-terraform-install-splunk-hec-p0-integration
| } | ||
|
|
||
| var api auditLogsApiReadWrite | ||
| var model auditLogsModel |
There was a problem hiding this comment.
Not sure you need model here. I think you can pass in &auditLogsModel to the installer functions.
There was a problem hiding this comment.
I tested this and you still need to declare a placeholder variable in this way to pass into the other functions.
There was a problem hiding this comment.
I think you can do this by typing &auditLogsModel{} but this is fine too 👍
|
|
||
| var currentTokenId types.String | ||
| resp.Diagnostics.Append(resp.State.GetAttribute(ctx, path.Root("token_id"), ¤tTokenId)...) | ||
| if currentTokenId.ValueString() != "" { |
There was a problem hiding this comment.
Wouldn't the token id always be set? Why do you need this check?
| s.installer.Stage(ctx, &resp.Diagnostics, &req.Plan, &resp.State, &api, &model) | ||
| s.installer.UpsertFromStage(ctx, &resp.Diagnostics, &req.Plan, &resp.State, &api, &model) | ||
|
|
||
| var currentTokenId types.String |
There was a problem hiding this comment.
Isn't the token id already in the plan? Can't you just reference it from there?
There was a problem hiding this comment.
You are right, I think this was but is no longer necessary.
|
|
||
| s.installer.UpsertFromStage(ctx, &resp.Diagnostics, &req.Plan, &resp.State, &auditLogsApiReadWrite{}, &auditLogsModel{}) | ||
|
|
||
| var currentTokenId types.String |
There was a problem hiding this comment.
Same question here I guess. Can't you just get the token id from the plan? And do you need the if condition here?
| // manually set the token cleartext attribute | ||
| // this is needed because the cleartext token is not returned by the API | ||
| var currTokenClearText types.String | ||
| resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root(HecTokenClearTextKey), &currTokenClearText)...) |
There was a problem hiding this comment.
Isn't this already in the plan variable?
dhing1024
deleted the
dominickhing/eng-3409-terraform-install-splunk-hec-p0-integration
branch
Adds the Splunk HEC terraform resource.
This reorganizes some of the utilities so that the file structure can match our backend's integration types:
Some of the commonalities are extracted to separate files for reusability, so that's why there are a bunch of small unrelated changes spread out.