CheckPointSW / terraform-provider-checkpoint

Terraform provider for Check Point
https://www.terraform.io/docs/providers/checkpoint/
Mozilla Public License 2.0
28 stars 40 forks source link

Provider accepts UIDs but provider reads in only names #132

Closed paulzerkel closed 9 months ago

paulzerkel commented 2 years ago

Many resources in the provider can accept either a UID or a name. For example from the checkpoint_management_access_rule resource:

service - (Optional) Collection of Network objects identified by the name or UID.

It seems that the provider will only read the name of the field, though. This results in two problems:

  1. The inability to ever have a "clean" plan with no changes
  2. Unnecessary changes are made when apply runs

A simple reproduction of this would be to apply a config like the following:

locals {
  layer       = "test Network"
  service     = "681a2333-7883-4400-84d7-4f6198d536ea"
  source      = "b9d05db6-ee4a-4fe4-a402-af794b3bfa2e"
  destination = "517e2027-ed22-4015-98c7-efd54585f23f"
}

resource "checkpoint_management_access_section" "app_section" {
  name     = "state-test"
  layer    = local.layer
  position = { top = "top" }
}

resource "checkpoint_management_access_rule" "rule" {
  layer    = local.layer
  position = { top = checkpoint_management_access_section.app_section.name }
  action   = "Accept"
  action_settings = {
    enable_identity_captive_portal = false
  }
  name     = "state-test-access"
  track = {
    accounting              = false
    alert                   = "none"
    enable_firewall_session = false
    per_connection          = true
    per_session             = false
    type                    = "Log"
  }
  service     = [local.service]
  source      = [local.source]
  destination = [local.destination]
}

A subsequent plan with no changes to the config outputs the following:

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the last "terraform apply":

  # checkpoint_management_access_rule.rule has changed
  ~ resource "checkpoint_management_access_rule" "rule" {
      + content            = []
      + custom_fields      = {}
      ~ destination        = [
          - "517e2027-ed22-4015-98c7-efd54585f23f",
          + "test-dest",
        ]
        id                 = "5d433779-5e29-4cd3-b238-4cd48ec8c039"
      + install_on         = []
        name               = "state-test-access"
      ~ service            = [
          - "681a2333-7883-4400-84d7-4f6198d536ea",
          + "tcp-2222",
        ]
      ~ source             = [
          + "test-source",
          - "b9d05db6-ee4a-4fe4-a402-af794b3bfa2e",
        ]
      + time               = []
        # (14 unchanged attributes hidden)
    }

Unless you have made equivalent changes to your configuration, or ignored the relevant attributes using ignore_changes, the following plan may include actions to undo or respond
to these changes.

──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # checkpoint_management_access_rule.rule will be updated in-place
  ~ resource "checkpoint_management_access_rule" "rule" {
      ~ destination        = [
          + "517e2027-ed22-4015-98c7-efd54585f23f",
          - "test-dest",
        ]
        id                 = "5d433779-5e29-4cd3-b238-4cd48ec8c039"
        name               = "state-test-access"
      ~ service            = [
          + "681a2333-7883-4400-84d7-4f6198d536ea",
          - "tcp-2222",
        ]
      ~ source             = [
          - "test-source",
          + "b9d05db6-ee4a-4fe4-a402-af794b3bfa2e",
        ]
        # (17 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Note the churn from the UID to the name, and then back from the name to the UID. Planning with refresh=false, of course, makes it so that the names are not read in, but that isn't a great solution in the event other real changes have happened.

If apply is run on the config, again with no changes, then the CMA reports a session with a lock and two changes, which should not be needed.

This was tested with version 1.8.0 and 2.2.0 of the provider.

Would it be feasible to change the provider so that if a UID is used in the config it is what is subsequently used in the read operations versus the name? The challenge here is that there are cases where the UID must be used such as more than one service with a given name exists in a CMA.

chkp-royl commented 1 year ago

Hi @paulzerkel , Thanks for sharing this issue. Documentation is indeed confusing and we will make it more clear but Check Point provider assume you force name uniqueness in your environment which is our best practice when working with our APIs (provider is based on Management API), so using object name in configuration is good enough and also more readably. When adding new service via Management API you get an error if a service with same name already exists so ignoring such errors might cause impact on other services like the provider.

Regards, Roy

paulzerkel commented 1 year ago

Hi @chkp-royl,

Is the UID a supported field when specifying sources, destinations, and services in a checkpoint_management_access_rule resource?

pz

chkp-royl commented 1 year ago

In terms of Management API we support both UID/name as object identifier when reference an object but in the provider we support only names which is our standard

paulzerkel commented 1 year ago

@chkp-royl Would you / Check Point be open to a design, proof of concept, or PR in order to handle both UID and names without the churn shown above in the provider? The service situation alone is enough for us to want this feature. We need to work with services that only differ by casing, which means that we must use the UID and therefore can never have a clean plan.

chkp-royl commented 9 months ago

@paulzerkel We added in provider v2.7.0 new field called fields_with_uid_identifier to access rule resource that gives user control in which resource fields the provider will use UID as object identifier (instead of name)

Thanks