LaunchDarkly Token Analyzer

[kashifkhan0771]

Description:

This PR adds a new analyzer for launchdarkly token
OSSTicket: OSS-95

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[ahrav]

Left a couple questions, but otherwise this looks great! Nice work.

[ahrav]

nit (potential alternative solution): A switch is a little more idiomatic, but totally up to you.

Ex:

switch {
case token.Role != "":
    return token.Role
case token.hasInlineRole():
    return "Inline Policy"
case token.hasCustomRoles():
    return "Custom Roles"
default:
    return ""
}

[ahrav]

nit: I've found it helpful to use mu almost like a "hat" for the resource(s) it protects by placing it directly above them. This makes it visually clear what it's guarding and can even replace the need for a comment.

That said, I’m not entirely sure which resources mu is protecting in this case. 🤔 This is definitely an opinionated take, so feel free to ignore it! 🤣

I forget where I first read about this, but here is a separate article about it. 😅

Ex:

type SecretInfo struct {
    ...
    mu sync.RWMutex <-- see, like a "hat" for "Resources"
    Resources []Resource
}

[ahrav]

🤣

[ahrav]

question: Do we actually need the mutex? 🤔 I didn’t notice any concurrent operations, but I might have missed something.

[kashifkhan0771]

I guess you missed the requests.go file. Large diffs are not rendered by default.

[ahrav]

😢 I knew there had to be a reason. Sorry about that.

[kashifkhan0771]

No worries!

[ahrav]

Sorry I missed some of the files in my initial review.

I left a few suggestions, but only one of them is really blocking. The rest of this looks great. Nice job!

[ahrav]

suggestion: If we use a []error, we can avoid repeatedly converting between string → error → string → error.

optional: Since aggregatedErrs isn’t used in any of the goroutines, moving its declaration right before it's built makes the code a bit easier to read—no need to jump back and forth.

Ex:

var aggregatedErrs []error
for err := range errChan {
    aggregatedErrs = append(aggregatedErrs, err)
}

if len(aggregatedErrs) > 0 {
    return errors.Join(aggregatedErrs...)
}

[ahrav]

suggestion: It looks like the current approach has a potential deadlock issue.

Using an unbuffered error channel means that if there’s no active consumer, any goroutine trying to send an error will block indefinitely. In this case, the consumer for errChan only starts after wg.Wait(), meaning a goroutine could be stuck waiting to send, preventing wg.Wait() from ever completing.

Possible Solutions:

1. Buffer the error channel – This would allow it to accommodate all potential errors. However, since the number of projects is unknown when this function is called, we don’t know how many errors to expect. We could use an arbitrarily large buffer size, but that comes with extra memory overhead.
2. Consume errors as they occur – Instead of trying to predict the number of errors, we could read from the channel continuously and build aggregatedErrs dynamically. I’d personally lean toward this approach, but I’ll leave it up to you.

Ex:

func CaptureResources(client *http.Client, token string, secretInfo *SecretInfo) error {
	errChan := make(chan error, 1)

	// Aggregator goroutine: collects errors as they come.
	var aggregatedErrs []error
	var errAggWg sync.WaitGroup
	errAggWg.Add(1)
	go func() {
		defer errAggWg.Done()
		for err := range errChan {
			aggregatedErrs = append(aggregatedErrs, err)
		}
	}()

	var wg sync.WaitGroup
	// Helper to launch tasks concurrently.
	launch := func(task func() error) {
		wg.Add(1)
		go func() {
			defer wg.Done()
			if err := task(); err != nil {
				errChan <- err
			}
		}()
	}

	// Launch top-level tasks.
	launch(func() error { return captureApplications(client, token, secretInfo) })
	launch(func() error { return captureRepositories(client, token, secretInfo) })
	// Capture projects.
	launch(func() error {
		if err := captureProjects(client, token, secretInfo); err != nil {
			return err
		}
		projects := secretInfo.listResourceByType(projectKey)
		for _, proj := range projects {
			launch(func() error { return captureProjectFeatureFlags(client, token, proj, secretInfo) })
			launch(func() error { return captureProjectEnv(client, token, proj, secretInfo) })
		}
		return nil
	})
	launch(func() error { return captureMembers(client, token, secretInfo) })
	launch(func() error { return captureDestinations(client, token, secretInfo) })
	launch(func() error { return captureTemplates(client, token, secretInfo) })
	launch(func() error { return captureTeams(client, token, secretInfo) })
	launch(func() error { return captureWebhooks(client, token, secretInfo) })

	wg.Wait()
	close(errChan)
	errAggWg.Wait()

	if len(aggregatedErrs) > 0 {
		return errors.Join(aggregatedErrs...)
	}
	return nil
}

[kashifkhan0771]

Done ✅

[ahrav]

suggestion/question: Do you think we should use a context with a timeout (ctx) to prevent any API calls from hanging? It might help avoid cases where a request gets stuck indefinitely.
Ex:

const defaultTimeout = 10 * time.Second // I chose this value arbitrarily
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
defer cancel()

// create request
req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+endpoint, http.NoBody)
...

[ahrav]

I like this pattern. Makes finding all the endpoints so much easier.

[ahrav]

question: Do we actually need the updateResourceMetadata method on Resource?

It looks like all it does is check if the map is nil, initialize it if needed, and then set entries. Could we just handle this inline when constructing the Resource above? That would eliminate the need for nil checks altogether.

Am I missing a different use case?

Similar question for this pattern in the functions below for the other resource types.

Ex:

resource := Resource{
    ID:   fmt.Sprintf("launchdarkly/app/%s", application.Key),
    Name: application.Name,
    Type: applicationKey,
    MetaData: map[string]string{
	    MetadataKey: application.Key,
	    "Maintainer Email": application.Maintainer.Email,
	    "Kind": application.Kind,
    },
}

secretInfo.appendResource(resource)

[kashifkhan0771]

I guess I was trying to make the code a bit more fancy. 😆

[ahrav]

I do like the idea of encapsulating all the logic for updating Resource in one place, but since we’re not using a constructor and just populating it directly, doing it inline made the review a bit easier.

Also, that GIF is way too relatable—I feel that on a personal level. 🤣

[ahrav]

nit: strconv.Itoa(repository.Version) is a bit more idiomatic for converting a single int to a string. It’s also slightly more performant, though that’s probably not a big deal here 😅.

[ahrav]

nit: ditto strconv.Itoa(destination.Version)

[ahrav]

nit: strconv.Itoa for these as well.

[ahrav]

question: How come these three are raw strings instead of constants like the others? 🤔 If it was done on purpose, would you mind just including a brief comment as to why.

[kashifkhan0771]

Yes, this is intentional. These three APIs are specifically for user information, not resources. Additionally, these keys are used only once, unlike resource keys, which are used in multiple places. I'll add a comment to clarify this.

[ahrav]

I feel like this will be a pretty commonly used Analyzer, awesome work!