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

Add path to invoke update without determining changed CRN data, add validator for name of secrets #4859

Conversation

jared-hayes-dev
Copy link
Contributor

@jared-hayes-dev jared-hayes-dev commented Oct 11, 2023

Users have been looking for way to invoke update on secret without field change to pull in upstream changes from secrets manager instance so the update path was changed to allow such behavior. Users wanted validation on secret name when using terraform to ensure compliance with Kubernetes resource naming scheme.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Output from acceptance testing:

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretTLS_Basic'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretTLS_Basic -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretTLS_Basic
--- PASS: TestAccIBMContainerIngressSecretTLS_Basic (44.43s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	45.876s

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretTLSDatasourceBasic'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretTLSDatasourceBasic -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretTLSDatasourceBasic
--- PASS: TestAccIBMContainerIngressSecretTLSDatasourceBasic (23.68s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	25.144s

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretOpaque_Basic'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretOpaque_Basic -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretOpaque_Basic
--- PASS: TestAccIBMContainerIngressSecretOpaque_Basic (41.07s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	42.528s

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretOpaqueDatasourceBasic'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretOpaqueDatasourceBasic -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretOpaqueDatasourceBasic
--- PASS: TestAccIBMContainerIngressSecretOpaqueDatasourceBasic (25.61s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	27.047s

@jared-hayes-dev
Copy link
Contributor Author

jared-hayes-dev commented Oct 16, 2023

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretTLS_InvalidName'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretTLS_InvalidName -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretTLS_InvalidName
--- PASS: TestAccIBMContainerIngressSecretTLS_InvalidName (0.55s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	2.079s
make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretOpaque_InvalidName'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretOpaque_InvalidName -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretOpaque_InvalidName
--- PASS: TestAccIBMContainerIngressSecretOpaque_InvalidName (0.55s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	2.261s
make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretOpaque_ForceUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretOpaque_ForceUpdate -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretOpaque_ForceUpdate
--- PASS: TestAccIBMContainerIngressSecretOpaque_ForceUpdate (43.36s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	44.963s
make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretTLS_BasicForceUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretTLS_BasicForceUpdate -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretTLS_BasicForceUpdate
--- PASS: TestAccIBMContainerIngressSecretTLS_BasicForceUpdate (40.20s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	41.789s

@@ -73,6 +76,17 @@ func ResourceIBMContainerIngressSecretOpaque() *schema.Resource {
Computed: true,
Description: "Status of the secret",
},
"update_secret": {
Type: schema.TypeBool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will have issue if you want to update_secret multiple times .
If user add this field for first time and mark update_secret = true it goes a head and update secret but next time when he wants to update again it will not show any diff to apply since update_secret = true already exists in state file.

Generally for this kind of scenario we define "update_secret" as int when user want to change it he will just increment the value

similar to https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/resources/container_vpc_cluster#retry_patch_version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@hkantare
Copy link
Collaborator

Update documentation files also respectively

@@ -34,6 +34,7 @@ Review the argument references that you can specify for your resource.
- `secret_name` - (Required, String) The name of the kubernetes secret.
- `secret_namespace` - (Required, String) The namespace of the kubernetes secret.
- `persistence` - (Bool) Persist the secret data in your cluster. If the secret is later deleted from the command line or OpenShift web console, the secret is automatically re-created in your cluster.
- `update_secret` - (Optional, Integer) This argument is used to force update from upstream secrets manager instance that stores secret. Increment the value to force an update to your Ingress secret for changes made to the upstream secrets manager secret.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mind adding a dash before the fields below this? I think I forgot to in the initial documentation so it's running into the next line

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can fix the "fields" argument indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed to be consistent with other definitions that have nested structs

"update_secret": {
Type: schema.TypeInt,
Optional: true,
Default: false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default can't be false for int type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed - field now has exact same structure as others that fulfill same purpose

Comment on lines +79 to +83
"update_secret": {
Type: schema.TypeInt,
Optional: true,
Description: "Updates secret from secrets manager if value is changed (increment each usage)",
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_secret is never set in the code with d.Set. if I get it right, terraform will detect this field as changed on every apply, if the field is not empty.
this can be verified with a test, where you apply the same testAccCheckIBMContainerIngressSecretOpaqueForceUpdate configuration twice.

you can to use d.Set and update the state after the ingressAPI.UpdateIngressSecret .

( or if this behaviour works for you, the field can be modified to a boolean. )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to replicate the behavior you mentioned either by running the test in the code with the same config again or by manually using my own terraform plan/resources. I see the update_secret field reflected in the state file and terraform doesn't execute an update on a repeat apply of the same config should the variable remain unchanged. I also took a look at the above mentioned field retry_patch_version whose pattern update_secret was modeled after and I didn't see any instances of that implementation using the d.set either. Although even in the case it did update every time that would be acceptable behavior. Please let me know if there are any other concerns.

I also went ahead and updated the tests to validate this behavior:

make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretOpaque_ForceUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretOpaque_ForceUpdate -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretOpaque_ForceUpdate
--- PASS: TestAccIBMContainerIngressSecretOpaque_ForceUpdate (84.83s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	86.414s
make testacc TEST=./ibm/service/kubernetes TESTARGS='-run=TestAccIBMContainerIngressSecretTLS_BasicForceUpdate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./ibm/service/kubernetes -v -run=TestAccIBMContainerIngressSecretTLS_BasicForceUpdate -timeout 700m
=== RUN   TestAccIBMContainerIngressSecretTLS_BasicForceUpdate
--- PASS: TestAccIBMContainerIngressSecretTLS_BasicForceUpdate (63.88s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/kubernetes	65.472s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this one.
thank you Jared for looking into it!

@hkantare hkantare merged commit 0e15cb1 into IBM-Cloud:master Oct 25, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants