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

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

Closed
1 task done
Danielku15 opened this issue Jan 16, 2024 · 5 comments · Fixed by #2114
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented vNext

Comments

@Danielku15
Copy link
Contributor

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:

errors.New("the allowed_actions_config {} block must be specified if allowed_actions == 'selected'")

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)

  • resource_github_actions_repository_permissions
  • resource_github_actions_organization_permissions

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

  • I agree to follow this project's Code of Conduct
@Danielku15 Danielku15 added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Jan 16, 2024
@Danielku15
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Danielku15 commented Jan 20, 2024

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
  3. Create a new repository (manually for the sake of clarity) and disable GHA:
    image
  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
  5. Now we enable GHA with allowed_actions="selected" and can see that the selected-actions endpoint delivers the config from the organization:
    image
  6. But when I try to set the selected-actions this will fail:
    image

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
Copy link
Member

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?

@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone vNext and removed Status: Triage This is being looked at and prioritized labels Jan 22, 2024
@Danielku15
Copy link
Contributor Author

Danielku15 commented Jan 25, 2024

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: #2114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented vNext
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants