Skip to content

feat(terraform): Add support for audit comments to policies #711

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.

Sign up for GitHub

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

Merged
kklimonda-cl merged 1 commit into from
Apr 23, 2026
Merged

feat(terraform): Add support for audit comments to policies #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions assets/terraform/internal/manager/entry_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"

sdkerrors "github.com/PaloAltoNetworks/pango/errors"
"github.com/PaloAltoNetworks/pango/util"
"github.com/PaloAltoNetworks/pango/version"
"github.com/PaloAltoNetworks/pango/xmlapi"

Expand Down Expand Up @@ -117,6 +118,11 @@ func (o *MockEntryClient[E]) MultiConfig(ctx context.Context, updates *xmlapi.Mu
return nil, nil, nil, nil
}

func (o *MockEntryClient[E]) Communicate(ctx context.Context, cmd util.PangoCommand, strip bool, ans any) ([]byte, *http.Response, error) {
// Mock implementation for Communicate API calls (audit comments, etc.)
return nil, nil, nil
}

func (o *MockEntryClient[E]) list() []E {
var entries []E
slog.Debug("MockEntryClient list()", "o.Current", o.Current, "o.Current.Front()", o.Current.Front())
Expand Down
2 changes: 2 additions & 0 deletions assets/terraform/internal/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/url"

"github.com/PaloAltoNetworks/pango/util"
"github.com/PaloAltoNetworks/pango/version"
"github.com/PaloAltoNetworks/pango/xmlapi"
)
Expand Down Expand Up @@ -55,4 +56,5 @@ type SDKClient interface {
GetTarget() string
ChunkedMultiConfig(context.Context, *xmlapi.MultiConfig, bool, url.Values) ([]xmlapi.ChunkedMultiConfigResponse, error)
MultiConfig(context.Context, *xmlapi.MultiConfig, bool, url.Values) ([]byte, *http.Response, *xmlapi.MultiConfigResponse, error)
Communicate(context.Context, util.PangoCommand, bool, any) ([]byte, *http.Response, error)
}
68 changes: 66 additions & 2 deletions assets/terraform/internal/manager/uuid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (

"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-log/tflog"

"github.com/PaloAltoNetworks/pango/audit"
sdkerrors "github.com/PaloAltoNetworks/pango/errors"
"github.com/PaloAltoNetworks/pango/movement"
"github.com/PaloAltoNetworks/pango/util"
Expand Down Expand Up @@ -258,7 +260,7 @@ func (o *UuidObjectManager[E, L, S]) moveNonExhaustive(ctx context.Context, loca
return nil
}

func (o *UuidObjectManager[E, L, S]) CreateMany(ctx context.Context, location L, parentComponents []string, planEntries []E, exhaustive ExhaustiveType, sdkPosition movement.Position) ([]E, error) {
func (o *UuidObjectManager[E, L, S]) CreateMany(ctx context.Context, location L, parentComponents []string, planEntries []E, exhaustive ExhaustiveType, sdkPosition movement.Position, auditComments map[string]string) ([]E, error) {
var diags diag.Diagnostics

planEntriesByName := o.entriesByName(planEntries, entryUnknown)
Expand Down Expand Up @@ -343,6 +345,37 @@ func (o *UuidObjectManager[E, L, S]) CreateMany(ctx context.Context, location L,
return nil, errors.Join(moveErr, cleanupErr)
}

// Op API calls cannot be batched with Config operations in MultiConfig (API limitation),
// so we send audit comments sequentially after the main configuration succeeds
tflog.Debug(ctx, "Processing audit comments", map[string]interface{}{"count": len(auditComments), "comments": auditComments})
for entryName, comment := range auditComments {
components := append(parentComponents, util.AsEntryXpath(entryName))
path, err := location.XpathWithComponents(o.client.Versioning(), components...)
if err != nil {
tflog.Error(ctx, "Failed to create xpath for audit comment", map[string]interface{}{"entry": entryName, "error": err.Error()})
return nil, fmt.Errorf("failed to create xpath for audit comment: %w", err)
}

opCmd := &xmlapi.Op{
Command: audit.SetComment{
Xpath: util.AsXpath(path),
Comment: comment,
},
Target: o.client.GetTarget(),
}

tflog.Debug(ctx, "Sending audit comment via Communicate", map[string]interface{}{"entry": entryName, "xpath": util.AsXpath(path), "comment": comment})
_, _, err = o.client.Communicate(ctx, opCmd, false, nil)
if err != nil {
tflog.Error(ctx, "Failed to send audit comment", map[string]interface{}{"entry": entryName, "error": err.Error()})
// Continue on audit comment errors - MultiConfig has already committed the rule changes,
// so we cannot roll back. We must save state with the successful rule operations.
continue
}
tflog.Debug(ctx, "Successfully sent audit comment", map[string]interface{}{"entry": entryName})
}
tflog.Debug(ctx, "Completed processing audit comments")

existing, err = o.service.List(ctx, location, "get", "", "")
if err != nil && !sdkerrors.IsObjectNotFound(err) {
return nil, fmt.Errorf("Failed to list remote entries: %w", err)
Expand All @@ -360,7 +393,7 @@ func (o *UuidObjectManager[E, L, S]) CreateMany(ctx context.Context, location L,
return entries, nil
}

func (o *UuidObjectManager[E, L, S]) UpdateMany(ctx context.Context, location L, parentComponents []string, stateEntries []E, planEntries []E, exhaustive ExhaustiveType, position movement.Position) ([]E, error) {
func (o *UuidObjectManager[E, L, S]) UpdateMany(ctx context.Context, location L, parentComponents []string, stateEntries []E, planEntries []E, exhaustive ExhaustiveType, position movement.Position, auditComments map[string]string) ([]E, error) {
stateEntriesByName := o.entriesByName(stateEntries, entryUnknown)
planEntriesByName := o.entriesByName(planEntries, entryUnknown)
if len(planEntriesByName) != len(planEntries) {
Expand Down Expand Up @@ -616,6 +649,37 @@ func (o *UuidObjectManager[E, L, S]) UpdateMany(ctx context.Context, location L,
return nil, errors.Join(moveErr, cleanupErr)
}

// Op API calls cannot be batched with Config operations in MultiConfig (API limitation),
// so we send audit comments sequentially after the main configuration succeeds
tflog.Debug(ctx, "Processing audit comments", map[string]interface{}{"count": len(auditComments), "comments": auditComments})
for entryName, comment := range auditComments {
components := append(parentComponents, util.AsEntryXpath(entryName))
path, err := location.XpathWithComponents(o.client.Versioning(), components...)
if err != nil {
tflog.Error(ctx, "Failed to create xpath for audit comment", map[string]interface{}{"entry": entryName, "error": err.Error()})
return nil, fmt.Errorf("failed to create xpath for audit comment: %w", err)
}

opCmd := &xmlapi.Op{
Command: audit.SetComment{
Xpath: util.AsXpath(path),
Comment: comment,
},
Target: o.client.GetTarget(),
}

tflog.Debug(ctx, "Sending audit comment via Communicate", map[string]interface{}{"entry": entryName, "xpath": util.AsXpath(path), "comment": comment})
_, _, err = o.client.Communicate(ctx, opCmd, false, nil)
if err != nil {
tflog.Error(ctx, "Failed to send audit comment", map[string]interface{}{"entry": entryName, "error": err.Error()})
// Continue on audit comment errors - MultiConfig has already committed the rule changes,
// so we cannot roll back. We must save state with the successful rule operations.
continue
}
tflog.Debug(ctx, "Successfully sent audit comment", map[string]interface{}{"entry": entryName})
}
tflog.Debug(ctx, "Completed processing audit comments")

existing, err = o.service.List(ctx, location, "get", "", "")
if err != nil && !sdkerrors.IsObjectNotFound(err) {
return nil, fmt.Errorf("Failed to list remote entries: %w", err)
Expand Down
40 changes: 20 additions & 20 deletions assets/terraform/internal/manager/uuid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ var _ = Describe("Server", func() {

It("CreateMany() should create new entries on the server, and return them with uuid set", func() {
entries := []*MockUuidObject{{Name: "1", Value: "A"}}
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.Exhaustive, movement.PositionFirst{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.Exhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(1))
Expand All @@ -114,7 +114,7 @@ var _ = Describe("Server", func() {
Context("and entries with the same name are being created in NonExhaustive mode", func() {
It("should not create any entries and return an error", func() {
entries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "4", Value: "D"}}
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).To(MatchError(sdkmanager.ErrConflict))
Expect(processed).To(BeNil())
Expand All @@ -126,7 +126,7 @@ var _ = Describe("Server", func() {
Context("and all entries being created are new to the server", func() {
It("should create those entries in the correct position", func() {
entries := []*MockUuidObject{{Name: "4", Value: "D"}, {Name: "5", Value: "E"}}
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(2))
Expand All @@ -142,7 +142,7 @@ var _ = Describe("Server", func() {
Context("and entries are created in Exhaustive mode", func() {
It("should not return any error and overwrite all entries on the server", func() {
entries := []*MockUuidObject{{Name: "1", Value: "A'"}, {Name: "3", Value: "C"}}
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.Exhaustive, movement.PositionFirst{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.Exhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -174,7 +174,7 @@ var _ = Describe("Server", func() {
stateEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B"}, {Name: "3", Value: "C"}}
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "3", Value: "C"}, {Name: "2", Value: "B"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -192,7 +192,7 @@ var _ = Describe("Server", func() {
stateEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B"}, {Name: "3", Value: "C"}}
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "3", Value: "C"}, {Name: "2", Value: "B"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.Exhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.Exhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -212,7 +212,7 @@ var _ = Describe("Server", func() {

It("should add the entry to the server", func() {
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B"}, {Name: "3", Value: "C"}}
processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionLast{})
processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionLast{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -224,7 +224,7 @@ var _ = Describe("Server", func() {
It("should delete the entry from the server", func() {
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "3", Value: "C"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(2))
Expand All @@ -236,7 +236,7 @@ var _ = Describe("Server", func() {
It("should update the entry on the server", func() {
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B_modified"}, {Name: "3", Value: "C"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -248,7 +248,7 @@ var _ = Describe("Server", func() {
It("should rename the entry on the server", func() {
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "two", Value: "B"}, {Name: "3", Value: "C"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, initial, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -275,7 +275,7 @@ var _ = Describe("Server", func() {
stateEntries := []*MockUuidObject{}
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.Exhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.Exhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(2))
Expand All @@ -291,7 +291,7 @@ var _ = Describe("Server", func() {
It("should return a conflict error", func() {
stateEntries := []*MockUuidObject{{Name: "1", Value: "A"}}
planEntries := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "conflict", Value: "new"}}
_, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
_, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).To(MatchError(sdkmanager.ErrConflict))
})
Expand Down Expand Up @@ -333,7 +333,7 @@ var _ = Describe("Server", func() {

expectedFinalServerObjects := append(expectedFinalState, &MockUuidObject{Name: "99", Value: "ZZ"})

processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.UpdateMany(ctx, location, []string{}, stateEntries, planEntries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(4))
Expand Down Expand Up @@ -369,7 +369,7 @@ var _ = Describe("Server", func() {
state := []*MockUuidObject{{Name: "1", Value: ""}, {Name: "2", Value: ""}, {Name: "3", Value: "C"}}
plan := []*MockUuidObject{{Name: "1", Value: "A"}, {Name: "2", Value: "B"}, {Name: "3", Value: "C"}}

processed, err := manager.UpdateMany(ctx, location, []string{}, state, plan, sdkmanager.NonExhaustive, movement.PositionLast{})
processed, err := manager.UpdateMany(ctx, location, []string{}, state, plan, sdkmanager.NonExhaustive, movement.PositionLast{}, map[string]string{})
Expect(err).ToNot(HaveOccurred())
Expect(processed).To(MatchEntries(plan))
Expect(client.list()).To(MatchEntries(append([]*MockUuidObject{{Name: "4", Value: ""}}, plan...)))
Expand Down Expand Up @@ -437,7 +437,7 @@ var _ = Describe("Server", func() {
Expect(moveRequired).To(BeFalse())
Expect(processed).To(HaveLen(2))

processed, err = manager.UpdateMany(ctx, location, []string{}, processed, entries, sdkmanager.NonExhaustive, movement.PositionLast{})
processed, err = manager.UpdateMany(ctx, location, []string{}, processed, entries, sdkmanager.NonExhaustive, movement.PositionLast{}, map[string]string{})
Expect(client.list()).To(HaveLen(3))
Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -452,7 +452,7 @@ var _ = Describe("Server", func() {
It("should create new entries on the top of the list", func() {
entries := []*MockUuidObject{{Name: "4", Value: "D"}, {Name: "5", Value: "E"}, {Name: "6", Value: "F"}}

processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})
Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))

Expand All @@ -470,7 +470,7 @@ var _ = Describe("Server", func() {
It("should create new entries on the bottom of the list", func() {
entries := []*MockUuidObject{{Name: "4", Value: "D"}, {Name: "5", Value: "E"}, {Name: "6", Value: "F"}}

processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionLast{})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionLast{}, map[string]string{})
Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))

Expand All @@ -488,7 +488,7 @@ var _ = Describe("Server", func() {
It("should create new entries directly after first existing element", func() {
entries := []*MockUuidObject{{Name: "4", Value: "D"}, {Name: "5", Value: "E"}, {Name: "6", Value: "F"}}

processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionAfter{Directly: true, Pivot: initial[0].Name})
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionAfter{Directly: true, Pivot: initial[0].Name}, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -513,7 +513,7 @@ var _ = Describe("Server", func() {

pivot := initial[2].Name // "3"
position := movement.PositionBefore{Directly: true, Pivot: pivot}
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, position)
processed, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, position, map[string]string{})

Expect(err).ToNot(HaveOccurred())
Expect(processed).To(HaveLen(3))
Expand All @@ -532,7 +532,7 @@ var _ = Describe("Server", func() {
Context("and there is a duplicate entry within a list", func() {
It("should properly raise an error", func() {
entries := []*MockUuidObject{{Name: "4", Value: "D"}, {Name: "4", Value: "D"}}
_, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{})
_, err := manager.CreateMany(ctx, location, []string{}, entries, sdkmanager.NonExhaustive, movement.PositionFirst{}, map[string]string{})

Expect(err).To(MatchError(sdkmanager.ErrPlanConflict))
})
Expand Down
5 changes: 5 additions & 0 deletions assets/terraform/internal/manager/uuid_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ func (o *MockUuidClient[E]) MultiConfig(ctx context.Context, updates *xmlapi.Mul
return nil, nil, nil, nil
}

func (o *MockUuidClient[E]) Communicate(ctx context.Context, cmd util.PangoCommand, strip bool, ans any) ([]byte, *http.Response, error) {
// Mock implementation for Communicate API calls (audit comments, etc.)
return nil, nil, nil
}

func (o *MockUuidClient[E]) list() []E {
var entries []E
for e := o.Current.Front(); e != nil; e = e.Next() {
Expand Down
Loading
Loading