Remove vendored terraform RPC diagnostics code

[craigfurman]

Projects that import both regula and terraform would see a runtime panic
when both this vendored and the original terraform's init functions try
to register duplicate types with gob.

[craigfurman]

Running make terraform_gen also produces this diff, which stops regula from compiling:

diff --git a/pkg/terraform/configs/configload/loader_snapshot.go b/pkg/terraform/configs/configload/loader_snapshot.go
index 6ca8b4f..243939f 100644
--- a/pkg/terraform/configs/configload/loader_snapshot.go
+++ b/pkg/terraform/configs/configload/loader_snapshot.go
@@ -325,10 +325,6 @@ func (fs snapshotFS) Chmod(name string, mode os.FileMode) error {
        return fmt.Errorf("cannot set file mode inside configuration snapshot")
 }

-func (fs snapshotFS) Chown(string, int, int) error {
-       return fmt.Errorf("cannot set file owner inside configuration snapshot")
-}
-
 func (fs snapshotFS) Chtimes(name string, atime, mtime time.Time) error {
        return fmt.Errorf("cannot set file times inside configuration snapshot")
 }

I'm not sure of the context this make target runs in - i.e. if it's ever orchestrated with further terraform modifications.

[jason-snyk]

I think we've only ever run it once so far. Digging through the git history, this must have a been added by hand since to make it compatible with the newer version of afero that we use.

[craigfurman]

I've added this to the patch

[craigfurman]

This is a straw man that I need help from maintainers to validate 🙏

[jason-snyk]

I agree that ed isn't the first tool I'd reach for for this purpose - mostly because I'm not experienced with it. Since we have at least one other edit that we need to make, I think we could consider using git apply to do these changes. Like this patch would apply the changes that I was testing with:

diff --git a/pkg/terraform/configs/configload/loader_snapshot.go b/pkg/terraform/configs/configload/loader_snapshot.go
index 6ca8b4f..243939f 100644
--- a/pkg/terraform/configs/configload/loader_snapshot.go
+++ b/pkg/terraform/configs/configload/loader_snapshot.go
@@ -325,6 +325,10 @@ func (fs snapshotFS) Chmod(name string, mode os.FileMode) error {
        return fmt.Errorf("cannot set file mode inside configuration snapshot")
 }
 
+func (fs snapshotFS) Chown(string, int, int) error {
+       return fmt.Errorf("cannot set file owner inside configuration snapshot")
+}
+
 func (fs snapshotFS) Chtimes(name string, atime, mtime time.Time) error {
        return fmt.Errorf("cannot set file times inside configuration snapshot")
 }
diff --git a/pkg/terraform/tfdiags/diagnostics.go b/pkg/terraform/tfdiags/diagnostics.go
index 30476ee..0a11867 100644
--- a/pkg/terraform/tfdiags/diagnostics.go
+++ b/pkg/terraform/tfdiags/diagnostics.go
@@ -107,23 +107,6 @@ func (diags Diagnostics) HasErrors() bool {
        return false
 }
 
-// ForRPC returns a version of the receiver that has been simplified so that
-// it is friendly to RPC protocols.
-//
-// Currently this means that it can be serialized with encoding/gob and
-// subsequently re-inflated. It may later grow to include other serialization
-// formats.
-//
-// Note that this loses information about the original objects used to
-// construct the diagnostics, so e.g. the errwrap API will not work as
-// expected on an error-wrapped Diagnostics that came from ForRPC.
-func (diags Diagnostics) ForRPC() Diagnostics {
-       ret := make(Diagnostics, len(diags))
-       for i := range diags {
-               ret[i] = makeRPCFriendlyDiag(diags[i])
-       }
-       return ret
-}
 
 // Err flattens a diagnostics list into a single Go error, or to nil
 // if the diagnostics list does not include any error-level diagnostics.
diff --git a/pkg/terraform/tfdiags/rpc_friendly.go b/pkg/terraform/tfdiags/rpc_friendly.go
deleted file mode 100644
index 485063b..0000000
--- a/pkg/terraform/tfdiags/rpc_friendly.go
+++ /dev/null
@@ -1,59 +0,0 @@
-package tfdiags
-
-import (
-       "encoding/gob"
-)
-
-type rpcFriendlyDiag struct {
-       Severity_ Severity
-       Summary_  string
-       Detail_   string
-       Subject_  *SourceRange
-       Context_  *SourceRange
-}
-
-// rpcFriendlyDiag transforms a given diagnostic so that is more friendly to
-// RPC.
-//
-// In particular, it currently returns an object that can be serialized and
-// later re-inflated using gob. This definition may grow to include other
-// serializations later.
-func makeRPCFriendlyDiag(diag Diagnostic) Diagnostic {
-       desc := diag.Description()
-       source := diag.Source()
-       return &rpcFriendlyDiag{
-               Severity_: diag.Severity(),
-               Summary_:  desc.Summary,
-               Detail_:   desc.Detail,
-               Subject_:  source.Subject,
-               Context_:  source.Context,
-       }
-}
-
-func (d *rpcFriendlyDiag) Severity() Severity {
-       return d.Severity_
-}
-
-func (d *rpcFriendlyDiag) Description() Description {
-       return Description{
-               Summary: d.Summary_,
-               Detail:  d.Detail_,
-       }
-}
-
-func (d *rpcFriendlyDiag) Source() Source {
-       return Source{
-               Subject: d.Subject_,
-               Context: d.Context_,
-       }
-}
-
-func (d rpcFriendlyDiag) FromExpr() *FromExpr {
-       // RPC-friendly diagnostics cannot preserve expression information because
-       // expressions themselves are not RPC-friendly.
-       return nil
-}
-
-func init() {
-       gob.Register((*rpcFriendlyDiag)(nil))
-}

So like if that was saved as terraform.patch, you could just add git apply terraform.patch to the end of the terraform_gen recipe. I had tested with removing rpc_friendly.go altogether, but you could use the same approach with a smaller edit that just removes the init. My only concern with that would be that without the init, the rest of the rpc functionality might not work at all, so it might not be worth keeping it.

[craigfurman]

re: patch vs ed, you're totally right - git-patch seems like the right choice, let's switch to that. I can't explain my initial choice of ed beyond my Friday brain wanting to have some fun with the standard editor.

My only concern with that would be that without the init, the rest of the rpc functionality might not work at all, so it might not be worth keeping it

Valid concern - I can't see gob actually being used anywhere in the vendored terraform, but of course any code (perhaps elsewhere) that expects gob types to be registered already will break. rpcFriendlyDiag appears to only be used (via makeRPCFriendlyDiag) by ForRPC() in diagnostics.go in the same package. We can remove all of that functionality, as long as it still compiles. I'll try that out.

[craigfurman]

Done - WDYT @jason-snyk?

It sounds like you're pretty sure we don't need this RPC functionality, if I understand correctly?

[craigfurman]

FYI @jason-fugue @moadibfr

[jason-fugue]

LGTM! Thanks for doing this.