Add vulnerability_alerts attribute for repositories #444

jtsaito · 2020-05-03T19:11:32Z

Add support for new attribute vulnerability_alerts for github_repository. GitHub calls this feature as vulnearability-alerts in the API, Security Alerts in the web console, and "security alerts for vulnerable dependencies" in the documentation.

The PR follows up on https://github.com/terraform-providers/terraform-provider-github/pull/382 and addresses https://github.com/terraform-providers/terraform-provider-github/issues/238.

This would introduce changes for some existing repos with vulnerabilities alerts enabled and show a diff accordingly. To address fix the diff, adding vulnerability_alerts = true suffices.

What's slightly weird is that by default GitHub enables this feature on public repos but disables it on private repos.

References

Acc tests
I ran make testacc but only for the new and the basic repo test.

TF_ACC=1 go test github.com/terraform-providers/terraform-provider-github/github -v -run=TestAccGithubRepository_vulnerabilityAlerts -timeout 120m
=== RUN   TestAccGithubRepository_vulnerabilityAlerts
=== PAUSE TestAccGithubRepository_vulnerabilityAlerts
=== CONT  TestAccGithubRepository_vulnerabilityAlerts
--- PASS: TestAccGithubRepository_vulnerabilityAlerts (16.84s)
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	16.870s

TF_ACC=1 go test github.com/terraform-providers/terraform-provider-github/github -v -run=TestAccGithubRepository_basic -timeout 120m
=== RUN   TestAccGithubRepository_basic
=== PAUSE TestAccGithubRepository_basic
=== CONT  TestAccGithubRepository_basic
--- PASS: TestAccGithubRepository_basic (16.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	16.555s

Also manually tested for private repos and that worked as well.

majormoses · 2020-05-03T20:40:22Z

Yay I am excited to see this, its something I have been looking forward to adding to our internal module for a while and I am currently relying on manually managing these on the core repos in my org. Clearly far from ideal as we know there are new projects being created daily.

I think we should consider having this default with a null value and only in the case where the passed value is a boolean we add this to the request. This would effectively maintain Github's current defaults and only evaluate updates when explicitly told to manage this. In both cases defaulting to true or false are technically both breaking changes IMO as they realistically require you to take a stance to maintain existing "status quo" updates. Either way the expectation for what should happen will have changed. If we decide we want to always control the default then I think we need to be clear about calling out the impact for users and ensure this is properly versioned as a major release. We can also release with a null default now and later take the stance on if we should manage the default for this or not.

I would prefer we follow the upstream defaults and encourage organizations to have their own module encapsulating their own opinions, standards, etc.

majormoses · 2020-05-03T20:51:03Z

🤔 I just realized we currently have documented that we support terraform 0.10+ and null was only added in 0.12 so technically that would be breaking as well. I suppose we could treat an empty string as the same thing as null to provide that type of functionality for older terraform? We might also consider dropping < 0.12 in a major version which appears to have already started happening in other core providers: https://www.hashicorp.com/blog/deprecating-terraform-0-11-support-in-terraform-providers/ I would appreciate a maintainer to weigh in when we should look at doing the same as this may affect what we want to do.

anGie44 · 2020-05-03T20:59:12Z

hi @jtsaito, should we add this field to the data source as well?

jtsaito · 2020-05-04T18:54:21Z

@majormoses Thank you for your feedback!

Regarding the null value, the documentation on Terraform expressions states:

If you set an argument of a resource or module to null, Terraform behaves as though you had completely omitted it — it will use the argument's default value if it has one, or raise an error if the argument is mandatory.

As long as the attribute is a boolean the default is either true or false. I think that a boolean is the correct choice here because it reflects exactly the API.

Forcing users to set the value according to the diff is actually helpful because they can be sure their infrastructure as code reflects the state of the API. This precludes ambiguity about the click-opsed state. Setting the actual value based on the diff requires minimal effort even for hundreds of repos a simple script or editor macro should do the job.

The default false seems natural assuming the most common use case is private repositories and disabled alerts.

Please llet me know what you think.

jtsaito · 2020-05-04T18:56:21Z

@anGie44 Sure, if this PR get's accepted I will gladly create another adding the attribute for the datasource!

anGie44 · 2020-05-12T05:29:41Z

github/resource_github_repository.go

@@ -374,6 +405,18 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er
 		}
 	}

+	if d.HasChange("vulnerability_alerts") {


my only suggestion here could be to save an API call after creation of a resource by adding the!d.IsNewResource() since Update immediately follows Create

anGie44 · 2020-05-12T05:53:06Z

github/resource_github_repository.go

+			"vulnerability_alerts": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,


my concern here is, as stated, this default value is only applicable to private repos as initially defined by the API, and a user may unintentionally disable alerts for a public repo by omitting this attribute in a config and making it through the apply step..could we consider leaving this w/o a default or, given the feedback in the original issue, is the general user experience better with having this value in place?

could we consider leaving this w/o a default

👍 my vote goes to removing the default.

anGie44 · 2020-05-12T06:07:36Z

github/resource_github_repository_test.go

@@ -557,6 +631,33 @@ func testAccCheckGithubRepositoryTemplateRepoAttribute(n string, repo *github.Re
 	}
 }

+func testAccCheckGithubVulnerabilityAlerts(n string, expected bool) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[n]


it may be redundant to check w/in the state here as a call to this function in the acceptance tests is preceded by testAccCheckGithubRepositoryExists which checks this for us, so alternatively the logic here can be omitted and the repoName can be determined by the repo object set in the Exists function

anGie44 · 2020-05-12T06:23:55Z

github/resource_github_repository_test.go

+  has_downloads      = true
+  auto_init          = false
+
+	vulnerability_alerts = true


Nit: can be aligned by using terrafmt

anGie44 · 2020-05-12T06:24:13Z

github/resource_github_repository_test.go

+  has_downloads      = true
+  auto_init          = false
+
+	vulnerability_alerts = false


same nit as above

majormoses · 2020-05-12T15:18:23Z

@jtsaito sorry for the delayed response. AFAIK we can use null, set the type to any, make the param not require, and can raise an exception if its !nul, true, or false. My opinion then if we must take a stance on default we should default to true rather than false as it will benefit more people with better secure defaults. If you don't trust github/microsoft you should probably move to another platform. There is an inherent trust and a server admin can 100% get your data if they really wanted to regardless of this setting. As both ways are opinions and I didn't want to get into an argument about it (as there is no winner) by keeping the upstream behavior.

jcudit

Looks good overall! Excited to see this ship after the remaining discussion items are resolved.

jcudit · 2020-05-19T11:33:21Z

github/resource_github_repository.go

+			"vulnerability_alerts": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,


could we consider leaving this w/o a default

👍 my vote goes to removing the default.

jtsaito · 2020-05-20T07:49:23Z

@anGie44 Thanks for the review! Sorry, I did not get around to fixing this earlier. I'll have time to fix this by the end of the week and will remove the default.

jtsaito · 2020-05-25T07:36:32Z

@anGie44 Sorry again for the delay. I hope to now have incorporated all your corrections and suggestions. Please check the latest commits and let me know if I should squash all commits into one.

anGie44

no worries @jtsaito, thanks for pushing your latest changes!

I ran into this test failure:

    TestAccGithubRepository_vulnerabilityAlerts: testing.go:654: Step 0 error: Check failed: Check 3/3 error: Unexpected vulnerability alerts, got: false

But a change in the test's check function as commented below should be the fix

website/docs/r/repository.html.markdown

anGie44 · 2020-05-25T18:19:53Z

github/resource_github_repository_test.go

@@ -83,6 +83,7 @@ func TestAccGithubRepository_basic(t *testing.T) {
 						DefaultBranch:       "master",
 						Archived:            false,
 					}),
+					testAccCheckGithubVulnerabilityAlerts(rn, false),


in calls to testAccCheckGithubVulnerabilityAlerts, we'll want to send the repo name (in this case stored in the repo variable) as needed for the github API method; alternatively you could pass the whole repo object and extract the name inside the testAccCheckGithubVulnerabilityAlerts method

anGie44 · 2020-05-25T18:20:01Z

github/resource_github_repository_test.go

@@ -101,6 +102,7 @@ func TestAccGithubRepository_basic(t *testing.T) {
 						HasProjects:      false,
 						Archived:         false,
 					}),
+					testAccCheckGithubVulnerabilityAlerts(rn, false),


similar comment as above

anGie44 · 2020-05-25T18:31:28Z

I wanted to also note @jtsaito, this feature request https://github.com/terraform-providers/terraform-provider-github/issues/390 brought to light a side-effect of enabling vulnerability alerts via the API as the dependency_graph is also enabled (https://developer.github.com/v3/repos/#enable-vulnerability-alerts). In this case, I think we should document this in website docs as well.

dkatona · 2020-06-23T11:31:37Z

Hi @jtsaito, we are eagerly waiting for this feature to turn on security checking via terraform - is there any way we can help you to finish this PR and get it merged? :) Thank you!

jcudit · 2020-09-13T19:04:20Z

Looks like recent updates to the branch protection plumbing has added conflicts to this PR. I've got this queued to address this week, but welcome anyone else jumping in to remove conflicts.

github/resource_github_repository_test.go

jcudit · 2020-09-18T14:24:37Z

Conflicting tests have been removed and will be re-added in a follow-up commit.

…ions#444)" This reverts commit a4eb4fa.

* Add vulnerability_alerts attribute for repositories * Check change of vulnerability alerts only if new resource * No default value for vulnerability alerts * Remove redundant test code * Update website on repository vulnerability alerts * Add newline * Update website/docs/r/repository.html.markdown Co-authored-by: Jeremy Udit <[email protected]>

Add vulnerability_alerts attribute for repositories

7dd7b9d

ghost added size/L Type: Documentation Improvements or additions to documentation labels May 3, 2020

jtsaito mentioned this pull request May 3, 2020

[Feature Request] support enabling and disabling vulnerability alerts #238

Closed

anGie44 linked an issue May 3, 2020 that may be closed by this pull request

[Feature Request] support enabling and disabling vulnerability alerts #238

Closed

anGie44 self-requested a review May 6, 2020 04:36

anGie44 added this to the v2.8.0 milestone May 8, 2020

anGie44 reviewed May 12, 2020

View reviewed changes

anGie44 requested a review from jcudit May 12, 2020 14:29

anGie44 modified the milestones: v2.8.0, v2.8.1 May 15, 2020

jcudit reviewed May 19, 2020

View reviewed changes

anGie44 mentioned this pull request May 23, 2020

[Feature Request] Add Option Data services Dependency graph #390

Closed

jtsaito added 4 commits May 25, 2020 07:08

Check change of vulnerability alerts only if new resource

244f079

No default value for vulnerability alerts

11651f6

Remove redundant test code

c4916dd

Update website on repository vulnerability alerts

059fa1e

anGie44 suggested changes May 25, 2020

View reviewed changes

jcudit modified the milestones: v2.9.0, v2.10.0 Jun 3, 2020

anGie44 added the Awaiting response label Jun 11, 2020

ghost removed the Awaiting response label Jun 23, 2020

jcudit modified the milestones: v2.10.0, v3.1.0 Jul 10, 2020

Merge branch 'master' into repo-vulnerability-alerts

cb87733

ghost added size/S and removed size/L labels Sep 18, 2020

jcudit reviewed Sep 18, 2020

View reviewed changes

github/resource_github_repository_test.go Outdated Show resolved Hide resolved

Jeremy Udit added 2 commits September 18, 2020 10:22

Add newline

f71e18f

Update website/docs/r/repository.html.markdown

40b17dc

jcudit merged commit a4eb4fa into integrations:master Sep 18, 2020

mallyvai added a commit to mallyvai/terraform-provider-afterpaygithub that referenced this pull request Sep 20, 2020

Revert "Add vulnerability_alerts attribute for repositories (integrat…

f8a15c6

…ions#444)" This reverts commit a4eb4fa.

jcudit mentioned this pull request Sep 22, 2020

Add Vulnerability Alert Fixes & Testing #558

Merged

r-bennett mentioned this pull request Mar 26, 2021

vulnerability-alerts 404 #716

Closed

kfcampbell mentioned this pull request Apr 25, 2021

Explore setting vulnerability alerts correctly #768

Merged

2 tasks

yordis mentioned this pull request Aug 27, 2022

Missing Features Aggregate #379

Closed

10 tasks

EQemccudden mentioned this pull request Nov 3, 2022

Feature: Support for granting access to security alerts #612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vulnerability_alerts attribute for repositories #444

Add vulnerability_alerts attribute for repositories #444

jtsaito commented May 3, 2020 •

edited

Loading

majormoses commented May 3, 2020 •

edited

Loading

majormoses commented May 3, 2020 •

edited

Loading

anGie44 commented May 3, 2020

jtsaito commented May 4, 2020

jtsaito commented May 4, 2020

anGie44 May 12, 2020

anGie44 May 12, 2020 •

edited

Loading

jcudit May 19, 2020

anGie44 May 12, 2020 •

edited

Loading

anGie44 May 12, 2020

anGie44 May 12, 2020

majormoses commented May 12, 2020 •

edited

Loading

jcudit left a comment

jcudit May 19, 2020

jtsaito commented May 20, 2020

jtsaito commented May 25, 2020

anGie44 left a comment

anGie44 May 25, 2020

anGie44 May 25, 2020

anGie44 commented May 25, 2020

dkatona commented Jun 23, 2020

jcudit commented Sep 13, 2020

jcudit commented Sep 18, 2020

Add vulnerability_alerts attribute for repositories #444

Add vulnerability_alerts attribute for repositories #444

Conversation

jtsaito commented May 3, 2020 • edited Loading

majormoses commented May 3, 2020 • edited Loading

majormoses commented May 3, 2020 • edited Loading

anGie44 commented May 3, 2020

jtsaito commented May 4, 2020

jtsaito commented May 4, 2020

anGie44 May 12, 2020

Choose a reason for hiding this comment

anGie44 May 12, 2020 • edited Loading

Choose a reason for hiding this comment

jcudit May 19, 2020

Choose a reason for hiding this comment

anGie44 May 12, 2020 • edited Loading

Choose a reason for hiding this comment

anGie44 May 12, 2020

Choose a reason for hiding this comment

anGie44 May 12, 2020

Choose a reason for hiding this comment

majormoses commented May 12, 2020 • edited Loading

jcudit left a comment

Choose a reason for hiding this comment

jcudit May 19, 2020

Choose a reason for hiding this comment

jtsaito commented May 20, 2020

jtsaito commented May 25, 2020

anGie44 left a comment

Choose a reason for hiding this comment

anGie44 May 25, 2020

Choose a reason for hiding this comment

anGie44 May 25, 2020

Choose a reason for hiding this comment

anGie44 commented May 25, 2020

dkatona commented Jun 23, 2020

jcudit commented Sep 13, 2020

jcudit commented Sep 18, 2020

jtsaito commented May 3, 2020 •

edited

Loading

majormoses commented May 3, 2020 •

edited

Loading

majormoses commented May 3, 2020 •

edited

Loading

anGie44 May 12, 2020 •

edited

Loading

anGie44 May 12, 2020 •

edited

Loading

majormoses commented May 12, 2020 •

edited

Loading