integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
905 stars 746 forks source link

[BUG]: Validation of resource_github_actions_repository_permissions requiring allowed_actions_config is wrong/problematic #2105

Closed Danielku15 closed 8 months ago

Danielku15 commented 9 months ago

Expected Behavior

It should be possible to enable github actions on a repository via resource_github_actions_repository_permissions and allowed_actions="selected" without manually specfiying a allowed_actions_config. The implemented custom validation is incorrect.

And in scenarios where you are not allowed to change the actions list on repository level, you will get an error with further validation errors.

Please allow resource_github_actions_repository_permissions with allowed_actions="selected" and allowed_actions_config unset.

Actual Behavior

This check kicks in and reports an error if you do not specify allowed_actions_config: https://github.com/integrations/terraform-provider-github/blob/5752b2581349fcf4dc7be0c093445591abdfbe91/github/resource_github_actions_repository_permissions.go#L101

There are config variations where the policies are defined on organization/enterprise level and they cannot be customized on repository level. In the UI they are readonly if you select the respective radio button:

image

On REST API level it is possible to control whether GHA is enabled and what actions are allowed without yet specfiying the list yet.

https://docs.github.com/en/rest/actions/permissions?apiVersion=2022-11-28#set-github-actions-permissions-for-a-repository

The list of actions and other policies are part of a separate resource which is not mandatory to be customized per repository:

https://docs.github.com/en/rest/actions/permissions?apiVersion=2022-11-28#set-allowed-actions-and-reusable-workflows-for-a-repository

Terraform Version

Terraform 1.5.6 on Windows Server 2022 CDKTF for .net, NuGet Package: HashiCorp.Cdktf.Providers.Github 12.0.2 Provider Version: 5.42.0 (https://github.com/cdktf/cdktf-provider-github/releases/tag/v12.0.2)

Affected Resource(s)

Terraform Configuration Files

No response

Steps to Reproduce

CDKTF for .net:

_ = new ActionsRepositoryPermissions(terraformStack,
    "actions-permission",
    new ActionsRepositoryPermissionsConfig
    {
        Repository = "my-repo",
        Enabled = true,
        AllowedActions = "selected",
        // Not set:
        // AllowedActionsConfig = new ActionsRepositoryPermissionsAllowedActionsConfig()
    });

Debug Output

No response

Panic Output

No response

Code of Conduct

Danielku15 commented 9 months ago

I shortly tried to rework the related code by removing the validation and add a test, but it seem to have a bit bigger impact:

As GitHub will "copy" the configuration from the Organization or Enterprise level, the Terraform provider downloads this related data which leads to an inconsistency (terraform defining nothing vs. state defining values).

Maybe some more experienced devs from the project know how to handle such scenarios.

AFAIK typically this is solved in TF providers by properly have a 1:1 mapping between Terraform Resources and API endpoints which is not the case here. 1 Resource in TF are 2 resources in the GitHub API.

Any hints are appreciated how this can be tackled as I'd be happy to attempt a PR.

kfcampbell commented 9 months ago

As GitHub will "copy" the configuration from the Organization or Enterprise level, the Terraform provider downloads this related data which leads to an inconsistency (terraform defining nothing vs. state defining values).

Do you mind explaining more about what you mean here? I'm not sure I follow, sorry.

AFAIK typically this is solved in TF providers by properly have a 1:1 mapping between Terraform Resources and API endpoints which is not the case here. 1 Resource in TF are 2 resources in the GitHub API.

In this scenario, would you be suggesting that the allowed_actions_config be its own resource?

Danielku15 commented 9 months ago

Do you mind explaining more about what you mean here? I'm not sure I follow, sorry.

On API level this is happening: the PUT /repos/{owner}/{repo}/actions/permissions/selected-actions is readonly if there are restrictions on enterprise or organization level. Calling it will lead to a 409 CONFLICT response. But you can still call PUT /repos/{owner}/{repo}/actions/permissions with {"enabled":true,"allowed_actions":"selected"} and the repository will inherit the configuration from the organization. Similarly if you enable actions on organization level it will inherit the configuration from the enterprise level.

Here a full example showing the scenario in more detail: 1. Create a new organization and go to Settings > Actions > General 2. Configure it like this: ![image](https://github.com/integrations/terraform-provider-github/assets/674916/4ee7146c-ad5b-4308-96f9-4356e3579c1f) 3. Create a new repository (manually for the sake of clarity) and disable GHA: ![image](https://github.com/integrations/terraform-provider-github/assets/674916/4ea1f426-e49e-4e0f-8166-2676df4b5300) 4. Now we inspect the current actions configuration on API and see that actions are disabled and the there is no `selected-actions` available: ![image](https://github.com/integrations/terraform-provider-github/assets/674916/be093b11-d7a9-4a1d-9c1d-2d2a72bea60c) 5. Now we enable GHA with `allowed_actions="selected"` and can see that the `selected-actions` endpoint delivers the config from the organization: ![image](https://github.com/integrations/terraform-provider-github/assets/674916/7740ef6a-cef5-4727-a820-06a01d679670) 6. But when I try to set the `selected-actions` this will fail: ![image](https://github.com/integrations/terraform-provider-github/assets/674916/df48ed50-3643-475b-9f16-638eb2c93f68)

In this scenario, would you be suggesting that the allowed_actions_config be its own resource?

Yes, I think having a separate resource could solve this problem. Currently the resource_github_actions_repository_permissions maps to two API resources/endpoints: /repos/{owner}/{repo}/actions/permissions/ and /repos/{owner}/{repo}/actions/permissions/selected-actions.

Having the endpoints under one resource has following problems:

  1. The call to selected-actions fails. These options are readonly. As a fix of this problem I tried to remove the validation and not call the endpoint if no value is set in code. But this lead to problem 2.
  2. Terraform will load/refresh the resource again to update the state accordingly. On the API there are now values set which leads to an inconsistency between the TF Code and the actual state. The TF code defines "null/nothing" for allowed_actions_config but when the resource is loaded/refreshed it will have readonly values.

Splitting those two API endpoints into two individual resources allows managing the /permissions part in TF but ignoring /permissions/selected-actions.

kfcampbell commented 9 months ago

Thanks a bunch for the detailed explanation! That's great.

Breaking changes are a little obnoxious in the Terraform world since they require state migration functions, extra testing, and Hashicorp recommends we don't release more than one per year. We have #2091 ready that I'd like to get officially released in the near future...do you have any interest in creating a PR to fix this behavior as you've described?

Danielku15 commented 9 months ago

I'm not a big expert in Go and the Terraform provider implementations but could give it a try to prepare a PR which would separate the TF resource into two if thats what we're aiming for.

I am still wondering if there would be maybe an alternative to a breaking change by handling the loading of the resource differently.

I started preparing a change where I allow allowed_actions_config to be unset for allowed_actions="selected" and struggled to get my tests green as the TF state was then not matching the code. But what I just realized lately is this idea:

What if we do not refresh / load the data from permissions/selected-actions if it is not configured in the code. That should keep the TF state and code consistent.

I adjusted the related code on the read operation and I think this way things should be fine. PR here: https://github.com/integrations/terraform-provider-github/pull/2114