2515 Adding import and export #2571

jensalm · 2024-12-03T20:55:08Z

Support in the cli to import entities into a network and exporting entities from a network. Supports both json and yaml.

closes #2515

dovholuknf · 2025-01-07T12:58:01Z

go.mod

@@ -102,6 +102,8 @@ require (
 	github.com/MichaelMure/go-term-text v0.3.1 // indirect
 	github.com/alecthomas/chroma v0.10.0 // indirect
 	github.com/andybalholm/brotli v1.1.0 // indirect
+	github.com/antchfx/jsonquery v1.3.6 // indirect


Why are these dependencies necessary? Maybe we can discuss?

it's used to select specific nodes out of the file input and it's used in the tests to compare the json outcomes.

zititest/go.sum

dovholuknf · 2025-01-07T12:59:54Z

ziti/cmd/edge/quickstart.go

@@ -200,6 +200,7 @@ func (o *QuickstartOpts) run(ctx context.Context) {
 		tmpDir, _ := os.MkdirTemp("", "quickstart")
 		o.Home = tmpDir
 		o.cleanOnExit = true
+		logrus.Infof("temporary --home '%s'", o.Home)


probably unintentional?

it is the counterpart to a log statement in the else which logs the permanent home in a similar way

dovholuknf · 2025-01-07T13:00:16Z

ziti/cmd/cmd.go

@@ -145,6 +147,8 @@ func NewCmdRoot(in io.Reader, out, err io.Writer, cmd *cobra.Command) *cobra.Com
 	opsCommands.AddCommand(NewUnwrapIdentityFileCommand(out, err))
 	opsCommands.AddCommand(verify.NewVerifyNetwork(out, err))
 	opsCommands.AddCommand(verify.NewVerifyTraffic(out, err))
+	opsCommands.AddCommand(download.NewDownloadCmd(out, err))


Rename to Import/Export to keep consistent pls

import is a protected word so i can't unless you have a different suggestion?

NewImportCmd/NewExportCmd

ziti/cmd/ascode/download/output.go

dovholuknf · 2025-01-07T13:03:42Z

internal/ascode/cache_util.go

+
+type CacheGetter func(id string) (interface{}, error)
+
+func GetItemFromCache(c *cache.Cache, key string, fn CacheGetter) (interface{}, error) {


why not just use a map and remove this and the dependencies? This "doing" something?

dovholuknf · 2025-01-07T13:04:59Z

internal/rest/mgmt/helpers.go

+		log.Errorf("Could not obtain an ID for the service policy with filter %s: %v", filter, err)
+		return nil
+	}
+	if resp == nil || resp.Payload == nil || resp.Payload.Data == nil || len(resp.Payload.Data) == 0 {


if resp == nil || resp.Payload == nil || resp.Payload.Data == nil || len(resp.Payload.Data) == 0 {

is repeated verbatim a few times (10). Feels like a candidate to make a small validator function to me

dovholuknf · 2025-01-07T13:05:50Z

internal/rest/mgmt/helpers.go

+		Filter:  &filter,
+		Context: context.Background(),
+	}
+	params.SetTimeout(5 * time.Second)


these 5 * time.Second should at very least references a single constant. might be worthwhile making it configurable some day (not now)

dovholuknf · 2025-01-07T13:08:38Z

ziti/cmd/ascode/ascode_test.go

+		return
+	}
+
+	externalJwtSigner1 := jsonquery.FindOne(doc, "//externalJwtSigners/*[name='NetFoundry Console Integration External JWT Signer']")


would you add a test case for whatever the issue was with "handling missing identities" from 498c206 ?

dovholuknf · 2025-01-07T13:09:25Z

ziti/cmd/ascode/download/download.go

+type Download struct {
+	loginOpts edge.LoginOptions
+	client    *rest_management_api_client.ZitiEdgeManagement
+


would you remove the whitespace so these things sort alphabetically and are indented similarly?

dovholuknf · 2025-01-07T13:10:34Z

ziti/cmd/ascode/download/download.go

+	loginOpts edge.LoginOptions
+	client    *rest_management_api_client.ZitiEdgeManagement
+
+	verbose bool


this is covered in edge.LoginOptions but it's Verbose there. pls remove

dovholuknf · 2025-01-07T13:11:06Z

ziti/cmd/ascode/download/download.go

+	authPolicyCache  *cache.Cache
+	externalJwtCache *cache.Cache
+
+	Out io.Writer


also covered in edge.LoginOptions i believe, no?

dovholuknf · 2025-01-07T13:12:59Z

ziti/cmd/ascode/download/download.go

+
+var output Output
+
+func NewDownload(loginOpts edge.LoginOptions, client *rest_management_api_client.ZitiEdgeManagement, ofJson bool, ofYaml bool, file *os.File, filename string) Download {


this should be removed. this isn't idiomatic go. Since Download is public, people can just make a new struct just like you did and set all the things on their own.

dovholuknf · 2025-01-07T13:17:11Z

ziti/cmd/ascode/download/download.go

+	result := map[string]interface{}{}
+
+	all := slices.Contains(args, "all") || len(args) == 0
+	if all ||


this whole section is confusing to the 'first time reader' (me). Can we discuss what's going on here with these or's?

dovholuknf · 2025-01-07T13:17:42Z

ziti/cmd/ascode/download/download.go

+	if all ||
+		slices.Contains(args, "ca") || slices.Contains(args, "cas") ||
+		slices.Contains(args, "certificate-authority") || slices.Contains(args, "certificate-authorities") {
+		if d.verbose {


if d.verbose is not needed. simply call log.debug. same comment -- multiple instances

dovholuknf · 2025-01-07T13:18:37Z

ziti/cmd/ascode/download/download.go

+		}
+		cas, err := d.GetCertificateAuthorities()
+		if err != nil {
+			return err


does this mean it stops at the first error? that could be .... annoying if it takes a while for this to run?

dovholuknf · 2025-01-07T13:20:26Z

ziti/cmd/ascode/download/download.go

+	"github.com/openziti/ziti/internal"
+	"github.com/openziti/ziti/internal/rest/mgmt"
+	"github.com/openziti/ziti/ziti/cmd/edge"
+	c "github.com/openziti/ziti/ziti/constants"


was there another "constants" that needed to be aliased?

dovholuknf · 2025-01-07T13:59:55Z

ziti/cmd/ascode/download/download_config_types.go

+
+			// convert to a map of values
+			m := d.ToMap(item)
+			d.Filter(m, []string{"id", "_links", "createdAt", "updatedAt"})


d.Filter(m, []string{"id", "_links", "createdAt", "updatedAt"}) seems like a good candidate to refactor imo

dovholuknf · 2025-01-07T14:00:26Z

ziti/cmd/ascode/download/download_edgerouter_policies.go

+
+			// filter unwanted properties
+			d.Filter(m, []string{"id", "_links", "createdAt", "updatedAt",
+				"edgeRouterRolesDisplay", "identityRolesDisplay", "isSystem"})


"isSystem" is really something we would want to filter? should any "system" entities be exported/imported at all?

dovholuknf · 2025-01-07T14:04:06Z

ziti/cmd/ascode/download/download_certificate_authorities.go

+			m := d.ToMap(item)
+
+			// filter unwanted properties
+			d.Filter(m, []string{"id", "_links", "createdAt", "updatedAt"})


CAs should probably come along with a warning, indicating the user is going to need to re-verify the CA on import. it might be there, but if not, that should be added to avoid the confusion when they import and the ca is not "verified"

dovholuknf · 2025-01-07T14:14:54Z

ziti/cmd/ascode/download/download_services.go

+	"github.com/openziti/edge-api/rest_management_api_client/config"
+	"github.com/openziti/edge-api/rest_management_api_client/service"
+	"github.com/openziti/edge-api/rest_model"
+	common "github.com/openziti/ziti/internal/ascode"


as a reader, it feels like it would be better to see all these "common" aliases removed in favor of using "ascode".

dovholuknf · 2025-01-07T14:18:21Z

ziti/cmd/ascode/upload/upload.go

+	client    *rest_management_api_client.ZitiEdgeManagement
+	reader    Reader
+
+	verbose bool


all the same comments as on Export (also rename to Import/Export? which are not reserved as they are capitalized)

dovholuknf · 2025-01-07T14:19:15Z

ziti/cmd/ascode/download/download.go

+
+	cmd := &cobra.Command{
+		Use:   "export [entity]",
+		Short: "Export Ziti entities",


mind removing the " ziti " parts of these descriptions? We know it's a ziti entity :)

dovholuknf · 2025-01-07T14:22:36Z

ziti/cmd/ascode/upload/upload.go

+	authPolicies := map[string]string{}
+	if all ||
+		slices.Contains(args, "auth-policy") || slices.Contains(args, "auth-policies") ||
+		slices.Contains(args, "identity") || slices.Contains(args, "identities") {


i feel like these or's should be changed into functions and tests added for them...

IdentitiesNeeded(args)

AuthPoliciesNeeded(args)

ServicesNeeded(args)

etc. i think it'll clean up the or's and it'll make it testable

jensalm added 2 commits December 3, 2024 15:51

2515 Added import and export

3fe67b8

2515 fixed lint issues

d5d0737

jensalm requested review from a team as code owners December 3, 2024 20:55

jensalm added 4 commits December 3, 2024 16:03

2515 fixed earlier commit

1a895a2

2515 Fixed verbose flag. Added output when importing.

8f3c4e5

2515 Fixed lint issues.

bcd157b

2515 Fixed formatting

bd1bfe2

dovholuknf mentioned this pull request Jan 6, 2025

Add support for exporting and importing all entities #2515

Open

jensalm added 2 commits January 6, 2025 19:44

2515 Fixed input reference. Fixed error handling for missing identities.

498c206

Merge branch 'main' into 2515-import-export

a5f813c

dovholuknf reviewed Jan 7, 2025

View reviewed changes

zititest/go.sum Show resolved Hide resolved

dovholuknf reviewed Jan 7, 2025

View reviewed changes

ziti/cmd/ascode/download/output.go Show resolved Hide resolved

dovholuknf reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2515 Adding import and export #2571

2515 Adding import and export #2571

jensalm commented Dec 3, 2024 •

edited by dovholuknf

Loading

dovholuknf Jan 7, 2025

jensalm Jan 7, 2025 •

edited

Loading

dovholuknf Jan 7, 2025

jensalm Jan 7, 2025

dovholuknf Jan 7, 2025

jensalm Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025

dovholuknf Jan 7, 2025


		type CacheGetter func(id string) (interface{}, error)

		func GetItemFromCache(c *cache.Cache, key string, fn CacheGetter) (interface{}, error) {


		var output Output

		func NewDownload(loginOpts edge.LoginOptions, client rest_management_api_client.ZitiEdgeManagement, ofJson bool, ofYaml bool, file os.File, filename string) Download {

2515 Adding import and export #2571

Are you sure you want to change the base?

2515 Adding import and export #2571

Conversation

jensalm commented Dec 3, 2024 • edited by dovholuknf Loading

Choose a reason for hiding this comment

jensalm Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensalm commented Dec 3, 2024 •

edited by dovholuknf

Loading

jensalm Jan 7, 2025 •

edited

Loading