Add support for network rules

[Relativity74205]

Terraform CLI and Provider Versions

Terraform v1.5.7 Providor v0.85.0

Use Cases or Problem Statement

The old way of assigning IPs directly to a network policy is deprecated by Snowflake. The new way is to define network rules (see e.g. here https://docs.snowflake.com/en/sql-reference/sql/create-network-rule) and to assign them to network policies (see https://docs.snowflake.com/en/sql-reference/sql/alter-network-policy).

Proposal

Add a snowflake_network_rule resource and modify snowflake_network_policy resource.

How much impact is this issue causing?

Medium

Additional Information

No response

[sfc-gh-asawicki]

Hey @Relativity74205. Thanks for reaching out to us.

We have adding the missing GA features on our roadmap: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#our-roadmap. We'll make sure to take care of it then.

[Relativity74205]

@sfc-gh-asawicki Thanks for the info. I would like to create a PR for this feature request, if you accept contributions at this stage of your roadmap. I have already made some contributions in the past.

[sfc-gh-asawicki]

@Relativity74205, we are always open to contributions. Some topics need to be addressed first:

• We do not have up-to-date contribution guidelines, PR requirements, etc. We want to update them and plan to do it in the next couple of weeks.
• We have recently moved to the SDK implementation. We have lots of conventions there that are not documented. Furthermore, we have written a generator to help us with the implementation. It is not that intuitive, though. This means we would prefer adding the SDK implementation ourselves - it will be much better from the timewise perspective. This is the first part needed to introduce the new resource.
• The second part is to add the resource itself, together with the acceptance tests. We design the new resources before implementation to know which direction we are heading. We have yet to have such a design, but it may not be that big of an issue.

From our side, the best course of action would be:

1. We will implement the SDK part in the upcoming weeks.
2. You can implement the resource part using the prepared SDK. We will go ahead and summarize the guidelines beforehand.
3. We can have the design discussions on the PR review level and adjust the resource later.

How does it sound?

[Relativity74205]

Hi @sfc-gh-asawicki, sure, that sounds good. Do you have any issues I can track so I will get notified automatically, when the SDK is finished?

[sfc-gh-asawicki]

There is no issue on GH for this yet. I will create one, and I will let you know its number.

[sfc-gh-jcieslak]

Hey @Relativity74205, I created a feature request for SDK part: https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2514, so you'll be able to track the progress.

[sfc-gh-asawicki]

Hey @Relativity74205. We have merged the SDK part of the network rules.

[wietze]

Sorry to chime in, I am on 0.87.0 but do not see a snowflake_network_rule resource type, which I was expecting the name to be. Am I missing something?

[sfc-gh-asawicki]

Hey @wietze. Please check the comment: https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2482#issuecomment-1946057478. Only the SDK part was implemented, not the resource itself.

[Relativity74205]

@sfc-gh-asawicki Thanks for the info and the implementation of the SDK. I have started to implement the network rules.

However, it seems to me, that the network policies SDK needs to be updated first. Network rules alone do not offer any direct benefit, only when they can be attached to network policies. In particular, the new attributes ALLOWED_NETWORK_RULE_LIST and BLOCKED_NETWORK_RULE_LIST are missing. Currently, only the old attributes ALLOWED_IP_LIST and BLOCKED_IP_LIST are present in the SDK. See https://docs.snowflake.com/en/sql-reference/sql/create-network-policy for details.

[sfc-gh-jcieslak]

@Relativity74205 We created a new GH issue, so you'll be able to track the progress - https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2593

[Relativity74205]

@sfc-gh-jcieslak Thanks a lot!

[Relativity74205]

@sfc-gh-jcieslak I noticed, that the headline of #2593 is wrong. Not the network rules SDK but the network policy SDK has to be updated.

And do you have perhaps an ETA, when the SDK will be updated?

[sfc-gh-asawicki]

Hey @Relativity74205. We should have it as part of the next week's release.

[sfc-gh-asawicki]

Hey @Relativity74205. The issue #2593 was completed in #2647. Let us know if you need anything more from us :)

[Relativity74205]

@sfc-gh-asawicki Thanks for the update. On the first glance, everything looks good, I will be working now on the network rules. :)

[Relativity74205]

@sfc-gh-asawicki I started to implement it and have encountered some issues/questions. A WIP PR can be found here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2746. Please note, that there are many todos there, among other things tests, docs, etc. are missing. However, before I finish the PR, I wanted to clarify the following:

My questions/issues:

• network policies: Is is not possible to completely unset ip lists or network rules. An Unset method is missing for this fields and when passing an empty list to the fields as alternative, a validator network_policies_validations_gen.go steps in. Additionally, when deactiving the validator, Snowflake returns an error, because of ALTER NETWORK POLICY "FOO_POLICY" SET. Please note, that this problem already exists in the main branch(!)
• network rules: Also here the unset command is not implemented in the SDK. And passing an empty list to value_list has the same problem as for the network policies.
• network rules are a more complicated object, because allowed values of some attributes depend on other attributes. e.g. rule_type = IVP4 is only allowed rule_mode = ingress. And the correct format of the value_list entries depend on the rule_type. Therefore, it is not possible to sufficiently check the values with the schema definition with the result, that while many terraform plans will be ok, a lot of crashes during tf apply will happen. Therefore I have started to implement a CustomizeDiff function, where I want to add all the cross attribute validators. Before I finish this (and also add some validators for the value_list entries like host_port, etc.), I wanted to make sure, that this option is fine.

[sfc-gh-asawicki]

Hey @Relativity74205. The SDK is created based on the Snowflake docs, so:

network policies: Is is not possible to completely unset ip lists or network rules. An Unset method is missing for this fields and when passing an empty list to the fields as alternative, a validator network_policies_validations_gen.go steps in. Additionally, when deactiving the validator, Snowflake returns an error, because of ALTER NETWORK POLICY "FOO_POLICY" SET. Please note, that this problem already exists in the main branch(!)

There is no UNSET listed in https://docs.snowflake.com/en/sql-reference/sql/alter-network-policy#syntax. There is, however, a REMOVE/ADD pair that can be utilized to "unset". Does the UNSET work by hand in a worksheet for any of those lists? If so, this may mean that the docs are incomplete.

network rules: Also here the unset command is not implemented in the SDK. And passing an empty list to value_list has the same problem as for the network policies.

It is implemented in the SDK (https://docs.snowflake.com/en/sql-reference/sql/alter-network-rule#syntax): https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/1f165bf72e29d1a9135d2e5e8bf3f46a0ffdddb6/pkg/sdk/network_rule_gen.go#L40. (you have to pass a boolean there not an empty list)

network rules are a more complicated object, because allowed values of some attributes depend on other attributes. e.g. rule_type = IVP4 is only allowed rule_mode = ingress. And the correct format of the value_list entries depend on the rule_type. Therefore, it is not possible to sufficiently check the values with the schema definition with the result, that while many terraform plans will be ok, a lot of crashes during tf apply will happen. Therefore, I have started to implement a CustomizeDiff function, where I want to add all the cross attribute validators. Before I finish this (and also add some validators for the value_list entries like host_port, etc.), I wanted to make sure, that this option is fine.

As I understand, it would be a duplication of Snowflake logic that would be coupled closely with any changes happening on the Snowflake side. In such cases we currently have an approach to leave the validation to Snowflake, so it the first version I would prefer not to have this additional logic. Additionally, before V1, we will probably be splitting multiple resources (probably this one included) into multiple ones when such different setups are possible. We have no decision yet, that's why let's do this without a split initially.

[Relativity74205]

@sfc-gh-asawicki Thanks for the fast reply.

It is implemented in the SDK (https://docs.snowflake.com/en/sql-reference/sql/alter-network-rule#syntax): terraform-provider-snowflake/pkg/sdk/network_rule_gen.go Line 40 in 1f165bf Unset *NetworkRuleUnset ddl:"list" sql:"UNSET" . (you have to pass a boolean there not an empty list)

Totally missed that, somehow. It works, thanks.

There is no UNSET listed in https://docs.snowflake.com/en/sql-reference/sql/alter-network-policy#syntax. There is, however, a REMOVE/ADD pair that can be utilized to "unset". Does the UNSET work by hand in a worksheet for any of those lists? If so, this may mean that the docs are incomplete.

The UNSET is mentioned in the parameters section, however, only with regards to Comments and Tags. Interestingly, it works on all other parameters when writing the SQL commands manually.

Currently, I am not sure, how to implement the removal of all entries for an ip_list/rule_list. Is it possible to access the old values in the UpdateContext? Then I could create a remove command. Like mentioned, setting ip_list/rule_list to an empty slice does not work, the command created by the SDK is faulty.

As I understand, it would be a duplication of Snowflake logic that would be coupled closely with any changes happening on the Snowflake side. In such cases we currently have an approach to leave the validation to Snowflake, so it the first version I would prefer not to have this additional logic. Additionally, before V1, we will probably be splitting multiple resources (probably this one included) into multiple ones when such different setups are possible. We have no decision yet, that's why let's do this without a split initially.

Yes, that would be a duplication of Snowflake logic and my first idea was also not to duplicate the logic. However, from a user perspective of the snowflake provider, it is a big downside if a wrong configuration of the ressource is not noticed during the plan phase only during the apply phase.

[sfc-gh-asawicki]

The UNSET is mentioned in the parameters section, however, only with regards to Comments and Tags. Interestingly, it works on all other parameters when writing the SQL commands manually.

This is what I meant by potentially incomplete documentation. I will reach out to the team responsible for this command internally and confirm if it works by accident or is it just the incomplete documentation.

Currently, I am not sure, how to implement the removal of all entries for an ip_list/rule_list. Is it possible to access the old values in the UpdateContext? Then I could create a remove command.

Given the current implementation of the SDK, you have multiple options:

1. (easy but far from ideal) forceNew with any change on the list
2. (a bit harder and less destructive) do a diff between the two lists - previous and current - and invoke adds for new ones, and removes for the old ones. We already have a similar logic e.g. in task resource (https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/eba3029bd2b6def28901c47df7e142a417d6ba3a/pkg/resources/task.go#L465) but in other resources too

Like mentioned, setting ip_list/rule_list to an empty slice does not work, the command created by the SDK is faulty.

This set is not meant to be used with an empty list (this is true for other objects in the SDK too).

Yes, that would be a duplication of Snowflake logic and my first idea was also not to duplicate the logic. However, from a user perspective of the snowflake provider, it is a big downside if a wrong configuration of the ressource is not noticed during the plan phase only during the apply phase.

Yes, this is a downside. But other option, when all the changes, not only the validations, but other parts of Snowflake logic would be migrated to the provider, and would have to be synchronized in every stable version of the provider almost instantly, and would require constant migrations from the users are not appealing. We think it would be unmanageable, both for us and for our users, so for V1 we won't do such validations (with exceptions ofc).

[Relativity74205]

d.GetChange() should work, that I was looking for, thanks. I will then implement the update on NetworkPolicies in this way.

Yes, this is a downside. But other option, when all the changes, not only the validations, but other parts of Snowflake logic would be migrated to the provider, and would have to be synchronized in every stable version of the provider almost instantly, and would require constant migrations from the users are not appealing. We think it would be unmanageable, both for us and for our users, so for V1 we won't do such validations (with exceptions ofc).

Good points. I will then remove the CustomizeDiff part from the ressource struct.

This is what I meant by potentially incomplete documentation. I will reach out to the team responsible for this command internally and confirm if it works by accident or is it just the incomplete documentation.

Thanks. I would be also interested in the answer ;)

[Relativity74205]

@sfc-gh-asawicki Sorry to bother you again. Implementing addition and removal of network rules to the network policy worked, also the removal of all network rules. However, removing all ips from on the ip list attributes does not work in this way. According to the docs: "To unset this parameter, specify an empty list.", which does not work with the SDK. Hence, the only way left with the current sdk is to set forceNew on both ip list attributes. Or am I missing something?

[sfc-gh-asawicki]

Hey @Relativity74205, I am off today, but I handed this topic to @sfc-gh-jcieslak and @sfc-gh-jmichalak; they will get in touch with you (I think the to unset this parameter, specify an empty list. will have to be added to the SDK).

[sfc-gh-jmichalak]

Hi @Relativity74205 I confirmed with network policy team that UNSET for other fields is just undocumented. I'm taking care of it in SDK, will keep you updated on this change.

[Relativity74205]

@sfc-gh-jmichalak Thanks for the update. I will then wait until the SDK is updated before I finish my work.

[sfc-gh-jmichalak]

@Relativity74205 I merged https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2759. Now, UNSET and empty SET are supported for the remaining network policy fields, so you can remove all IPs and network rules easily. Let us know if you need anything :)

[Relativity74205]

Hi @sfc-gh-jmichalak , thanks that worked.

I am now nearly finished (PR: https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2746). I have still to add the docs and the tests (will do it in the next days). In the meantime, I have a question how about to validate a list of SchemaObjectIdentifiers. I think that this problem is new to the provider. At least, I haven't found an example in the code for another ressource. I have added a length comment in the PR, can you please have a look?