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

fix: fail the provider when env vars are expected but no value is set #1085

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cdsre
Copy link

@cdsre cdsre commented Nov 26, 2024

🔧 Changes

This fix is to address #1074 which is a result of changes in #1053 and #1065 . It appears that the aim of both these PR's was to fail the provider when no value was provided directly in the provider hcl, and no value was found from the ENV vars using the EnvDefaultFunc from the terraform schema.

However this solution fails to address the case where the value configured in the provider block is not static, but instead comes from either a data lookup, or module which may not be evaluated to its value until later in the terraform flow. The current logic checks the value of each attribute at the provider initialization. If the value is configured in the provider block but is a reference to another resource that has not yet been applied it will evaluate to "" and will look the same as when missing env vars evaluate to "".

In order to address this I have taken the approach that since the EnvDefaultFunc function is only invoked when the attribute is not directly configured in the provider block. I.E meaning that we don't have a static or a reference to another resource or data block. Then we look for the value in the environment.

The EnvDefaultFunc takes 2 parameters, the key to look up, and a default value. Instead of passing a nil default which will evaluate as an empty string, instead pass a string value called "MISSING". This allows for us to differentiate between a provider attribute that is empty at initialization because its dynamically provided later, and a provider attribute that was not configured so we have defaulted to the ENV var and its still not set.

This achieves the objective of being able to validate missing ENV vars while allowing us to keep the backwards compatibility of provider attributes that are configured with a value that comes from another resource or data block.

📚 References

#1074
#1053
#1065

🔬 Testing

See #1074 for an example on how to reproduce this bug in the current provider and which is solved by this PR

📝 Checklist

  • [N/A ] All new/changed/fixed functionality is covered by tests (or N/A)
  • [N/A ] I have added documentation for all new/changed functionality (or N/A)

@duedares-rvj
Copy link
Contributor

Hello again @cdsre
Thank you for coming up with the approach and making this contribution.
Before we could merge this, we'd need some last changes.

Could you please run below commands and push the change:
make docs
make lint

Also, We require all commits on the contributing PR to be signed.

@cdsre
Copy link
Author

cdsre commented Nov 26, 2024

Hi @duedares-rvj I have run both make docs and make lint. I solved the linting error adding a comment to the exported constant in config.go

Other than that there was no changes to the docs.

In regards to signing the commits, I am not sure if that comment is just in general or if I have missed something as generally on open source projects I sign my commits anyway. The two commits were done using

git commit -sam "fix: fail the provider when env vars are expected but no value is set"
git commit -sam "chore: fixing linting error on exported constant"

Let me know if you need anything more.

Chris.

@cdsre
Copy link
Author

cdsre commented Nov 26, 2024

I Have just clicked the Update from branch button as main was 2 commits ahead, this has generated a 3rd commit on the PR that isnt directly from me.

@duedares-rvj
Copy link
Contributor

@cdsre Thanks for signing the commits, lint and generating the docs!
Although, it seems the the change is breaking most of the tests.
Once new updates are ready, you can try running make test-acc locally and see if all tests pass. And then push your code.

Thanks!

@cdsre
Copy link
Author

cdsre commented Nov 27, 2024

Thanks @duedares-rvj I will have a look at the tests today.

@cdsre
Copy link
Author

cdsre commented Nov 27, 2024

So the tests are mostly failing because the EnvDefaultFunc will now return a string value of "MISSING" for the attributes when when they are not defined in the provider config and have not been set at the env var level. However because of this when ever you initialize the provider these values will always be set.

This then causes tests like missing client id to fail since the value is no longer nil the provider doesn't consider it to be missing when you only pass client_secret. And tests like conflicting auth0 client and management token without domain since the values for all these attributes in the provider will always have a value or default to "MISSING" so will always coexist breaking the ConflictsWith conditions.

For now I think the best course of action is to close this PR. I will have a think a bit more about the problem and update issue #1074 . The more I look at it the more I wonder if this is an issue to be solved in the SDK rather than the provider. Since the original issue #1023 is for an error that actually arises in the SDK rather then the provider. Its just propagated back to the provider. https://github.com/search?q=repo%3Aauth0%2Fgo-auth0%20%22failed%20to%20send%20the%20request%22&type=code

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.

2 participants