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

chore: update schema #915

Merged
merged 2 commits into from
Sep 17, 2024
Merged

chore: update schema #915

merged 2 commits into from
Sep 17, 2024

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Sep 16, 2024

PR Type

Enhancement, Tests, Dependencies


Description

  • Added Alerting and Contacts configurations for Grafana, including various contact methods such as Email, Pagerduty, Discord, Slack, and Webhook.
  • Updated validation tests to include the new Grafana Alerting and Contacts configurations.
  • Modified local and Docker Compose test configurations to incorporate Grafana Alerting and Contacts.
  • Updated github.com/nhost/be dependency version in go.mod and go.sum.

Changes walkthrough 📝

Relevant files
Enhancement
example.go
Add Grafana Alerting and Contacts Configuration                   

cmd/config/example.go

  • Added Alerting and Contacts configurations for Grafana.
  • Included various contact methods like Email, Pagerduty, Discord,
    Slack, and Webhook.
  • +52/-0   
    config.go
    Add default Grafana Alerting and Contacts configuration   

    project/config.go

    • Added default configuration for Grafana Alerting and Contacts.
    +2/-0     
    Tests
    validate_test.go
    Update validation tests for Grafana configuration               

    cmd/config/validate_test.go

  • Updated expected configuration to include Grafana Alerting and
    Contacts.
  • +8/-1     
    local_test.go
    Add Grafana Alerting and Contacts to local tests                 

    cmd/configserver/local_test.go

  • Added Alerting and Contacts fields to local test configuration for
    Grafana.
  • +2/-0     
    main_test.go
    Update Docker Compose tests with Grafana configuration     

    dockercompose/main_test.go

  • Included Alerting and Contacts in Docker Compose test configuration
    for Grafana.
  • +2/-0     
    Dependencies
    go.mod
    Update nhost/be dependency version                                             

    go.mod

    • Updated github.com/nhost/be dependency version.
    +1/-1     
    go.sum
    Update go.sum for nhost/be dependency                                       

    go.sum

    • Updated checksums for github.com/nhost/be dependency.
    +2/-10   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive Information Exposure:
    The PR includes sensitive information such as passwords and integration keys in the configuration files. These should be managed securely, possibly through environment variables or a secrets management tool.

    ⚡ Key issues to review

    Sensitive Information Exposure
    The Password and IntegrationKey fields contain sensitive information and should be handled securely. Consider using environment variables or a secrets management tool.

    Incomplete Test Coverage
    The new Grafana Alerting and Contacts configurations are added but not fully tested. Ensure all new configurations are covered by tests.

    Copy link
    Contributor

    github-actions bot commented Sep 16, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use environment variables for sensitive information instead of hardcoding

    Consider using environment variables or a secure secret management system for
    sensitive information like integration keys, tokens, and passwords instead of
    hardcoding them in the configuration.

    cmd/config/example.go [446-483]

    -IntegrationKey: "integration-key",
    +IntegrationKey: os.Getenv("PAGERDUTY_INTEGRATION_KEY"),
     ...
    -Token:     "token",
    +Token:     os.Getenv("SLACK_TOKEN"),
     ...
    -Password:                 "password",
    +Password:                 os.Getenv("WEBHOOK_PASSWORD"),
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical security concern by recommending the use of environment variables for sensitive information, which is a best practice for protecting credentials and keys.

    9
    Validate and ensure HTTPS for URL fields

    Consider adding validation for URL fields to ensure they are properly formatted and
    use HTTPS.

    cmd/config/example.go [455-479]

    -Url:       "https://discord.com/api/webhooks/...",
    -AvatarUrl: "https://discord.com/api/avatar/...",
    +Url:       validateHTTPSUrl("https://discord.com/api/webhooks/..."),
    +AvatarUrl: validateHTTPSUrl("https://discord.com/api/avatar/..."),
     ...
    -Url:            "https://slack.com/api/webhooks/...",
    -EndpointURL:    "https://slack.com/api/endpoint/...",
    +Url:            validateHTTPSUrl("https://slack.com/api/webhooks/..."),
    +EndpointURL:    validateHTTPSUrl("https://slack.com/api/endpoint/..."),
     ...
    -Url:                      "https://webhook.example.com",
    +Url:                      validateHTTPSUrl("https://webhook.example.com"),
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves security by ensuring proper URL formatting and HTTPS usage, which is important for maintaining secure communications in the application.

    8
    Enhancement
    Add rate limiting for webhook configurations

    Consider adding a rate limiting mechanism for the webhook to prevent potential abuse
    or overload of the target system.

    cmd/config/example.go [477-487]

     Webhook: []*model.ConfigGrafanacontactsWebhook{
         {
             Url:                      "https://webhook.example.com",
             HttpMethod:               "POST",
             Username:                 "user",
             Password:                 "password",
             AuthorizationScheme:      "Bearer",
             AuthorizationCredentials: "token",
             MaxAlerts:                10,
    +        RateLimit:                &model.RateLimit{
    +            RequestsPerMinute:    60,
    +            BurstSize:            10,
    +        },
         },
     },
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances the robustness of the webhook system by preventing potential abuse or overload, which is a good practice for maintaining system stability and security.

    7

    @dbarrosop dbarrosop merged commit c1a77d0 into main Sep 17, 2024
    8 checks passed
    @dbarrosop dbarrosop deleted the upd-schema branch September 17, 2024 07:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants