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

SUMO-248024: #691

Merged
merged 8 commits into from
Sep 26, 2024
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
DEPRECATIONS:
* resource_sumologic_ingest_budget : Deprecated in favour of `resource_sumologic_ingest_budget_v2`.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a little strange to be doing a 2.x release while having the 3.0.0 section in the CHANGELOG, with content in it already. But I think it's ok.

(Also, an (Unreleased) on the 2.31.5 line would be appropriate, but there's no need to change it; one of us will be editing that line with the correct date anyway.)

Copy link
Collaborator Author

@namangoya namangoya Sep 24, 2024

Choose a reason for hiding this comment

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

Tbh, I thought that 3.0.0 is written for some other purpose.. and didn't thought of using that, just continued after 2.31.3 ..

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dagould are you planning to maintain two versions of the provider? I would be in favor of just continuing 3.x.x version.

## 2.31.5 (September 23, 2024)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove date. Date shall be added on the day of release

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, done

ENHANCEMENTS:
* Added *index_id* attribute to sumologic_scheduled_view. (GH-691)
* Added support for configuring sumologic_data_forwarding_rule for sumologic_scheduled_view. (GH-691)

## 2.31.4 (September 19, 2024)
* **New Resource:** sumologic_data_forwarding_destination (GH-678)
* **New Resource:** sumologic_data_forwarding_rule (GH-688)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ func resourceSumologicDataForwardingDestination() *schema.Resource {
"enabled": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
"s3_region": {
Type: schema.TypeString,
Expand Down
2 changes: 0 additions & 2 deletions sumologic/resource_sumologic_data_forwarding_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ func resourceSumologicDataForwardingRule() *schema.Resource {
"enabled": {
Type: schema.TypeBool,
Optional: true,
Default: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a change in behavior. A rule will be disabled by default now? This might be a breaking change. The good part is we are scheduled for a major release if this is a breaking change it should be fine. We should document the new behavior.

Copy link
Collaborator Author

@namangoya namangoya Sep 24, 2024

Choose a reason for hiding this comment

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

Amm, no it's not a breaking change. Because in the API, we don't enforce any default value for enabled, which means that it'll be treated as false if left blank.

Here, I'm correcting a mistake from my previous PR (#688) .. and and bringing it in-line with the API.

},
"file_format": {
Type: schema.TypeString,
Optional: true,
Default: "{index}_{day}_{hour}_{minute}_{second}",
},
"payload_schema": {
Type: schema.TypeString,
Expand Down
4 changes: 2 additions & 2 deletions sumologic/resource_sumologic_data_forwarding_rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func TestAccSumologicDataForwardingRule_basic(t *testing.T) {
testAccCheckDataForwardingRuleExists(),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "index_id", indexId),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "destination_id", destinationId),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "enabled", "true"),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "enabled", "false"),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "file_format", "test/"),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "payload_schema", "builtInFields"),
resource.TestCheckResourceAttr(dataForwardingRuleResourceName, "format", "json"),
Expand Down Expand Up @@ -227,7 +227,7 @@ func testAccSumologicDataForwardingRuleUpdateConfig() string {
resource "sumologic_data_forwarding_rule" "test" {
index_id = "%s"
destination_id = "%s"
enabled = true
enabled = false
file_format = "test/"
payload_schema = "builtInFields"
format = "json"
Expand Down
7 changes: 7 additions & 0 deletions sumologic/resource_sumologic_scheduled_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func resourceSumologicScheduledView() *schema.Resource {
ForceNew: true,
ValidateFunc: validation.IsRFC3339Time,
},
"index_id": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"retention_period": {
Type: schema.TypeInt,
Optional: true,
Expand Down Expand Up @@ -76,6 +81,7 @@ func resourceSumologicScheduledViewCreate(d *schema.ResourceData, meta interface
}

d.SetId(createdSview.ID)
d.Set("index_id", createdSview.IndexId)
d.Set("retention_period", createdSview.RetentionPeriod)
}

Expand All @@ -99,6 +105,7 @@ func resourceSumologicScheduledViewRead(d *schema.ResourceData, meta interface{}
return nil
}

d.Set("index_id", sview.IndexId)
d.Set("query", sview.Query)
d.Set("index_name", sview.IndexName)
d.Set("start_time", sview.StartTime.Format(time.RFC3339))
Expand Down
2 changes: 1 addition & 1 deletion sumologic/sumologic_data_forwarding_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (s *Client) DeleteDataForwardingRule(indexId string) error {
type DataForwardingRule struct {
IndexId string `json:"indexId"`
DestinationId string `json:"destinationId"`
Enabled bool `json:"enabled,omitempty"`
Enabled bool `json:"enabled"`

Choose a reason for hiding this comment

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

Why are we making this change? This will send value as false instead of not serialising it in json response(which eventually gets treated as null)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bad part is that enabled = false is treated as null.... and whenever we are trying to set false, it is not sent in the API call...

We are not setting any default for enabled .. so it will not be send as false in case left blank (see here).

I've tested this scenario..

FileFormat string `json:"fileFormat,omitempty"`
PayloadSchema string `json:"payloadSchema,omitempty"`
Format string `json:"format,omitempty"`
Expand Down
1 change: 1 addition & 0 deletions sumologic/sumologic_scheduled_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func (s *Client) UpdateScheduledView(sview ScheduledView) error {

type ScheduledView struct {
ID string `json:"id,omitempty"`
IndexId string `json:"indexId,omitempty"`
Query string `json:"query"`
IndexName string `json:"indexName"`
StartTime time.Time `json:"startTime"`
Expand Down
4 changes: 3 additions & 1 deletion website/docs/r/data_forwarding_destination.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ resource "sumologic_data_forwarding_destination" "example_data_forwarding_destin
access_key_id = "accessKeyId"
secret_access_key = "secretAccessKey"
role_arn = "arn:aws:iam::some-valid-arn"
encrypted = "false"
encrypted = false
enabled = true
}
```
## Argument reference
Expand All @@ -35,6 +36,7 @@ The following arguments are supported:
- `secret_access_key` - (Optional) The AWS Secret Key to access the S3 bucket.
- `role_arn` - (Optional) The AWS Role ARN to access the S3 bucket.
- `encrypted` - (Optional) Enable S3 server-side encryption.
- `enabled` - (Optional) True when the data forwarding destination is enabled. Will be treated as _false_ if left blank.

The following attributes are exported:

Expand Down
52 changes: 44 additions & 8 deletions website/docs/r/data_forwarding_rule.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,61 @@ description: |-
Provider to manage [Sumologic Data Forwarding Rule](https://help.sumologic.com/docs/manage/data-forwarding/amazon-s3-bucket/#forward-datato-s3)

## Example Usage

For Partitions
```hcl
resource "sumologic_partition" "test_partition" {
name = "testing_rule_partitions"
routing_expression = "_sourcecategory=abc/Terraform"
is_compliant = false
retention_period = 30
analytics_tier = "flex"
}

resource "sumologic_data_forwarding_rule" "example_data_forwarding_rule" {
index_id = "00000000024C6155"
destination_id = "00000000000732AA"
enabled = "true"
file_format = "test/{index}/{day}/{hour}/{minute}"
payload_schema = "builtInFields"
format = "json"
index_id = sumologic_partition.test_partition.id
destination_id = "00000000000732AA"
enabled = true
file_format = "test/{index}/{day}/{hour}/{minute}"
payload_schema = "builtInFields"
format = "json"
}
```
For Scheduled Views
```hcl
resource "sumologic_scheduled_view" "failed_connections" {
index_name = "failed_connections"
query = "_sourceCategory=fire | count"
start_time = "2024-09-01T00:00:00Z"
retention_period = 1
lifecycle {
prevent_destroy = true
ignore_changes = [index_id]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not needed now?

Copy link
Collaborator Author

@namangoya namangoya Sep 26, 2024

Choose a reason for hiding this comment

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

Yeah I have just kept if for the fact that if someone explicitly tries to set index_id = "000000ABCDE..." (some random value) in the resource definition, then with the help of ignore_changes that will be ignored.

}
}

resource "sumologic_data_forwarding_rule" "test_rule_sv" {
index_id = sumologic_scheduled_view.failed_connections.index_id
destination_id = sumologic_data_forwarding_destination.test_destination.id
Comment on lines +45 to +47

Choose a reason for hiding this comment

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

What happens when user supplies both IndexName and IndexId ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IndexId will simply be ignored ... as IndexId is never taken into consideration in PUT/POST calls .. please check the API documentation here

Copy link
Collaborator Author

@namangoya namangoya Sep 23, 2024

Choose a reason for hiding this comment

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

We are just using IndexId for creating the DF Rule... it is never updated in the PUT call, nor it is ever required to maintain the SV resource's state.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IndexId will simply be ignored .

This behavior should be documented in the terraform docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we are documenting it in the SV page itself as it belongs to SV.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the docs in the latest commit.

enabled = false
file_format = "test/{index}"
payload_schema = "raw"
format = "text"
}
```
## Argument reference

The following arguments are supported:

- `index_id` - (Required) The _id_ of the Partition or Scheduled View the rule applies to.
- `index_id` - (Required) The *id* of the Partition or *index_id* of the Scheduled View the rule applies to.
- `destination_id` - (Required) The data forwarding destination id.
- `enabled` - (Required) True when the data forwarding rule is enabled.
- `enabled` - (Optional) True when the data forwarding rule is enabled. Will be treated as _false_ if left blank.

Choose a reason for hiding this comment

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

Should we add possible values for file_format here to aid customers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not needed, we already have a descriptive documentation ... and that is added on top of this page - see here

Copy link
Collaborator

@sumovishal sumovishal Sep 24, 2024

Choose a reason for hiding this comment

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

Not needed

I disagree, we should cover the possible values here. Can't rely on an example to do that.

Copy link
Collaborator Author

@namangoya namangoya Sep 25, 2024

Choose a reason for hiding this comment

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

Shall I update this Rule's TF documentation and add a pointer to the original documentation here (point 6) in that case?

Possible values for file_format are a combination of few placeholders, and adding all of them here will not look good in the TF docs imho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the docs in the latest commit.

- `file_format` - (Optional) Specify the path prefix to a directory in the S3 bucket and how to format the file name.
- `payload_schema` - (Optional) Schema for the payload. Default value of the payload schema is _allFields_ for scheduled view, and _builtInFields_ for partition.
_raw_ payloadSchema should be used in conjunction with _text_ format and vice versa.
- `format` - (Optional) Format of the payload. Default format will be _csv_.
_text_ format should be used in conjunction with _raw_ payloadSchema and vice versa.

The following attributes are exported:

- `id` - The Index ID of the data_forwarding_rule
2 changes: 2 additions & 0 deletions website/docs/r/scheduled_view.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ QUERY
retention_period = 365
lifecycle {
prevent_destroy = true
ignore_changes = [index_id]
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 add a comment here explaining why we need to add index_id to ignore_changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added in this same doc a little below at line 48.

}
}
```
Expand All @@ -44,6 +45,7 @@ The following arguments are supported:
The following attributes are exported:

- `id` - The internal ID of the scheduled view.
- `index_id` - The Index ID of the scheduled view. It never updates at any point of time during resource updates, therefore make sure to ignore this via `ignore_changes = [index_id]`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a gap in the API. index_id is a write-only field and it never updates after the initial creation. This behavior should have been enforced in the API rather than relying on users to not update index_id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is already enforced in the API (PUT and POST api docs). Here we are just communicating/informing customers as it is a non-updatable field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if the user passes index_id, it will simply be ignored at the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if the user passes index_id, it will simply be ignored at the API.

I don't think this is right behavior. We are silently dropping a field provided by the client. Instead we should return error b/c request is invalid as you are not allowed to change the index id. This is the real issue here.

It should be noted that ignore_changes is not strictly required if you don't change the index id which I believe most customer won't do, given we don't support it?

Copy link
Collaborator Author

@namangoya namangoya Sep 24, 2024

Choose a reason for hiding this comment

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

It should be noted that ignore_changes is not strictly required if you don't change the index id which I believe most customer won't do, given we don't support it?

Yes, not a strict requirement to add ignore_changes .. but recommended, and hence conveying that msg in the documentation here.

I don't think this is right behavior. We are silently dropping a field provided by the client. Instead we should return error b/c request is invalid as you are not allowed to change the index id. This is the real issue here.

  • This will not even reach the API layer as it'll be dropped by OpenAPI translation itself and will not be translated into the request payload's Java Object.
  • Moreover, there can be some invalid fields added by the user (by mistake or intentionally), so those are also simply ignored and not translated by OpenAPI...
  • Therefore, we can't really do much with this index_id field being ignored.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not even reach the API layer as it'll be dropped by OpenAPI translation itself and will not translated into the request payload's Java Object.

We can follow-up internally on this one. I don't see a reason why OpenAPI would silently drop a field.

Yes, not a strict requirement to add ignore_changes .. but recommended, and hence conveying that msg in the documentation here.

The language should be updated to reflect that. Currently, it seems we are asking to add ignore_changes rather than a recommendation.

Copy link
Collaborator Author

@namangoya namangoya Sep 24, 2024

Choose a reason for hiding this comment

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

We can follow-up internally on this one. I don't see a reason why OpenAPI would silently drop a field.

Sure, can I know the set of people who can help me here?

Also, is it a blocker for this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The language should be updated to reflect that. Currently, it seems we are asking to add ignore_changes rather than a recommendation.

But without ignore_changes, we are running into this issue.. let's continue for this on the slack thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But without ignore_changes, we are running into this issue.. let's continue for this on the slack thread.

Issue is resolved with the introduction of computed prop for index_id. (see thread)

I'm updating the documentation that ignore_changes is just a recommendation and not a mandatory thing.


## Import
Scheduled Views can can be imported using the id. The list of scheduled views and their ids can be obtained using the Sumologic [scheduled views api][2].
Expand Down
Loading