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]: snowflake_oauth_integration_for_custom_clients - blocked_roles_list should be optional #3171

Open
1 task
imre-kerr-sb1 opened this issue Nov 5, 2024 · 3 comments
Assignees
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@imre-kerr-sb1
Copy link

Terraform CLI Version

1.9.8

Terraform Provider Version

0.97.0

Terraform Configuration

resource "snowflake_oauth_integration_for_custom_clients" "example" {
  name               = "example"
  enabled            = true
  oauth_client_type  = "CONFIDENTIAL"
  oauth_redirect_uri = "https://example.com/complete/snowflake"
  blocked_roles_list = ["ACCOUNTADMIN", "SECURITYADMIN"] # Fails if ORGADMIN is present in the account
  # blocked_roles_list = ["ACCOUNTADMIN", "ORGADMIN", "SECURITYADMIN"] # Fails if ORGADMIN is *not* present in the account
}

Category

category:resource

Object type(s)

resource:oauth_integration

Expected Behavior

blocked_roles_list should be optional, matching the syntax of CREATE SECURITY_INTEGRATION (https://docs.snowflake.com/en/sql-reference/sql/create-security-integration-oauth-snowflake)

Actual Behavior

blocked_roles_list is mandatory. This is annoying, since I have to check if I'm operating on an orgadmin account or not to give the correct list. Bit of a niche case, but could be avoided entirely by just making the field optional.

Error message when missing orgadmin in an orgadmin account:

╷
│ Error: 003629 (42501): Roles [ACCOUNTADMIN, ORGADMIN, SECURITYADMIN] are blocked since parameter OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST is enabled.
│ 
│   with snowflake_oauth_integration_for_custom_clients.example,
│   on main.tf line 1, in resource "snowflake_oauth_integration_for_custom_clients" "example":
│    1: resource "snowflake_oauth_integration_for_custom_clients" "example" {
│ 
╵

Error message when including orgadmin in a non-orgadmin account:

╷
│ Error: 001008 (22023): SQL compilation error:
│ invalid value [ORGADMIN] for parameter 'BLOCKED_ROLES_LIST'
│ 
│   with snowflake_oauth_integration_for_custom_clients.example,
│   on main.tf line 1, in resource "snowflake_oauth_integration_for_custom_clients" "example":
│    1: resource "snowflake_oauth_integration_for_custom_clients" "example" {
│ 
╵

Steps to Reproduce

Apply the configuration above to two accounts, one orgadmin, one not. It will fail on at least one.

How much impact is this issue causing?

Low

Logs

No response

Additional Information

No response

Would you like to implement a fix?

  • Yeah, I'll take it 😎
@imre-kerr-sb1 imre-kerr-sb1 added the bug Used to mark issues with provider's incorrect behavior label Nov 5, 2024
@imre-kerr-sb1
Copy link
Author

Current workaround:

data "snowflake_roles" "orgadmin" {
  like = "ORGADMIN"
}
locals {
  maybe_orgadmin_role = length(data.snowflake_roles.orgadmin.roles) > 0 ? "ORGADMIN" : null
}

resource "snowflake_oauth_integration_for_custom_clients" "example" {
  name               = "example"
  enabled            = true
  oauth_client_type  = "CONFIDENTIAL"
  oauth_redirect_uri = "https://example.com/complete/snowflake"

  blocked_roles_list = compact(["ACCOUNTADMIN", "SECURITYADMIN", local.maybe_orgadmin_role])
}

@sfc-gh-jmichalak
Copy link
Collaborator

Hi @imre-kerr-sb1 👋

It's true that this field should be optional to match Snowflake. We marked this field as required during the rework to handle default roles properly (to handle permadiff in Terraform). However, a similar external_oauth_blocked_roles_list field in snowflake_external_oauth_integration is handled differently and is optional. We'll take a look and see if the behavior is similar in both of these resources. If it is, we can make this field optional with a custom diff suppression function.

@sfc-gh-jmichalak sfc-gh-jmichalak self-assigned this Nov 5, 2024
sfc-gh-jmichalak added a commit that referenced this issue Dec 12, 2024
…#3267)

<!-- Feel free to delete comments as you fill this in -->
- make the `blocked_roles_field` optional in OAuth security integrations
- when tried to use `NormalizeAndCompareIdentifiersInSet`, the diff was
suppressed in step 2 of the new tests, resulting in the role not being
removed - this will be fixed next
<!-- summary of changes -->

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] acceptance tests
<!-- add more below if you think they are relevant -->
* [ ] …

## References
<!-- issues documentation links, etc  -->
#3171

## TODO
- use `NormalizeAndCompareIdentifiersInSet` in this field; also in
external oauth resource
@sfc-gh-jmichalak
Copy link
Collaborator

Hi @imre-kerr-sb1 👋

We've just released v0.100.0 (release, migration guide) in which we made blocked_roles_list optional. Presence of the privileged roles can be controlled by OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST. Please upgrade to this version and let us know if you have any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
Development

No branches or pull requests

2 participants