PaloAltoNetworks / terraform-provider-panos

Terraform Panos provider
https://www.terraform.io/docs/providers/panos/
Mozilla Public License 2.0
87 stars 69 forks source link

panos_security_policy "audit_comment" field doesn't work #359

Open tommyalatalo opened 1 year ago

tommyalatalo commented 1 year ago

Describe the bug

Setting the audit_comment field in the panos_security_policy resource doesn't work.

Expected behavior

Setting the audit_comment in the panos_security_policy resource would write the audit comment into the security policy.

Current behavior

No audit comment is written to the security policy.

(This results in the operator having to manually write the audit comments into the rule in the UI for instance, since the changes can't be committed without filling in an audit comment)

Steps to reproduce

Applying a security policy like below the audit comment should be written as Allow internet egress from lan, but the provisioned security policy has a blank audit comment.

resource "panos_security_policy" "internet_egress" {
  rule {
    name                  = "allowInternetEgressTest"
    audit_comment         = "Allow internet egress from lan"
    source_zones          = ["lan"]
    source_addresses      = ["any"]
    source_devices        = ["any"]
    source_users          = ["any"]
    destination_devices   = ["any"]
    destination_zones     = ["inet"]
    destination_addresses = ["any"]
    applications          = ["any"]
    services              = ["application-default"]
    categories            = ["any"]
    action                = "allow"
    tags                  = ["allow", "outbound"]
  }

  lifecycle {
    create_before_destroy = true
  }
}

Your Environment

welcome-to-palo-alto-networks[bot] commented 1 year ago

:tada: Thanks for opening your first issue here! Welcome to the community!

Marek-Madi commented 1 year ago

I have opened the same issue and it got closed - https://github.com/PaloAltoNetworks/terraform-provider-panos/issues/339. But I completely agree with you, setting up audit_comment via terraform doesn't have any affect and we have to do it via GUI.

shinmog commented 1 year ago

@altosys

As @Marek-Madi said, the audit_comment param is handled a bit different. There is a whole guide devoted to audit comment logic in the documentation:

https://registry.terraform.io/providers/PaloAltoNetworks/panos/latest/docs/guides/auditcomments

Audit comments should be working, given the rule associated with the audit comment is changing. If the rule is changing and the audit comment is not applied, please given specific steps to repro the issue you're seeing.

tommyalatalo commented 1 year ago

@altosys

As @Marek-Madi said, the audit_comment param is handled a bit different. There is a whole guide devoted to audit comment logic in the documentation:

https://registry.terraform.io/providers/PaloAltoNetworks/panos/latest/docs/guides/auditcomments

Audit comments should be working, given the rule associated with the audit comment is changing. If the rule is changing and the audit comment is not applied, please given specific steps to repro the issue you're seeing.

It's very simple to reproduce, take the resource from my original post and change the name field of the rule to something else. After applying the change the audit comment isn't written to the rule, but when accessing it through the UI the audit comment field is highlighted as a required field, and the changes can't be committed until it's filled out.

shinmog commented 1 year ago

I can reproduce this, so this is a confirmed bug.

The problem is that terraform-plugin-sdk's retrieval of the audit_comment param is always returning an empty string. However, if I remove the DiffSuppressFunc associated with audit_comment then I get the actual value specified in the plan file.

My guess is that there's a bug here in terraform-plugin-sdkv1.17.2, as it's pretty old at this point...? If this is the case, then upgrading to terraform-plugin-sdkv2 would fix it, but that is a massive undertaking that needs some changes in pango as well.

tommyalatalo commented 1 year ago

I can reproduce this, so this is a confirmed bug.

The problem is that terraform-plugin-sdk's retrieval of the audit_comment param is always returning an empty string. However, if I remove the DiffSuppressFunc associated with audit_comment then I get the actual value specified in the plan file.

My guess is that there's a bug here in terraform-plugin-sdkv1.17.2, as it's pretty old at this point...? If this is the case, then upgrading to terraform-plugin-sdkv2 would fix it, but that is a massive undertaking that needs some changes in pango as well.

Is the gist of what you're saying that this isn't going to be fixed?

shinmog commented 1 year ago

It will be fixed. I'm still guessing at the specifics of this bug's situation and thus resolution is all I'm saying.

rwgnr commented 9 months ago

Still seeing this issue a year later - Any updates?

pavelrn commented 8 months ago

@shinmog any updates on this? Our customer is also interested in audit_comment. Thanks!

wouldd commented 7 months ago

So... this has been a problem for us for a while. and I have finally decided to just fix it myself. My choice of how to fix is is perhaps not perfect for everyone but I think it is better than not functioning at all.. Basically I pulled audit_comment out of the object schema alltogether and have the provider just load a value from an envionment variable PANOS_AUDIT_COMMENT. this way we can have our deployment tooling populate this with the package version/PR information etc that links to all the changes we're making. we can use that to trace back to the commits in the relevant repos. which is fine for our use case. Whilst I was at it, I also bumped everything up to sdkv2 and converted the key thing I was interested in (security rules) to those newer interfaces and such. I figured the sdk bump was easier that fully moving to the even newer framework.

I have all this on a fork atm. I would be happy to submit all this as a PR if you are willing to accept my changes? Also happy to discuss variations on the solution if it will help get something into the official version that does what we need.

@shinmog - tagging in case this doesn't already alert you to new message.

Marek-Madi commented 7 months ago

The problem is that this repo is kind of forgotten. Last commit/merge to master from March, which is quite sad.

wouldd commented 7 months ago

The problem is that this repo is kind of forgotten. Last commit/merge to master from March, which is quite sad.

well, for now I can use my fork with a local terraform override. worst case I guess I can investigate what it takes to properly publish an unofficial provider