pulumi / pulumi-github

A Pulumi package to facilitate interacting with GitHub
Apache License 2.0
59 stars 10 forks source link

ActionsOrganizationSecret: Updates delete but do not create the secret. #250

Open MitchellGerdisch opened 2 years ago

MitchellGerdisch commented 2 years ago

What happened?

An update to a github.ActionsOrganizationSecret resource results in the resource being replaced (expected) but the secret is deleted and then not recreated and no error message is given. Pulumi indicates that everything worked as expected but going to github to see the secret shows it is deleted.

Steps to reproduce

Using the code provided below, do the following:

  1. pulumi up -y
  2. Go to github organization and see the "TESTSECRET" secret is created.
  3. Change the plaintext field to something else.
  4. pulumi up -y (this will require replacing the secret)
  5. See the pulumi up completes as expected with no errors.
  6. Go to Github and see that the secret is no longer there.
  7. WORKAROUND: Use deleteBeforeReplace resource option.

TEST CODE:

import * as github from "@pulumi/github";

const secret = new github.ActionsOrganizationSecret("secret", {
    secretName: "testsecret",
    visibility: "private",
    plaintextValue: "mysecret",
}, 
// { deleteBeforeReplace: true}
)

Expected Behavior

Without the deleteBeforeReplace setting, I would expect some error to be presented instead of silently failing with indications of success.

Actual Behavior

The pulumi up that replaces the secret completes with no error message but the secret is deleted and not recreated.

Output of pulumi about

CLI          
Version      3.40.2
Go Version   go1.19.1
Go Compiler  gc

Plugins
NAME    VERSION
github  4.17.0
nodejs  unknown

Host     
OS       darwin
Version  12.5.1
Arch     x86_64

This project is written in nodejs: executable='/Users/mitch/.nvm/versions/node/v18.0.0/bin/node' version='v18.0.0'

Current Stack: dev

TYPE                                                              URN
pulumi:pulumi:Stack                                               urn:pulumi:dev::github-org-testing::pulumi:pulumi:Stack::github-org-testing-dev
pulumi:providers:github                                           urn:pulumi:dev::github-org-testing::pulumi:providers:github::default_4_17_0
github:index/actionsOrganizationSecret:ActionsOrganizationSecret  urn:pulumi:dev::github-org-testing::github:index/actionsOrganizationSecret:ActionsOrganizationSecret::secret

Found no pending operations associated with dev

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/xxxxxx
User           xxxxxxx
Organizations  xxxxx

Dependencies:
NAME            VERSION
@pulumi/github  4.17.0
@pulumi/pulumi  3.40.2
@types/node     14.18.31

Pulumi locates its logs in /var/folders/qp/6k0zsrj13rz5ll53hsmlksvw0000gq/T/ by default

Additional context

Workaround is to use deleteBeforeReplace: true

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

MitchellGerdisch commented 2 years ago

Forgot to mention that using the TF provider directly works as expected. So it appears to be specific to the bridged provider.

Here's TF code that can be used to test:

terraform {
  required_providers {
    github = {
      source  = "integrations/github"
      version = "5.2.0"
    }
  }
}

# Configure the GitHub Provider
provider "github" {
  token = "XXXXXXXXXX"
  owner = "YYYYYYYYYY"

}

resource "github_actions_organization_secret" "tf_example_secret" {
  secret_name     = "tf_example_secret_name"
  visibility      = "private"
  plaintext_value = "mybadsecret"
}
cbruun commented 1 year ago

The same behavior occurs with ActionsSecret.

MagnusHJensen commented 1 month ago

This is definitely still a problem, is there any ETA on fixing this? For our case, it keeps recreating secrets that we gave a value through the Github UI, which is definitely not ideal and makes it unusable for managing secrets.

@lblackstone @mnlumi ?

VenelinMartinov commented 1 month ago

Thanks for flagging this up and sorry you've hit this. This looks like an instance of https://github.com/pulumi/pulumi/issues/918 and https://github.com/pulumi/pulumi/issues/15982

We need to add DeleteBeforeReplace by default to the affected resources, similar to https://github.com/pulumi/pulumi-gcp/pull/2498

The workaround for anyone affected is to specify DeleteBeforeReplace for the resources in your program.

MagnusHJensen commented 1 month ago

Thanks for flagging this up and sorry you've hit this. This looks like an instance of pulumi/pulumi#918 and pulumi/pulumi#15982

We need to add DeleteBeforeReplace by default to the affected resources, similar to pulumi/pulumi-gcp#2498

The workaround for anyone affected is to specify DeleteBeforeReplace for the resources in your program.

That did not seem to work in our case, it still flagged it as being deleted. Should it respect the ignoreChanges attribute? If tell it to ignore changes to plaintextValue or encryptedValue and it changes, then it should just skip it.

VenelinMartinov commented 1 month ago

@MagnusHJensen for your case I believe you need to refresh the resource - ignore changes tells the engine to use the old value for the property: https://www.pulumi.com/docs/iac/concepts/options/ignorechanges/#:~:text=The%20ignoreChanges%20resource%20option%20specifies,update%20or%20replace%20is%20needed.

In your case the old value is not the one you want, hence it tries to replace the resource.

MagnusHJensen commented 1 month ago

@MagnusHJensen for your case I believe you need to refresh the resource - ignore changes tells the engine to use the old value for the property: https://www.pulumi.com/docs/iac/concepts/options/ignorechanges/#:~:text=The%20ignoreChanges%20resource%20option%20specifies,update%20or%20replace%20is%20needed.

In your case the old value is not the one you want, hence it tries to replace the resource.

I can see the ignoreChanges might not work as expected as to my original thought.

However I do know refreshing did not work, as our CI refreshes all resources before running up and in the refresh command it gets refreshed to a deleted state, which also is wrong.

VenelinMartinov commented 1 month ago

@MagnusHJensen, it sounds like your problem is not the same as in the original Github issue - could you please open a new issue with step by step instructions on how to reproduce your problem?