-
Notifications
You must be signed in to change notification settings - Fork 26
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 resource for integrations #24
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Integration | ||
|
||
SignalFx supports integrations to ingest metrics from other monitoring systems, connect to Single Sign-On providers, and to report notifications for messaging and incident management. Note that your SignalForm API key must have admin permissions to use the SignalFx integration API. | ||
|
||
## Example Usage | ||
|
||
```terraform | ||
resource "signalform_integration" "pagerduty_myteam" { | ||
provider = "signalform" | ||
name = "PD - My Team" | ||
enabled = true | ||
type = "PagerDuty" | ||
api_key = "1234567890" | ||
} | ||
``` | ||
|
||
## Argument Reference | ||
|
||
* `name` - (Required) Name of the integration. | ||
* `enabled` - (Required) Whether the integration is enabled. | ||
* `type` - (Required) Type of the integration. See the full list at <https://developers.signalfx.com/reference#integrations-overview>. | ||
* `api_key` - (Required for `PagerDuty`) PagerDuty API key. | ||
* `webhook_url` - (Required for `Slack`) Slack incoming webhook URL. | ||
|
||
**Notes** | ||
|
||
This resource does not support all known types of integration. Contributions are welcome to implement more types. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
package signalform | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
|
||
"github.com/hashicorp/terraform/helper/schema" | ||
) | ||
|
||
const ( | ||
INTEGRATION_API_URL = "https://api.signalfx.com/v2/integration" | ||
) | ||
|
||
func integrationResource() *schema.Resource { | ||
return &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"synced": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: true, | ||
Description: "Whether the resource in SignalForm and SignalFx are identical or not. Used internally for syncing.", | ||
}, | ||
"last_updated": &schema.Schema{ | ||
Type: schema.TypeFloat, | ||
Computed: true, | ||
Description: "Latest timestamp the resource was updated", | ||
}, | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
Description: "Name of the integration", | ||
}, | ||
"enabled": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Required: true, | ||
Description: "Whether the integration is enabled or not", | ||
}, | ||
"type": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
Description: "Type of the integration", | ||
ValidateFunc: validateIntegrationType, | ||
}, | ||
"api_key": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Description: "PagerDuty API key", | ||
Sensitive: true, | ||
ConflictsWith: []string{"webhook_url"}, | ||
}, | ||
"webhook_url": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Description: "Slack Incoming Webhook URL", | ||
Sensitive: true, | ||
ConflictsWith: []string{"api_key"}, | ||
}, | ||
}, | ||
|
||
Create: integrationCreate, | ||
Read: integrationRead, | ||
Update: integrationUpdate, | ||
Delete: integrationDelete, | ||
} | ||
} | ||
|
||
func validateIntegrationType(v interface{}, k string) (we []string, errors []error) { | ||
value := v.(string) | ||
allowedWords := []string{"PagerDuty", "Slack"} | ||
for _, word := range allowedWords { | ||
if value == word { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be a good usability improvement to make this check less strict so also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think SFX's own API might actually be case sensitive on this value, so exact match seems correct to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, my fault, that was poorly phrased. It's just a suggestion, though, not an hard blocker at all. |
||
return | ||
} | ||
} | ||
errors = append(errors, fmt.Errorf("%s not allowed; must be one of: %s", value, strings.Join(allowedWords, ", "))) | ||
return | ||
} | ||
|
||
func getPayloadIntegration(d *schema.ResourceData) ([]byte, error) { | ||
integrationType := d.Get("type").(string) | ||
payload := map[string]interface{}{ | ||
"name": d.Get("name").(string), | ||
"enabled": d.Get("enabled").(bool), | ||
"type": integrationType, | ||
} | ||
|
||
switch integrationType { | ||
case "PagerDuty": | ||
payload["apiKey"] = d.Get("api_key").(string) | ||
case "Slack": | ||
payload["webhookUrl"] = d.Get("webhook_url").(string) | ||
} | ||
|
||
return json.Marshal(payload) | ||
} | ||
|
||
func integrationCreate(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*signalformConfig) | ||
payload, err := getPayloadIntegration(d) | ||
if err != nil { | ||
return fmt.Errorf("Failed creating json payload: %s", err.Error()) | ||
} | ||
url := fmt.Sprintf("%s?skipValidation=true", INTEGRATION_API_URL) | ||
|
||
return resourceCreate(url, config.AuthToken, payload, d) | ||
} | ||
|
||
func integrationRead(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*signalformConfig) | ||
url := fmt.Sprintf("%s/%s", INTEGRATION_API_URL, d.Id()) | ||
|
||
return resourceRead(url, config.AuthToken, d) | ||
} | ||
|
||
func integrationUpdate(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*signalformConfig) | ||
payload, err := getPayloadIntegration(d) | ||
if err != nil { | ||
return fmt.Errorf("Failed creating json payload: %s", err.Error()) | ||
} | ||
url := fmt.Sprintf("%s/%s", INTEGRATION_API_URL, d.Id()) | ||
|
||
return resourceUpdate(url, config.AuthToken, payload, d) | ||
} | ||
|
||
func integrationDelete(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*signalformConfig) | ||
url := fmt.Sprintf("%s/%s", INTEGRATION_API_URL, d.Id()) | ||
return resourceDelete(url, config.AuthToken, d) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are not validating the presence of
api_key
for PagerDuty andwebhook_url
for slack ? The check here is only done for integration type but I think it should also perform other necessary checks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation function can only check a single property at a time, so cross-property checks ("if this resource has type=pagerduty, then it must have an apikey") cannot be represented using validation functions. The only way to implement this check (that I can see) is in
getPayloadIntegration
. I suppose it's fine to add validation there, but signalfx itself will also do that validation on the server side immediately after the result ofgetPayloadIntegration
is POSTed to their api, so I don't think there's much safety added by validating in the provider.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjung-stripe sounds good. I think we can still ensure some validation in the schema itself with
ConflictsWith
. This is not really a blocker, but would be nice to have one level of protection here itself.Also, another issue is if we are to make this extendible to other integration, this schema might become a problem. We might have to end up modelling nested schema here for different integration with all the optional fields out there.
Ref (
ConflictsWith
): https://github.com/Yelp/terraform-provider-signalform/blob/master/src/terraform-provider-signalform/signalform/detector.go#L75There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do something like this, but wasn't able to represent it in the schema. Do you have something concrete in mind?
re conflictswith - sure, I can add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could do something like:
In this case each integration now becomes a key. It will be optional, conflictWith other integrations ensuring there is only one integration type is defined. Further, you can now make the attributes such as
webhook_url
orapi_key
required within the integrationsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that approach is that it's using
schema.TypeSet
, which means this is now legal:ie you can specify zero copies of everything. You can't fix this with
ValidateFunc
either, because you'd have to check each of the keys (slack
,pagerduty
, etc) and make sure at least one was nonzero.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I didn't quite understand your previous explanation.
I was thinking of including "slack" like this (axis_right) https://github.com/Yelp/terraform-provider-signalform/blob/master/src/terraform-provider-signalform/signalform/time_chart.go#L115 not as a resource.
You can define the elemTypes you need now and make them required. Further, make slack conflictWith the other integration types you define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the example you indicated has the same problem I'm describing. It's using a TypeSet which means it can be specified zero or more times. All of the following would be legal:
This is why I didn't use typed keys to begin with - because the only way to do so is to specify them on an
Elem
, which requires using a map, list, or set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah! understood what you meant now, Sorry about that.
The project structure more or less follows this structure elsewhere (the one I mentioned above of using TypeSet). I feel that way, the structure is more appropriate than it currently is, considering the number of integrations out there and most of the attributes vary.
signalform-tools is another repository we maintain for validation and we can definitely bake in the functionality there.
This is not really a blocker, feel free to disregard.