feat(dogtags): Adds DogTags functionality for SKU Flagging BED-5560 #2147
Conversation
WalkthroughAdds a typed DogTags subsystem: provider interface with a Noop implementation, a Service that returns typed flags with defaults, typed dogtag definitions and defaults, tests asserting defaults, startup aggregation/logging of all dogtags, and API wiring to expose dogtags via GET /api/v2/dogtags. Changes
sequenceDiagram
autonumber
participant Entrypoint
participant DogTagsService as DogTagsService
participant Provider
participant Logger
participant API
participant Client
Entrypoint->>DogTagsService: NewDefaultService()
Note right of DogTagsService `#E6F7FF`: constructed with NoopProvider
Entrypoint->>DogTagsService: GetAllDogTags()
DogTagsService->>Provider: GetFlagAsBool/Int/String (per tag)
alt Provider returns value
Provider-->>DogTagsService: value
else Provider returns ErrProviderNotImplemented / error
Provider-->>DogTagsService: error
DogTagsService-->>DogTagsService: use registry default
end
DogTagsService-->>Entrypoint: map[string]any (all tags)
Entrypoint->>Logger: log each key/value
Entrypoint->>API: pass dogtags.Service into Resources
Client->>API: GET /api/v2/dogtags
API->>DogTagsService: GetAllDogTags()
DogTagsService-->>API: map[string]any
API-->>Client: 200 + dogtags map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
kpom-specter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cmd/api/src/services/entrypoint.go (1)
82-88: Prefer structured slog logging over per-flag fmt.Sprintf loopCreating the default service and logging dogtags at startup looks fine, but you can simplify and make logs more structured by logging the entire map (or key/value fields) instead of doing a per-entry fmt.Sprintf:
- flags := dogtagsService.GetAllDogTags() - slog.InfoContext(ctx, "DogTags Configuration:") - for key, value := range flags { - slog.InfoContext(ctx, fmt.Sprintf(" • %s = %v", key, value)) - } + flags := dogtagsService.GetAllDogTags() + slog.InfoContext(ctx, "DogTags configuration", "flags", flags)This avoids the extra fmt.Sprintf and makes it easier to query dogtag values from structured logs later.
cmd/api/src/services/dogtags/service_test.go (1)
8-30: Strengthen assertions aroundGetAllDogTagsand consider splitting testsThis test correctly validates the default values, but
GetAllDogTagsis only exercised viat.Logfand not asserted. You could tighten coverage by checking that the map contains the expected keys/values (and maybe size), and optionally split into focused tests (e.g.,TestDefaultsvsTestGetAllDogTags) or a small table-driven test for the int flags.For example:
all := svc.GetAllDogTags() if got := all[string(PZ_TIER_LIMIT)]; got != int64(1) { t.Errorf("GetAllDogTags tier_limit = %v, want 1", got) }cmd/api/src/services/dogtags/service.go (3)
16-23: Guard against a nilProviderinNewServiceIf a caller ever passes
nilintoNewService, any subsequent method call will panic ons.provider.Get*. Consider defaultingniltoNewNoopProvider()(or rejecting it) to make the API safer:func NewService(provider Provider) Service { - return &service{provider: provider} + if provider == nil { + provider = NewNoopProvider() + } + return &service{provider: provider} }
25-51: Differentiate “not found” from other provider errors before falling back to defaultsAll
Get*methods currently treat any provider error as “use the default,” which makes genuine provider failures (e.g., transport, auth, misconfig) indistinguishable from a missing key and silently masks them.Consider introducing/using a sentinel “not found” error from the provider (separate from a provider-not-configured error) and only falling back to defaults for that case, while logging or surfacing other errors. This will make configuration issues easier to detect without changing the
Serviceinterface signature.
53-69: Preallocate the result map inGetAllDogTags
GetAllDogTagsknows exactly how many entries it will insert and can preallocate the map capacity to avoid small reallocations in hot paths:-func (s *service) GetAllDogTags() map[string]any { - result := make(map[string]any) +func (s *service) GetAllDogTags() map[string]any { + result := make(map[string]any, + len(AllBoolDogTags)+len(AllStringDogTags)+len(AllIntDogTags)+len(AllFloatDogTags))The rest of the function can remain unchanged.
cmd/api/src/services/dogtags/sku_flags.go (1)
36-51: Consider makingAll*DogTagseffectively read‑only to callersThe
AllBoolDogTags,AllIntDogTags,AllStringDogTags, andAllFloatDogTagsmaps are exported and mutable, so any external package can change defaults or add/remove dogtags at runtime. If these collections are intended as authoritative, static registries, consider:
- Making the maps unexported and exposing read-only accessors/iterators, or
- Documenting that mutation is allowed and part of the design.
Locking this down will prevent surprising runtime changes to flag behavior coming from unrelated packages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/api/src/services/dogtags/provider.go(1 hunks)cmd/api/src/services/dogtags/service.go(1 hunks)cmd/api/src/services/dogtags/service_test.go(1 hunks)cmd/api/src/services/dogtags/sku_flags.go(1 hunks)cmd/api/src/services/entrypoint.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/services/dogtags/service_test.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/services/entrypoint.go
🧬 Code graph analysis (3)
cmd/api/src/services/dogtags/service_test.go (2)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(21-23)cmd/api/src/services/dogtags/sku_flags.go (3)
PZ_TIER_LIMIT(32-32)PZ_LABEL_LIMIT(33-33)PZ_MULTI_TIER_ANALYSIS(30-30)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(21-23)
cmd/api/src/services/dogtags/sku_flags.go (1)
packages/go/graphschema/common/common.go (1)
Description(55-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
irshadaj
left a comment
irshadaj
left a comment
There was a problem hiding this comment.
Appoved pending the naming changes discussed over on BHE
kpom-specter
force-pushed
the
kpom/dogtags-m3/BED-6650
branch
from
aef4324 to
43c3979
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/api/src/services/dogtags/service.go (1)
25-25: Note: Unresolved feedback from previous review.A previous reviewer (irshadaj) suggested renaming these functions to align with enterprise suggestions. Please verify whether this feedback has been addressed or if the current naming (
GetBool,GetString,GetInt,GetFloat) is the agreed-upon convention.
🧹 Nitpick comments (6)
cmd/api/src/services/entrypoint.go (1)
82-88: Consider refactoring for cleaner code and deterministic output.The current implementation works but could be improved:
- The
dogtagsServicevariable could be inlined since it's only used once.- Consider using structured logging instead of
fmt.Sprintffor better log parsing.- Map iteration order is non-deterministic; sorting keys would provide consistent log output.
🔎 View suggested refactor
- dogtagsService := dogtags.NewDefaultService() - - flags := dogtagsService.GetAllDogTags() + flags := dogtags.NewDefaultService().GetAllDogTags() slog.InfoContext(ctx, "DogTags Configuration:") - for key, value := range flags { - slog.InfoContext(ctx, fmt.Sprintf(" • %s = %v", key, value)) + + // Sort keys for deterministic output + keys := make([]string, 0, len(flags)) + for k := range flags { + keys = append(keys, k) + } + sort.Strings(keys) + + for _, key := range keys { + slog.InfoContext(ctx, " dogtag", "key", key, "value", flags[key]) }Note: You'll need to add
"sort"to the imports if you adopt the sorted approach.cmd/api/src/services/dogtags/service_test.go (1)
8-30: Consider test improvements for better coverage and idiomatic Go.The test provides good basic coverage of the default service. A few optional improvements:
- Line 23:
if multiTier != falsecan be simplified toif multiTier.- The
GetAllDogTagsresult is logged but not validated—consider asserting that expected keys are present.- Using subtests would improve organization and failure reporting.
🔎 View suggested refactor
func TestService(t *testing.T) { svc := NewDefaultService() - // Test typed getters return defaults - tierLimit := svc.GetInt(PZ_TIER_LIMIT) - if tierLimit != 1 { - t.Errorf("expected 1, got %d", tierLimit) - } - - labelLimit := svc.GetInt(PZ_LABEL_LIMIT) - if labelLimit != 10 { - t.Errorf("expected 10, got %d", labelLimit) - } - - multiTier := svc.GetBool(PZ_MULTI_TIER_ANALYSIS) - if multiTier != false { - t.Errorf("expected false, got %v", multiTier) - } - - // Test GetAllDogTags - all := svc.GetAllDogTags() - t.Logf("All dogtags: %+v", all) + t.Run("GetInt returns defaults", func(t *testing.T) { + if got := svc.GetInt(PZ_TIER_LIMIT); got != 1 { + t.Errorf("PZ_TIER_LIMIT: expected 1, got %d", got) + } + if got := svc.GetInt(PZ_LABEL_LIMIT); got != 10 { + t.Errorf("PZ_LABEL_LIMIT: expected 10, got %d", got) + } + }) + + t.Run("GetBool returns defaults", func(t *testing.T) { + if got := svc.GetBool(PZ_MULTI_TIER_ANALYSIS); got { + t.Errorf("PZ_MULTI_TIER_ANALYSIS: expected false, got %v", got) + } + }) + + t.Run("GetAllDogTags includes all keys", func(t *testing.T) { + all := svc.GetAllDogTags() + expectedKeys := []string{ + string(PZ_TIER_LIMIT), + string(PZ_LABEL_LIMIT), + string(PZ_MULTI_TIER_ANALYSIS), + } + for _, key := range expectedKeys { + if _, exists := all[key]; !exists { + t.Errorf("expected key %q not found in GetAllDogTags", key) + } + } + t.Logf("All dogtags: %+v", all) + }) }cmd/api/src/services/dogtags/sku_flags.go (2)
9-27: Consider enhancing descriptions for better documentation.The spec structs are well-structured with JSON tags, but the
Descriptionfields in the actual map entries (lines 37, 41-42) are quite brief. Consider providing more detailed descriptions that explain:
- What the flag controls
- When it should be enabled/disabled
- Any constraints or valid ranges
This would be especially valuable if these specs are exposed via an API or used for documentation generation.
45-51: Empty maps are fine for WIP but consider cleanup before final merge.The empty
AllStringDogTagsandAllFloatDogTagsmaps with "none yet" comments serve as placeholders for future flags. This is acceptable for a WIP PR, but consider whether to keep or remove them before final merge depending on the timeline for string/float dogtags.cmd/api/src/services/dogtags/service.go (2)
25-51: Consider logging provider errors for debugging.All
GetXmethods silently ignore errors from the provider (checkingerr == niland falling through to defaults). While this is acceptable for normal operation, consider logging provider errors at a debug or warn level to aid troubleshooting when a provider is expected to supply values but fails.🔎 Example for GetBool
func (s *service) GetBool(key BoolDogTag) bool { - if val, err := s.provider.GetBool(string(key)); err == nil { + val, err := s.provider.GetBool(string(key)) + if err == nil { return val + } else if !errors.Is(err, ProviderNotImplemented) { + slog.Debug("provider error, using default", "key", key, "error", err) } return AllBoolDogTags[key].Default }Apply similar changes to
GetString,GetInt, andGetFloat.
53-70: Consider preallocating map capacity for minor efficiency gain.The map could be preallocated with a known capacity since the number of dogtags is determined by the size of the four
AllXDogTagsmaps.🔎 Apply this diff
func (s *service) GetAllDogTags() map[string]any { - result := make(map[string]any) + capacity := len(AllBoolDogTags) + len(AllStringDogTags) + len(AllIntDogTags) + len(AllFloatDogTags) + result := make(map[string]any, capacity) for key := range AllBoolDogTags { result[string(key)] = s.GetBool(key) } for key := range AllStringDogTags { result[string(key)] = s.GetString(key) } for key := range AllIntDogTags { result[string(key)] = s.GetInt(key) } for key := range AllFloatDogTags { result[string(key)] = s.GetFloat(key) } return result }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/api/src/services/dogtags/provider.go(1 hunks)cmd/api/src/services/dogtags/service.go(1 hunks)cmd/api/src/services/dogtags/service_test.go(1 hunks)cmd/api/src/services/dogtags/sku_flags.go(1 hunks)cmd/api/src/services/entrypoint.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/services/dogtags/provider.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/services/dogtags/service_test.go
🧬 Code graph analysis (3)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(21-23)
cmd/api/src/services/dogtags/service.go (2)
cmd/api/src/services/dogtags/sku_flags.go (8)
BoolDogTag(4-4)StringDogTag(5-5)IntDogTag(6-6)FloatDogTag(7-7)AllBoolDogTags(36-38)AllStringDogTags(45-47)AllIntDogTags(40-43)AllFloatDogTags(49-51)cmd/api/src/services/dogtags/provider.go (2)
Provider(11-16)NewNoopProvider(22-24)
cmd/api/src/services/dogtags/service_test.go (2)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(21-23)cmd/api/src/services/dogtags/sku_flags.go (3)
PZ_TIER_LIMIT(32-32)PZ_LABEL_LIMIT(33-33)PZ_MULTI_TIER_ANALYSIS(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (4)
cmd/api/src/services/entrypoint.go (1)
41-41: LGTM!The import is correctly added to support the dogtags service initialization.
cmd/api/src/services/dogtags/sku_flags.go (1)
4-7: LGTM!The typed dogtag key aliases provide excellent compile-time type safety, preventing callers from mixing boolean, string, integer, and float dogtags. This is a clean, idiomatic approach.
cmd/api/src/services/dogtags/service.go (2)
4-10: LGTM!The
Serviceinterface provides excellent type safety by using typed dogtag keys (BoolDogTag, IntDogTag, etc.) for each getter. The design prevents type confusion at compile time while allowing flexible aggregation viaGetAllDogTags.
16-23: LGTM!The constructor pattern is clean and well-documented.
NewDefaultServiceprovides a sensible default whileNewServiceallows dependency injection of custom providers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/api/src/services/dogtags/service.go (1)
24-43: Panic risk: Unguarded map access in all getter methods.Lines 28, 35, and 42 directly index into the
AllBoolDogTags,AllStringDogTags, andAllIntDogTagsmaps without checking key existence. If any caller passes a typed constant that's not registered in the corresponding map, the application will panic.This issue was previously flagged for
GetFlagAsBooland remains unresolved. The same vulnerability exists inGetFlagAsStringandGetFlagAsInt.🔎 Apply defensive map lookups for all getters
func (s *service) GetFlagAsBool(key BoolDogTag) bool { if val, err := s.provider.GetFlagAsBool(string(key)); err == nil { return val } - return AllBoolDogTags[key].Default + if spec, ok := AllBoolDogTags[key]; ok { + return spec.Default + } + return false // safe zero value for missing keys } func (s *service) GetFlagAsString(key StringDogTag) string { if val, err := s.provider.GetFlagAsString(string(key)); err == nil { return val } - return AllStringDogTags[key].Default + if spec, ok := AllStringDogTags[key]; ok { + return spec.Default + } + return "" // safe zero value for missing keys } func (s *service) GetFlagAsInt(key IntDogTag) int64 { if val, err := s.provider.GetFlagAsInt(string(key)); err == nil { return val } - return AllIntDogTags[key].Default + if spec, ok := AllIntDogTags[key]; ok { + return spec.Default + } + return 0 // safe zero value for missing keys }
🧹 Nitpick comments (1)
cmd/api/src/services/dogtags/service.go (1)
15-17: Consider adding nil provider check.For defensive coding, consider validating that the provider is not nil. This would prevent confusing panics if the constructor is called incorrectly.
🔎 Optional defensive nil check
func NewService(provider Provider) Service { + if provider == nil { + panic("dogtags: nil provider") + } return &service{provider: provider} }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/services/dogtags/provider.go(1 hunks)cmd/api/src/services/dogtags/service.go(1 hunks)cmd/api/src/services/dogtags/service_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/services/dogtags/provider.go
- cmd/api/src/services/dogtags/service_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/services/dogtags/service.go
🧬 Code graph analysis (1)
cmd/api/src/services/dogtags/service.go (2)
cmd/api/src/services/dogtags/sku_flags.go (6)
BoolDogTag(4-4)StringDogTag(5-5)IntDogTag(6-6)AllBoolDogTags(36-38)AllStringDogTags(45-47)AllIntDogTags(40-43)cmd/api/src/services/dogtags/provider.go (2)
Provider(11-15)NewNoopProvider(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (2)
cmd/api/src/services/dogtags/service.go (2)
4-13: Clean interface design with type-safe dogtag access.The Service interface and implementation struct are well-designed. The typed keys (BoolDogTag, StringDogTag, IntDogTag) provide type safety and the Provider pattern allows for pluggable implementations.
19-22: Well-implemented default constructor and aggregation method.
NewDefaultService()provides a clean way to create a service with default behavior.GetAllDogTags()is correctly implemented—it safely iterates through the dogtag maps and aggregates all values. Since the keys are sourced from the maps themselves, there's no panic risk here.Also applies to: 45-59
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/services/entrypoint.go (1)
82-88: Use structured logging and deterministic ordering for dogtag configuration output.The current implementation has several issues:
- Using
fmt.Sprintfwithinslog.InfoContextbypasses structured logging benefits- Each flag is logged separately, which is inefficient and harder to parse in log aggregation systems
- Map iteration order is non-deterministic in Go, making log output unpredictable for debugging and testing
Consider refactoring to use structured logging with sorted keys.
🔎 Proposed refactor for structured logging
dogtagsService := dogtags.NewDefaultService() flags := dogtagsService.GetAllDogTags() - slog.InfoContext(ctx, "DogTags Configuration:") + + // Sort keys for deterministic output + keys := make([]string, 0, len(flags)) for key, value := range flags { - slog.InfoContext(ctx, fmt.Sprintf(" • %s = %v", key, value)) + keys = append(keys, key) } + sort.Strings(keys) + + // Log with structured fields + attrs := make([]slog.Attr, 0, len(flags)) + for _, key := range keys { + attrs = append(attrs, slog.Any(key, flags[key])) + } + slog.LogAttrs(ctx, slog.LevelInfo, "DogTags Configuration", attrs...)Note: You'll need to import "sort" if not already imported.
Based on learnings, slog should be used with structured fields for consistency with the codebase's logging standards.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/api/src/api/registration/registration.go(3 hunks)cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/dogtags.go(1 hunks)cmd/api/src/api/v2/model.go(4 hunks)cmd/api/src/services/entrypoint.go(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/services/entrypoint.go
🧬 Code graph analysis (2)
cmd/api/src/api/v2/dogtags.go (2)
cmd/api/src/api/v2/model.go (1)
Resources(106-120)cmd/api/src/api/marshalling.go (1)
WriteBasicResponse(90-99)
cmd/api/src/services/entrypoint.go (2)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(20-22)cmd/api/src/api/registration/registration.go (1)
RegisterFossRoutes(55-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (5)
cmd/api/src/api/registration/v2.go (1)
140-141: LGTM! DogTags API route properly registered.The new GET /api/v2/dogtags endpoint is correctly integrated with authentication requirements and follows the established V2 API routing pattern.
cmd/api/src/services/entrypoint.go (1)
133-133: LGTM! DogTags service correctly wired.The dogtagsService is properly passed to RegisterFossRoutes, completing the dependency injection chain.
cmd/api/src/api/v2/dogtags.go (1)
25-28: LGTM! Handler implementation is clean and follows established patterns.The GetDogTags handler correctly retrieves all dogtag flags via the service and returns them using the standard WriteBasicResponse pattern.
cmd/api/src/api/registration/registration.go (1)
32-32: LGTM! DogTags service properly integrated into route registration.The import, function signature update, and service wiring to v2.NewResources are all correctly implemented and follow the established dependency injection pattern.
Also applies to: 66-66, 85-85
cmd/api/src/api/v2/model.go (1)
29-29: LGTM! DogTags service correctly added to Resources.The import, struct field addition, constructor signature update, and field initialization all follow the established pattern for service dependencies in the Resources struct.
Also applies to: 119-119, 132-132, 147-147
kpom-specter
changed the title
kpom-specter
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/api/src/services/dogtags/provider.go (1)
32-34: Update comment to reference correct error name.The comment references
ErrNotFound, but the actual error returned isErrProviderNotImplemented(defined at line 21).🔎 Fix the comment
-// NoopProvider returns ErrNotFound for all keys. +// NoopProvider returns ErrProviderNotImplemented for all keys. // The service will use defaults for everything. type NoopProvider struct{}cmd/api/src/services/dogtags/service.go (1)
39-44: Add defensive map lookup to prevent panic.Direct map access on line 43 will panic if the key doesn't exist in
AllBoolDogTags. This issue was flagged in a previous review and remains unaddressed.🔎 Apply defensive checking
func (s *service) GetFlagAsBool(key BoolDogTag) bool { if val, err := s.provider.GetFlagAsBool(string(key)); err == nil { return val } - return AllBoolDogTags[key].Default + if spec, ok := AllBoolDogTags[key]; ok { + return spec.Default + } + return false // safe fallback for unregistered keys }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/services/dogtags/provider.gocmd/api/src/services/dogtags/service.gocmd/api/src/services/dogtags/service_test.gocmd/api/src/services/dogtags/sku_flags.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/services/dogtags/service_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/services/dogtags/service.go
🧬 Code graph analysis (2)
cmd/api/src/services/dogtags/sku_flags.go (1)
packages/go/graphschema/common/common.go (1)
Description(55-55)
cmd/api/src/services/dogtags/service.go (2)
cmd/api/src/services/dogtags/sku_flags.go (6)
BoolDogTag(19-19)StringDogTag(20-20)IntDogTag(21-21)AllBoolDogTags(52-55)AllStringDogTags(62-64)AllIntDogTags(57-60)cmd/api/src/services/dogtags/provider.go (2)
Provider(26-30)NewNoopProvider(36-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-go-unit-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
There was a problem hiding this comment.
blocking because the default is incorrect for the label limit for PZ. I would also advise not merging until both sides of this (BHCE / BHE) are fully approved and functioning as this change impacts entrypoint logic.
My last questions are more generic:
how / what is the expectation for the logic that uses these dogtags?
Is there going to be follow-up work to migrate the logic to these or does this changeset need them?
What is the expectation for the old parameters table that houses these fields, should those records be deleted?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/registration/v2.gocmd/api/src/services/dogtags/sku_flags.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/api/registration/v2.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/api/src/services/dogtags/sku_flags.go (1)
packages/go/graphschema/common/common.go (1)
Description(55-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (3)
cmd/api/src/services/dogtags/sku_flags.go (3)
18-36: LGTM: Type-safe flag system is well designed.The typed flag keys (BoolDogTag, StringDogTag, IntDogTag) with corresponding spec structs provide compile-time type safety and prevent misuse. The JSON tags are correctly structured with
omitemptyon descriptions and mandatory defaults.
38-54: Well-structured registry with clear defaults.The constants and registry maps are cleanly organized with descriptive names and appropriate default values. The defaults align with the past review feedback (PZ_LABEL_LIMIT now set to 0).
56-58: Empty registry is clearly marked.The empty
AllStringDogTagswith the comment// none yetappropriately signals that string-typed flags are supported but not yet defined.
kpom-specter
force-pushed
the
kpom/dogtags-m3/BED-6650
branch
from
bdb3323 to
88f9df0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cmd/api/src/services/dogtags/provider.go:
- Around line 32-34: Update the top comment for NoopProvider to accurately
reflect the error it returns: replace the mention of "ErrNotFound" with
"ErrProviderNotImplemented" so the docstring matches the actual behavior of
NoopProvider (see struct NoopProvider and its methods that return
ErrProviderNotImplemented).
🧹 Nitpick comments (2)
cmd/api/src/services/dogtags/service_test.go (1)
37-40: Simplify boolean comparison.The comparison
if multiTier != falsecan be simplified toif multiTierfor better readability.♻️ Proposed simplification
multiTier := svc.GetFlagAsBool(PZ_MULTI_TIER_ANALYSIS) - if multiTier != false { + if multiTier { t.Errorf("expected false, got %v", multiTier) }cmd/api/src/services/entrypoint.go (1)
84-88: Prefer structured logging over formatted strings.The logging uses
fmt.Sprintfto format key-value pairs. Based on learnings, BloodHound code should use explicit slog type functions for better structure and type safety.♻️ Proposed refactor to structured logging
- flags := dogtagsService.GetAllDogTags() - slog.InfoContext(ctx, "DogTags Configuration:") - for key, value := range flags { - slog.InfoContext(ctx, fmt.Sprintf(" • %s = %v", key, value)) - } + flags := dogtagsService.GetAllDogTags() + slog.InfoContext(ctx, "DogTags Configuration") + for key, value := range flags { + slog.InfoContext(ctx, "DogTag", slog.String("key", key), slog.Any("value", value)) + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/api/src/api/registration/registration.gocmd/api/src/api/registration/v2.gocmd/api/src/api/v2/dogtags.gocmd/api/src/api/v2/model.gocmd/api/src/services/dogtags/provider.gocmd/api/src/services/dogtags/service.gocmd/api/src/services/dogtags/service_test.gocmd/api/src/services/dogtags/sku_flags.gocmd/api/src/services/entrypoint.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/api/src/api/registration/registration.go
- cmd/api/src/api/registration/v2.go
- cmd/api/src/services/dogtags/sku_flags.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Applied to files:
cmd/api/src/api/v2/dogtags.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/services/entrypoint.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/services/dogtags/service.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/services/dogtags/service_test.go
🧬 Code graph analysis (4)
cmd/api/src/api/v2/dogtags.go (2)
cmd/api/src/api/v2/model.go (1)
Resources(106-120)cmd/api/src/api/marshalling.go (1)
WriteBasicResponse(90-99)
cmd/api/src/services/entrypoint.go (2)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(35-37)cmd/api/src/api/registration/registration.go (1)
RegisterFossRoutes(55-87)
cmd/api/src/services/dogtags/service.go (2)
cmd/api/src/services/dogtags/sku_flags.go (6)
BoolDogTag(19-19)StringDogTag(20-20)IntDogTag(21-21)AllBoolDogTags(46-49)AllStringDogTags(56-58)AllIntDogTags(51-54)cmd/api/src/services/dogtags/provider.go (2)
Provider(26-30)NewNoopProvider(36-38)
cmd/api/src/services/dogtags/service_test.go (2)
cmd/api/src/services/dogtags/service.go (1)
NewDefaultService(35-37)cmd/api/src/services/dogtags/sku_flags.go (3)
PZ_TIER_LIMIT(40-40)PZ_LABEL_LIMIT(41-41)PZ_MULTI_TIER_ANALYSIS(39-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (5)
cmd/api/src/api/v2/model.go (1)
119-119: LGTM!The DogTags service integration into Resources follows the established pattern for dependency injection and is consistent with other service fields.
Also applies to: 132-132, 147-147
cmd/api/src/services/dogtags/service.go (1)
39-44: Direct map access can panic if key is unregistered.The getter methods at lines 43, 50, and 57 directly index the
AllBoolDogTags,AllStringDogTags, andAllIntDogTagsmaps without checking if the key exists. If a typed constant is passed that hasn't been registered in the corresponding map, the service will panic.This issue has been previously identified and should be addressed with defensive map lookups as suggested in earlier review comments.
Also applies to: 46-51, 53-58
⛔ Skipped due to learnings
Learnt from: JonasBK Repo: SpecterOps/BloodHound PR: 1671 File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687 Timestamp: 2025-07-10T14:33:20.317Z Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.cmd/api/src/api/v2/dogtags.go (1)
25-28: Authentication is already properly configured via.RequireAuth()middleware.The endpoint is registered with
.RequireAuth()at the route level (cmd/api/src/api/registration/v2.go, line 141), which enforces authentication for all requests. No changes needed.Likely an incorrect or invalid review comment.
cmd/api/src/services/dogtags/provider.go (2)
23-30: Clean interface design with good separation of concerns.The Provider interface is well-designed with typed getters and clear documentation about the division of responsibility between providers (key-value lookup) and the service layer (defaults).
36-50: Well-implemented no-op provider.The constructor and methods correctly implement the Provider interface with appropriate zero values and consistent error handling. The pointer receiver is a good choice for consistency.
Adds the plumbing and structure for our SKU level flags
DogTags
Add a sku level flagging system with providers to be implemented in enterprise only
Motivation and Context
Resolves
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Behavior / Chore
Tests
✏️ Tip: You can customize this high-level summary in your review settings.