Go 1.27 modernization: crash fixes, dependency refresh, and architecture docs - #637
Open
christiangda wants to merge 18 commits into
Open
christiangda wants to merge 18 commits into
christiangda wants to merge 18 commits into
Conversation
Bump the go directive from 1.26.5 to 1.27.0 and apply the Go 1.27
modernizers via `go fix ./...`:
* forvar: drop the redundant `user := user` re-declaration in
internal/scim.GetGroupsMembers. Per-iteration loop variables have been
the semantics since Go 1.22, so the line was dead.
* embedlit: adopt Go 1.27's struct-literal field selectors, collapsing
`ListResponse: aws.ListResponse{...}` into the promoted fields in two
test composite literals. Semantically identical.
Also ignore the local-only .plan/ tracking folder.
No workflow changes are needed: every GitHub Actions job already resolves
the toolchain via `go-version-file: ./go.mod`, and the README Go badge is
derived from go.mod too.
Verified against the Go 1.27 behavioral changes; none affect this project:
* encoding/json is now backed by v2, but the v1 API still sorts map keys
(verified empirically). The only marshalled map is the SCIM patch value
in internal/scim, and the state file contains no map fields.
* Timer channels are now always unbuffered; the tree has no time.After,
time.NewTimer, time.NewTicker or time.Tick.
* http.Response.Body now drains on Close for HTTP/1, which only improves
connection reuse in pkg/aws; nothing aborts a body early.
* //go:linkname validation is stricter, but there is no linkname and no
unsafe import anywhere.
Deliberately not applying the omitzero modernizer: omitempty and omitzero
differ for false, 0 and empty-non-nil slices, and internal/model uses
omitempty on ~40 fields that are serialised into the S3 state file.
gofmt, go vet, go build and the full -race test suite are clean, and
golangci-lint reports the same 18 pre-existing issues as before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Runs `make go-mod-update`. All 8 direct dependencies that had updates available moved forward; no major versions and no module-path changes. Direct: * github.com/aws/aws-lambda-go v1.54.0 -> v1.55.0 * github.com/aws/aws-sdk-go-v2 v1.43.4 -> v1.45.1 * github.com/aws/aws-sdk-go-v2/config v1.32.35 -> v1.33.1 * github.com/aws/aws-sdk-go-v2/credentials v1.19.34 -> v1.20.1 * github.com/aws/aws-sdk-go-v2/service/s3 v1.107.0 -> v1.109.1 * github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.44.4 -> v1.46.1 * github.com/stretchr/testify v1.11.1 -> v1.12.1 * google.golang.org/api v0.292.0 -> v0.294.0 The remaining 8 direct dependencies were already current. Notable indirect movement: cloud.google.com/go/auth v0.22.0 -> v0.23.2, smithy-go v1.27.7 -> v1.28.1, and the golang.org/x/* set. testify v1.12 dropped its go-spew and go-difflib requirements, so both leave go.mod. Verification: * go mod verify: all modules verified * make go-generate: mocks regenerated, byte-identical (no consumed interface changed shape) * go build, go vet, gofmt: clean * go test -race: all 10 packages with tests pass * golangci-lint: same 18 pre-existing issues, no new findings * govulncheck: 0 vulnerabilities called; 1 in a required-but-unreached module * FUZZ_TIME=20s make fuzz: both state-file parsing targets pass (4.7M and 3.2M execs, no crashers) * pkg/aws credential-resolution tests pass, including the AWS_PROFILE and shared-config-file paths Still worth a manual check before release: the interactive stscreds.StdinTokenProvider MFA path in pkg/aws.NewDefaultConf, which CI cannot exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Each of these panicked on data the upstream APIs legitimately return.
Because the S3 state file is only written after a successful sync, a panic
means the next scheduled run re-reads the same record and panics again, so
a single malformed user could block all synchronisation indefinitely.
Every fix landed test-first; the failing output for each is in .plan.
1. internal/scim.GetGroupsMembers indexed user.Emails[0] unguarded.
buildUser only keeps addresses flagged primary, so an AWS SCIM user with
"emails": [] — or with addresses but none primary — produced a nil slice
and "index out of range [0] with length 0".
pkg/aws/testdata/ListUserResponse_no_emails.json is a checked-in fixture
of exactly that shape.
Replaced with a memberEmail helper that prefers the primary address and
falls back to the first one, so no existing expectation changes: pure
GetPrimaryEmailAddress() would have silently dropped emails for values
where Primary is not set.
2. idp.buildUser and scim.buildUser return nil to reject records the AWS
SCIM API would refuse (missing name, given name, family name, primary
email, or SCIM id), logging the reason as they do. Three callers stored
that nil instead of honouring it:
* internal/idp.GetUsers
* internal/idp.GetUsersByGroupsMembers
* internal/scim.GetUsers
A nil element then reached *Result.SetHashCode, whose sort comparator
dereferences it. Verified: "invalid memory address or nil pointer
dereference" at internal/model/user.go:581. A Google Workspace account
without a family name was enough to trigger it.
All three now skip and warn, matching what buildUser already does and
what GetGroups already does for duplicate group names, so one bad
directory record cannot block every other user.
3. Defence in depth in internal/model: SetHashCode now drops nil elements
from the hash input via compactNilPointers. A nil-safe comparator alone
would not have been enough — gob panics outright on a nil pointer
("gob: cannot encode nil pointer of type ..."), so the panic would simply
have moved from sort.Slice into MarshalBinary. Slices with no nil
elements are returned unchanged, keeping hashes byte-identical to
previous releases.
4. pkg/aws.CreateOrGetUser and CreateOrGetGroup called themselves after
clearing ExternalID, with no attempt limit. An endpoint that keeps
answering 409 while the follow-up lookup keeps returning nothing would
recurse until the stack was exhausted; the red test measured 51 POSTs
before its own tracker cut it off. Both now retry exactly once — which is
what their comments always claimed — and return ErrConflictUnresolved
afterwards.
gofmt, go vet, go build clean; full -race suite passes; golangci-lint
reports the same 18 pre-existing issues and no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Three concurrent fan-outs used the same hand-rolled shape: sync.WaitGroup, a channel semaphore, and a buffered error channel drained once at the end. That shape has two defects. There is no cancellation, so when one request fails every sibling still runs to completion, spending upstream API quota on results the caller will discard. And only the first error off the channel is ever read; the rest are silently dropped. errgroup was already a direct dependency and already used correctly in internal/scim.GetGroupsMembers, so this brings the remaining call sites onto the idiom the codebase had already settled on. * pkg/google.ListGroupMembersBatch — errgroup.WithContext with SetLimit(listGroupMembersBatchConcurrency). The former inline `const maxConcurrent = 10` is now a documented package constant. Red test measured 60/60 groups still requested after the first failure. * internal/idp.GetUsersByGroupsMembers — same treatment, with getUsersConcurrency as the documented cap. Red test confirmed no in-flight GetUser call ever observed a cancelled context. * internal/setup.Secrets — now takes a context.Context instead of calling context.Background() twice internally, so the four Secrets Manager reads are cancellable. Previously a Lambda approaching its deadline could not abandon them; they ran until the runtime froze the environment. The concurrent read moved into an unexported fetchSecrets that accepts a secretsFetcher interface. internal/setup was the only package in the tree with no tests at all, despite being the package that wires everything together; it now has four, covering field population, error propagation, an already-cancelled context, and sibling cancellation. cmd/idpscim passes rootCmd.Context() (cobra has set it by the time OnInitialize runs), with a defensive Background fallback so no nil context can reach errgroup. Also makes GetUsersByGroupsMembers deterministic. Its result was assembled by ranging a map, so the order changed run to run. UsersResult.HashCode is order-insensitive — SetHashCode sorts a copy — so this was never a correctness bug, but the state file written to S3 preserves slice order, so every single sync produced a large spurious diff against the previous object. The result is now sorted by UserName (the user's primary email, with IPID as a tiebreak), which makes the S3 artifact reviewable. The first sync after deploying this will therefore write one reordering diff. Subsequent syncs are stable. Two existing idp test cases matched GetUser's context argument by exact value (gomock.Eq on context.Background()). The fan-out now runs under an errgroup-derived child context, so those matchers were over-specified; relaxed to gomock.Any(), which is what the rest of the file already used. gofmt, go vet, go build clean; full -race suite passes; golangci-lint reports the same 18 pre-existing issues and no new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Implements D-6 (M6) and D-18 (M7), the two approved changes that touch hash computation and state-file decoding. Both were gated on proving the hashes do not move, so a golden fixture lands first. Golden proof (internal/model/golden_hash_test.go) Builds a fixed, fully-populated State and pins all ten hash codes it produces — the State, the three containers, and each individual resource. Coverage is deliberate: every optional pointer field is populated in one user and left nil in another (the nil cases are what exercise the EOF branches), two groups share an identical hash code so the ordering is exercised where the sort key is ambiguous, and resources are supplied out of order so the sort actually does work. It also round-trips the state through gob and through JSON, asserting the hash survives both and that JSON serialisation is stable. The ten golden values were captured before these changes and are unchanged after them. D-6 — slices.SortStableFunc Replaces all six sort.Slice calls in internal/model. SortStableFunc, not SortFunc: three of these sort by HashCode and one by IPID, none of which is guaranteed unique across resources. sort.Slice is unstable, so the order of tied elements was unspecified — and since the container hash is computed over the sorted slice, the hash of a set containing ties was itself unspecified. This makes it deterministic. Also converts the sort in GetPrimaryEmailAddress. D-18 — errors.Is(err, io.EOF) Replaces eight err.Error() != "EOF" string comparisons across user.go, member.go and state.go. gob signals "no further value was encoded" with io.EOF, which is the expected outcome for a value whose optional pointer field was nil at encode time. The string comparison missed wrapped errors and io.ErrUnexpectedEOF, so a genuinely truncated state file could be reported as a decode failure while a wrapped EOF was mistaken for one. Verification: all ten golden hashes unchanged; full -race suite green; FUZZ_TIME=45s make fuzz clean over both state-file parsing targets (3.4M execs on FuzzDiskRepositoryGetState, no crashers). New finding recorded while writing the golden fixture — M17 State.SetHashCode is order-dependent, unlike the three container SetHashCode methods. It rebuilds each *Result via a builder (which sorts only its own private copy, for its own hash) and then gob-encodes copyState, whose MarshalBinary walks Resources in slice order. Two runs over identical data can therefore produce different State.HashCode values. This is cosmetic today: State.HashCode is written into the S3 object but never read back — internal/core/actions.go compares only the three container hashes. It does make the state file's top-level hashCode field useless as an external change indicator. Deliberately not fixed here, since it was not part of the approved scope and fixing it would change that field's value for every deployed state file. TestState_hashCodeIsOrderDependent documents the behaviour and will skip with an explanatory message if a future change fixes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Conformance pass against google.github.io/styleguide/go. No behaviour
changes except M16, which is called out below. Full -race suite green
throughout, and the internal/model golden hashes are untouched.
M1 — no naked returns in non-trivial functions
internal/core/reconciling.go: all three reconciling* functions returned
bare `return` from ~50-line bodies with four early exits each, and named
their error result `e`. Now they declare locals and return explicitly.
internal/model/operations.go: the same, plus the guard clauses no longer
assign to every named result before returning
(`create, update, equal, remove, err = nil, nil, nil, nil, Err…`), and the
three Merge* helpers plus UpdateGroupsMembersSCIMID drop their unused
named results.
M2 — errors.New for constant sentinel messages
Twenty-two sentinels declared with fmt.Errorf and no format verbs, across
internal/core/reconciling.go, internal/config/config.go, pkg/google and
internal/scim. The rest of the codebase already used errors.New, so this
was inconsistency rather than preference. Two inline
fmt.Errorf("scim: … is nil") values in internal/scim became the exported
ErrGroupsResultNil and ErrUsersResultNil.
M3 — WithMaxMembersPerRequest(max int) shadowed the max builtin, in a file
that uses the min builtin forty lines further down. Renamed to n.
M4 — %w instead of %v when wrapping errors
pkg/google (NewService ×2, GetUser, GetGroup) and pkg/aws/secretsmanager
(×2) formatted errors with %v, severing the chain. This matters here
because internal/core relies on errors.AsType to classify
*types.NoSuchKey and *repository.ErrStateFileEmpty — the codebase already
depends on wrapping working.
M8 — for range n over a count, in the five UnmarshalBinary loops in
internal/model. go fix's rangeint correctly declined these because the
bodies mutate the receiver; the index was never used.
M9 — removed a dead `if len(chunks) > 0` guard in
internal/scim.patchGroupOperations. Every branch above it appends at
least one element.
M10 — one logger implementation
cmd/idpscimcli carried a 35-line copy of internal/setup.Logger's level and
format switches. It now calls setup.Logger, so the two binaries cannot
drift apart in log behaviour.
M12 — pkg/aws.NewDefaultConf was the only exported function in the tree
with no doc comment, and its named results were never referenced. Both
fixed; the doc comment records why stscreds.StdinTokenProvider is wired in
for the AWS_PROFILE path and not for Lambda.
M13 — HTTPResponseError had no doc comment and its field comments said
"Datahub error code", copy-pasted from an unrelated project. Documented
properly, including that it is part of the package's error contract:
callers recover it with errors.AsType and branch on StatusCode.
M14 — the SCIM PatchOp schema URN was inlined twice; now the patchOpSchema
constant, with an RFC 7644 reference.
M15 — pkg/aws declared UsersPath, GroupsPath and ServiceProviderConfigPath
but bypassed them at 15 of 18 call sites, using raw "/Users" literals and
fmt.Sprintf("/Users/%s", id). All now go through the constants and
path.Join, which also means an id containing a slash can no longer forge
extra path segments.
M16 — log levels fatal and panic now work (behaviour change)
config.Validate has always accepted "fatal" and "panic" — logrus levels
that predate the move to log/slog — but setup.Logger handled neither, so
they fell through its default branch, logged a misleading "unknown log
level" warning, and silently ran at info. They now map to
slog.LevelError, the closest slog equivalent, so a deployment configured
with either gets the severity it asked for.
Chose this over dropping them from the valid set, which would have turned
a working deployment's config into a startup failure.
The idpscim --log-level help text also advertised a "trace" level that was
never valid at all; corrected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
… tests
M11 — one field mapping for both user requests
buildCreateUserRequest and buildPutUserRequest were ~150 lines of
near-identical mapping, differing only in that Put sets ID. Adding a newly
synced SCIM attribute meant editing both, and forgetting one would make
creates and updates disagree with nothing to catch it.
aws.CreateUserRequest and aws.PutUserRequest are both declared as
`type X User`, so a single buildAWSUser serves both via conversion. Only the
put sets ID, because only a put targets an existing resource.
internal/scim/helpers.go drops from 307 to 256 lines.
Guarded by a characterisation test written before the change and unchanged
after it. A pure refactor has no bug to make fail first, so instead of
manufacturing a red the test pins the existing contract: both requests must
be field-for-field identical apart from ID, only the primary email is
forwarded, nil in gives nil out, and the marshalled wire payload still
carries every attribute AWS requires including the enterprise extension. It
passed before the refactor and passes after.
M5 — errcheck: 17 findings down to 3
The three that remain are `defer resp.Body.Close()` in pkg/aws/scim.go and
internal/repository/s3.go, where the Close error is not actionable — and
under Go 1.27 even less so, since Close now drains the body. Notably errcheck
flags only 3 of the 16 identical call sites in scim.go, which is its own
argument for the committed .golangci.yml that finding R2 asks for; they are
handled there rather than by scattering `_ =` across otherwise idiomatic Go.
The 14 test findings are fixed properly, and doing so uncovered two real
problems:
1. Two tests did `tmpDir := os.TempDir()` followed by
`defer os.Remove(tmpDir)` — an attempt to delete the shared system temp
directory. It failed silently only because /tmp is never empty. Both now
use t.TempDir(), which creates a per-test directory and removes it during
cleanup.
2. Switching pkg/aws/config_test.go from os.Setenv to t.Setenv exposed three
subtests that were passing for the wrong reason.
os.Setenv never cleaned up, so the AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY
and AWS_REGION set by "using access key from env vars" leaked into every
subtest after it. Three of those subtests therefore asserted
cred.Source == "EnvConfigCredentials" and AWS_REGION == "us-east-1" while
their own setup pointed exclusively at a shared credentials file — and
their names said so ("using credential file", "using credential and config
file and profile default").
The key and secret assertions never caught the discrepancy because
testdata/default/credentials holds exactly the same values the env-var
subtest uses; only cred.Source differed, and it was reading leaked state.
Each now asserts what its own setup actually produces:
SharedConfigCredentials with the corresponding path, and an empty region
where no config file is supplied. The AWS_PROFILE variant already expected
the correct source, which is what made the inconsistency findable.
Verified both ways: the whole file passes in order, and all five subtests
pass individually, so they no longer depend on execution order.
Also noted in a comment that "using profile from env vars" points
AWS_CONFIG_FILE at testdata/case1/config, which does not exist. Left as-is
— a missing config file is worth covering and is what the empty region
asserts — but the name looks like a leftover and is worth a look.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
R2 — add .golangci.yml
The contributor checklist tells everyone to run `golangci-lint run ./...`,
but with no committed config each contributor and CI runner used whatever
defaults their own version shipped, so results were not reproducible.
The config starts from the standard set and adds bodyclose, copyloopvar,
errorlint, misspell, nilerr, revive, unconvert, usestdlibvars, wastedassign
and whitespace, with goimports grouping local imports under the module path.
Two deliberate tunings, both documented in the file:
* govet's shadow check is disabled. It is off in govet's own default set for
good reason — idiomatic `if err := f(); err != nil` shadows any outer err
and trips it every time. All five hits in this tree were exactly that,
in tests.
* errcheck excludes (io.Closer).Close and (io.ReadCloser).Close. The Close
error on an HTTP response body is not actionable, and under Go 1.27 Close
also drains the body, so not checking it cannot leak a connection.
Excluded rather than papered over with `_ =` at 17 sites. bodyclose stays
enabled and still enforces that bodies ARE closed, which is the part that
matters. (Notably errcheck flagged only 3 of the 16 identical call sites
in pkg/aws/scim.go — its inconsistency here is itself an argument for
pinning the config.)
Generated mocks and test files carry narrow, commented exclusions.
R6 — add a Lint job to build.yml
Runs golangci-lint v2.13.1 against the committed config. The action is
SHA-pinned to v9.3.0 (ba0d7d2), consistent with the repo's existing pinning
convention for OpenSSF Scorecard.
R3 — codecov has been receiving nothing
build.yml uploaded `./coverage.out`, but the Makefile writes coverage to
`$(BUILD_DIR)/coverage.txt`. That path never existed in CI, so every run
uploaded nothing and the README badge has been reporting stale data. Now
points at ./build/coverage.txt, verified by running `make test` from a clean
tree and confirming the file appears there.
R4 — documented that -tags=unit is a no-op
No file in the tree carries a //go:build tag, so PROJECT_COVERAGE_TAGS
selects nothing. Left in place as the hook for a future unit/integration
split, but the Makefile now says so, rather than implying a split that does
not exist.
Result: golangci-lint reports 0 issues, down from 18 at baseline against a
stricter configuration.
Getting there also closed 21 revive findings — exported symbols with no doc
comment, which the Google style guide requires. Documented model.PhoneNumber,
model.Manager, model.EnterpriseData, the six ErrorMessage/ErrorCode methods on
the repository error types, the four Validate methods and three types in
pkg/aws/scim_model.go, pkg/google.DirectoryServiceConfig, and split
pkg/aws's combined path/content-type const block so each constant carries its
own comment. Also fixed an empty `// ErrEmailsEmpty` stub comment.
The one remaining staticcheck finding, SA1019 on the deprecated
oauth2/google.CredentialsFromJSONWithParams, carries a documented nolint
alongside a TODO explaining the migration target and why it is deliberately
deferred to its own change: it is the authentication path of a production
Lambda and needs separate tests plus a manual run against a real Google
Workspace tenant. Tracked as finding D1.
Verified the internal/model golden hashes are still unchanged after the whole
style and lint pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
…ariants
.github/copilot-instructions.md is the single source of truth for every
assistant — CLAUDE.md, AGENTS.md, .cursorrules, .clinerules and
DEVELOPMENT_GUIDELINES.md are all symlinks to it. All five verified to still
resolve.
Two factual errors removed:
* "Database: PostgreSQL" — there is no database anywhere in this project.
State is a single JSON object in S3 (internal/repository/s3.go), with a
disk-backed alternative for local runs. This actively misled agents into
looking for, or proposing, a data layer.
* "Go 1.26+" — now 1.27+.
The document was otherwise a generic Go style checklist that said nothing about
the things an agent will actually get wrong here. Added:
* What the project is, and that it mutates real production identity data, so
correctness outranks elegance.
* The real stack, replacing the unhelpful "Framework: Go standard library".
* **Architecture invariants** — the most valuable addition. Never construct a
model.* value directly (Build() is what sets Items and HashCode); the
hand-written MarshalBinary methods define the gob hash input, so changing
field order invalidates every deployed state file; SCIMID is excluded from
hashes on purpose; sync compares container hashes, not the State hash, and
those must stay order-independent; schema changes require bumping
StateSchemaVersion; interfaces are declared by the consumer; internal/core
must not import concrete AWS or Google types; a nil from buildUser means
"skip and warn", never "store the nil".
* Repo-specific style rules distilled from this pass: %w not %v, no naked
returns, errgroup over hand-rolled fan-outs, context flows from cmd/, never
log secrets, SortStableFunc when the key feeds a hash.
* Testing requirements, including red-test-first for fixes and refactors —
with the explicit carve-out that a pure refactor gets a characterization
test rather than a manufactured red — plus t.Setenv/t.TempDir over
os.Setenv/os.TempDir, mock regeneration, fuzzing, and keeping
golden_hash_test.go green.
* The post-change gate, now including golangci-lint reporting 0 issues.
* Planning and tracking: .plan/ with a live PROGRESS.md, and behaviour-
changing decisions recorded and approved before implementation.
* Documentation requirements, including mermaid diagrams over binary exports
so they are reviewable in a diff.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
…iagrams The docs/ folder was entirely operational — how to deploy and configure — with nothing explaining how the system is built. The only diagrams were a .drawio.png and a .drawio.html, neither reviewable in a diff. Three new guides, 10 mermaid diagrams, all verified against the code rather than written from the README's description. docs/Architecture.md System context; package topology (with the interface-ownership table showing that internal/core declares its own dependencies and imports no AWS or Google types); the first-run-vs-state-run branch and why both exist; the create/update/equal/remove reconciliation model and the matching keys that make it safe across two systems assigning their own identifiers; the hashing scheme, what it excludes and why; the state-file lifecycle; how SCIM group membership has to be inferred by inverting the query, since AWS never populates the members array; every concurrency limit; and a failure-mode table. docs/Implementation-Guide.md Repository layout, the five invariants that are easy to break by accident, a 9-step flow for adding a synced user attribute (the change most likely to be done half-right), the mock workflow, testing conventions, state-file compatibility rules split by whether a change is safe / forces a re-sync / needs a schema bump, and the local gate. docs/User-Manual.md End-to-end operator guide: prerequisites, Google Workspace service-account and domain-wide delegation setup, IAM Identity Center SCIM enablement, all four deployment options, read-only verification with idpscimcli before letting the sync write anything, what to watch in CloudWatch, a troubleshooting table, and upgrade/rollback including that deleting the state file is the standard recovery. Updated: * Every package comment expanded. internal/core/docs.go described utilities for deep copying data structures — which is internal/deepcopy, not core. * docs/State-File-example.md now explains what participates in a hash and why SCIMID does not, that the MarshalBinary methods are effectively a wire format, and that deleting the file is safe because the next run adopts the existing SCIM population rather than recreating it. * docs/Development.md: Go 1.27 prerequisite, the full local gate including golangci-lint at a 0-issue baseline, the additional per-area checks, the dependency-update flow, a warning that internal/model changes must keep golden_hash_test.go green, and a macOS note about Go 1.27 honouring SSL_CERT_FILE / SSL_CERT_DIR. * docs/Configuration.md: documents the accepted log levels, including that fatal and panic are now aliases for error rather than silently degrading to info. * docs/Whats-New.md: full entry for this work with motivation, impact and the target version, flagging the one-off state-file reordering diff. * README.md: Quick Start, an inline mermaid architecture diagram, an Architecture-at-a-Glance section, a Contributing section, consistent emoji headings, and a documentation table covering all 14 docs instead of 5. Verified: 173 relative markdown links across README and docs all resolve; all code fences balanced; mermaid blocks checked for the two constructs that break the parser (escaped quotes in labels, raw angle brackets in sequence-diagram text) and cleaned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
…ding cmd/ Follow-up round from finishing the idpscim/idpscimcli documentation audit I had flagged as shallow. It turned up two real bugs and a correction to something this branch had already committed. 1. `idpscimcli aws users list --filter` never worked The flag was registered on awsUsersCmd — the `aws users` grouping command, which has no RunE. Cobra's Flags() are local to the command they are set on, so the flag was advertised by `idpscimcli aws users --help`, where nothing consumes it, and rejected by `idpscimcli aws users list --filter …` with "unknown flag: --filter". `aws groups list` registered it correctly on the leaf command all along. Moved to awsUsersListCmd, and corrected the example in the help text from a groups-shaped filter (`displayName eq …`) to a users-shaped one (`userName eq …`). Red test first, in cmd/idpscimcli/cmd/flags_test.go: both leaf list commands must accept `--filter` with shorthand `-q`, and neither grouping command may advertise it while it has no run function. 2. .gitignore was hiding the cmd/ source trees The entries `idpscim` and `idpscimcli` were meant for stray binaries built into the repository root. But a gitignore pattern containing no slash matches at *any* depth, so both also matched the cmd/idpscim/ and cmd/idpscimcli/ **source directories**. Already-tracked files are never affected by .gitignore, which is why the repository works and nobody noticed. The damage was to anything new: a file added under either directory was invisible to `git status` and would be silently omitted from a commit. Every ignore-aware tool — ripgrep, editors, Docker build contexts, some scanners — skipped both trees entirely. Both patterns are now anchored as `/idpscim` and `/idpscimcli`. Verified: a probe file under cmd/idpscim/ is now visible to `git status`, and a binary at the repository root is still ignored. 3. Correction: -tags=unit is NOT a no-op Commit 40b36a2 in this branch added a Makefile comment claiming no file in the tree carries a //go:build tag and that `-tags=unit` therefore selects nothing. Both claims were wrong. cmd/idpscimcli/cmd/common_test.go is tagged `unit`, so a bare `go test ./...` reports "no test files" for that package while `make test` runs it. The root cause of the mistake is item 2: the shell's grep is an ignore-file-aware wrapper, so the survey that produced the claim could not see into cmd/ at all. Re-ran that audit with the real grep once the ignore bug was understood; nothing else was missed. The Makefile comment now describes the actual behaviour and points at the go.buildTags setting that keeps editors consistent with it. 4. Documentation audit, properly this time The earlier pass had only compared flag *names* between the docs and the code. Now compared against real `--help` output: * Defaults column added to every flag table in idpscim.md and idpscimcli.md, matching the binary exactly. * `--log-level` values documented, including that fatal and panic are aliases for error, cross-linked to Configuration.md. * Valid `--sync-user-fields` values listed, with a note that narrowing the set also narrows what is fetched from Google and causes a one-off update of every user. * AWS filter flags documented for both list commands, with a `users list --filter` example that now actually runs. * `--output-format`, `--timeout` and the flags with no shorthand marked as such. * Example paths no longer put the service-account JSON in the working directory; added a caution that the default `credentials.json` resolves there and that production resolves the JSON from Secrets Manager instead. Also includes the requested .vscode/settings.json change: go.buildTags set to "integration,unit" so gopls and the editor's test discovery see the tagged files that `make test` runs. Verified: go fix reports no changes; gofmt, go vet clean; golangci-lint 0 issues; full -race suite green with -tags=unit; internal/model golden hashes unchanged; 174 relative markdown links resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
ddcb5bc bumped google.golang.org/api from v0.294.0 to v0.295.0 after the Whats-New entry was written. The entry now matches go.mod. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Findings M17 and M23. One root cause, one fix, but they differ sharply in consequence: M23 changes a hash the sync reads, M17 does not. The problem Every hash here is a gob encoding, and the hand-written MarshalBinary methods walk their Resources slices in slice order. Arrival order is therefore part of the hash unless normalised first — and it must be normalised, because the identity-provider fan-outs and the SCIM membership inversion all build their slices from maps. The normalisation was inlined per SetHashCode method, and two had drifted. M23 — GroupsMembersResult (this hash IS read) It sorted its outer slice but not the members nested inside each entry. GroupMembers.MarshalBinary walks its own members in slice order, so member order *within* a group reached the bytes of the enclosing hash, while GroupMembers' own hash stayed stable because that method does sort its copy. internal/core/actions.go compares GroupsMembersResult hashes to decide whether membership needs reconciling, so a reordering upstream produced a spurious full membership sync — real SCIM PATCH traffic for no change. Two sources reorder: the Google Directory API guarantees no member ordering, and internal/scim.GetGroupsMembers appends members in goroutine completion order, which is non-deterministic by construction. Demonstrated before the fix: identical data, reversed members within one group, GroupsMembersResult hash ba6fa20d… vs 0ac789a7…. M17 — State (write-only) State.SetHashCode sorted nothing. It rebuilt each *Result through a builder and gob-encoded the result; Build() sorts only its own private copy to compute that container's HashCode field, so the sort never reached the State hash bytes. Nothing reads State.HashCode — verified there is no read anywhere — so this had no runtime effect. It did make the state file's top-level hashCode field useless as an external change indicator, which is a trap worth closing. The fix The five orderings now live in internal/model/sort.go and are called by every SetHashCode, so they cannot drift apart again. GroupsMembersResult additionally normalises the members nested in each entry, and State normalises all three of its rebuilt slices. Computing a hash must not reorder the caller's live data. deepcopy.SliceOfPointers copies each *GroupMembers struct, but a struct copy shares the backing array of its Resources slice — so sorting those members in place would still mutate the caller. Each entry now gets its own copied member slice. TestSetHashCode_doesNotMutateCallerOrdering pins this. Impact * groupsMembers golden hash changed: 6c91be62… -> f391383e… The first run after deploying performs one membership reconciliation pass. * state golden hash changed: 841819f1… -> e7a1cd95… No runtime effect; the field is never read. * groups, users, and every individual resource hash are UNCHANGED, so groups and users do not re-sync. MembersResult deliberately still orders by IPID rather than email; switching it would change every MembersResult hash for no benefit. Tests Red first. TestGoldenHashes_containerHashesAreOrderIndependent was strengthened to reverse the members inside each group as well as the outer slices, which is what exposed M23 from the existing golden fixture; it failed with "GroupsMembersResult hash depends on input order". TestState_hashCodeIsOrderDependent, which previously documented M17 as accepted behaviour, is replaced by TestGoldenHashes_stateHashIsOrderIndependent asserting the opposite. Added TestGroupsMembersResult_hashIgnoresWithinGroupMemberOrder covering all three hashes at once. Verified: go fix no changes; gofmt, go vet clean; golangci-lint 0 issues; full -race suite green; FUZZ_TIME=30s make fuzz clean on both state-file targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
Finding A3, enabled by Go 1.27.
Go 1.27 prints runtime/pprof goroutine labels in traceback headers, so a panic
in CloudWatch reads:
goroutine 42 [running] {sync: "users", user: "zoe@example.com"}:
instead of leaving an operator to bisect the directory. The C1/C2 crashes fixed
earlier in this branch were exactly that shape — an index-out-of-range inside a
bounded fan-out worker, with nothing in the trace identifying the record.
Labelled:
* core.SyncGroupsAndTheirMembers sync=root, codeVersion
* idp.GetUsersByGroupsMembers sync=users, user=<email>
* google.ListGroupMembersBatch sync=group-members, group=<id>
* scim.GetGroupsMembers sync=scim-group-members, user=<scimid>
Uses pprof.SetGoroutineLabels, NOT pprof.Do
This is the part worth knowing. pprof.Do is the idiomatic scoped API and the
obvious choice, but it does not work for this purpose: it defers restoring the
previous label set, and on a panic those defers run during unwinding *before* the
runtime prints the traceback. The labels are therefore gone at exactly the moment
they are needed.
Verified both ways against Go 1.27 before choosing:
pprof.Do -> goroutine 1 [running]:
SetGoroutineLabels -> goroutine 1 [running] {user: "zoe@example.com"}:
runtime.Stack shows the labels under either API, so the pprof.Do version passes
a naive check and fails only in the case it exists to serve.
TestGetUsersByGroupsMembers_labelsGoroutinesForTracebacks captures a real
traceback from inside a worker and asserts the labels are in the goroutine
header, so the substitution cannot be made silently.
Cost
Measured: 28.65 ns/op, 104 B/op, 3 allocs/op — per goroutine, not per iteration.
For a 500-user sync that is roughly 15 µs and 52 KB in total, noise against the
network round-trips it accompanies. Opt out with GODEBUG=tracebacklabels=0.
Purely additive: no control flow, no exported API, no hashing, no state-file
change. The internal/model golden hashes are unchanged, as they must be.
Documented in docs/Architecture.md (Failure modes) and docs/Whats-New.md,
including the pprof.Do caveat so the reasoning is not lost.
Verified: go fix no changes; gofmt, go vet clean; golangci-lint 0 issues; full
-race suite green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai>
The path fix in the previous commit was only half the problem, which a real CI run on PR #637 revealed. The path fix did work. Codecov went from finding nothing to: info -- Found 1 coverage files to report info -- > /home/runner/.../build/coverage.txt But the upload is then rejected: error -- Upload queued for processing failed: {"message":"Token required because branch is protected"} `main` is a protected branch, and codecov requires a token for protected branches. The CODECOV_TOKEN repository secret does not exist — the repo has only AWS_OIDC_ROLE_TO_ASSUME, DOCKER_HUB_TOKEN, DOCKER_HUB_USER, GH_PAT, GH_USER and SAM_APP_BUCKET. Because fail_ci_if_error defaults to false, both failures were silent. That is exactly how the wrong path survived unnoticed, so the step now carries a comment saying to check its log after the token is added. The workflow passes token: ${{ secrets.CODECOV_TOKEN }}, which completes the fix the moment the secret is created. fail_ci_if_error is deliberately left false so a missing token cannot break the build. Also corrects docs/Whats-New.md, which claimed codecov was "fixed and verified". That was true of the path and not of the upload, and overstated the result. ACTION REQUIRED: add the CODECOV_TOKEN secret (Settings -> Secrets and variables -> Actions) for the coverage badge to update. Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit intended to correct this but its text replacement silently failed to match, so the overstated wording stayed in the PR. docs/Whats-New.md claimed codecov was "fixed and verified from a clean tree". That was true of the file path and not of the upload: `main` is a protected branch, codecov requires a token for protected branches, and the CODECOV_TOKEN secret does not exist. The entry now states both causes and that adding the secret is what completes the fix. Signed-off-by: Christian González Di Antonio <christian.gonzalez@aizon.ai> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Go 1.27 modernization: crash fixes, dependency refresh, and architecture docs
Target release: minor,
v0.46.0.Branch is up to date with
main(225f2ef, no divergence). 15 commits, 77 files,+4,556 / −779. Coverage 71.7% → 73.5%, test files 35 → 46.
Two things change on the first run after deploying. Neither needs a config change, and both settle
after one run.
groupsMemberscontainer hash changed (finding M23 below)Groups and users do not re-sync. Their hashes are byte-identical to
main— verified by the newgolden test.
model.StateSchemaVersionis unchanged at1.0.0, and the state-file JSON schema isidentical, so this is not a migration.
No breaking changes. Verified against
mainin a clean worktree:pkg/awsexported API+ErrConflictUnresolved)pkg/googleexported APIStateSchemaVersionIDPSCIM_*env vars%v→%won two error wraps)🐛 Crash fixes — the reason this is worth merging
Three code paths panicked on data that Google Workspace and AWS IAM Identity Center legitimately
return. Because the state file is only written after a successful run, a panic meant the next
scheduled invocation re-read the same record and panicked again — so a single malformed directory
entry could block all synchronization indefinitely, with no self-recovery.
C1 — a SCIM user with no email crashed membership resolution.
internal/scim.GetGroupsMembersindexeduser.Emails[0]unguarded.buildUseronly keeps addressesflagged primary, so a user with
"emails": []produced a nil slice.pkg/aws/testdata/ListUserResponse_no_emails.jsonis a checked-in fixture of exactly that shape.C2 — a Google user without a family name crashed hashing.
buildUserreturnsnilto reject records AWS SCIM would refuse, logging why. Three callers storedthat
nil, and it reachedSetHashCode, whose comparator dereferences every element. Now skip-and-warn,matching what
GetGroupsalready did for duplicate group names.H4 — a persistent 409 could exhaust the stack.
CreateOrGetUser/CreateOrGetGroupclearedexternalIdand recursed with no limit. Now retry exactlyonce — which is what their comments always claimed.
Red-test evidence
Every fix landed test-first. Failing output before each:
A note on method:
gobpanics on a nil pointer element, so a nil-safe comparator alone would havemoved the panic from
sort.SliceintoMarshalBinary.SetHashCodetherefore drops nils from thehash input; slices with no nils are returned unchanged, keeping hashes byte-identical.
🧮 M23 — a hash the sync reads was order-sensitive
Found while writing the M17 test.
GroupsMembersResult.SetHashCodesorted its outer slice but not themembers nested inside each entry, and
GroupMembers.MarshalBinarywalks those in slice order.This hash is read, by
internal/core/actions.go, to decide whether membership needs reconciling.Two upstream sources reorder members:
internal/scim.GetGroupsMembersappends in goroutine completion order — non-deterministic byconstruction, every run.
So membership could be fully reconciled for no change. Fixed together with M17 (
State.HashCode,which sorted nothing but is write-only, so it had no runtime effect). All five orderings now live in
internal/model/sort.goand are called by everySetHashCode, so they cannot drift apart again.Care taken that hashing never reorders the caller's data: copying a
GroupMembersstruct shares itsResourcesbacking array, so each entry gets its own copied member slice. A test pins this.⬆️ Go 1.27
go.mod1.26.5→1.27.0. No workflow changes needed — every job already resolves the toolchainfrom
go.mod.go fix ./...removed a redundant loop-variable re-declaration and adopted the newstruct-literal field selectors.
Every Go 1.27 behavioural change was checked; none affects this project:
encoding/jsonbacked by v2gob.time.After/NewTimer/NewTicker/Tickanywhere.Response.Bodydrains onClose//go:linknamelinkname, nounsafe.SSL_CERT_FILEon macOSdocs/Development.md.Deliberately not applied: the
omitzeromodernizer.omitemptyandomitzerodiffer forfalse,0and empty-non-nil slices, andinternal/modelusesomitemptyon ~40 state-file fields.📦 Dependencies
All 8 direct dependencies with updates moved forward; no majors, no module-path changes.
aws-lambda-gov1.55.0,aws-sdk-go-v2v1.45.1 (+config,credentials,s3,secretsmanager),testifyv1.12.1,google.golang.org/apiv0.295.0.Verified:
go mod verify, mocks regenerated byte-identical,govulncheck0 called.⚡ Concurrency
Three fan-outs shared a hand-rolled
WaitGroup+ channel-semaphore + buffered-error-channel shapewith two defects: no cancellation (one failure left siblings burning upstream API quota for discarded
results) and every error after the first silently dropped.
errgroupwas already a direct dependencyand already used this way in
internal/scim.pkg/google.ListGroupMembersBatch,internal/idp.GetUsersByGroupsMembersandinternal/setup.Secretsnow useerrgroup.WithContextwith explicit limits.setup.Secretsalsotakes a
context.Contextinstead of callingcontext.Background()internally, so the four SecretsManager reads are cancellable on Lambda timeout.
🔎 Panics now name the record
Go 1.27 prints
runtime/pproflabels in traceback headers:Implementation note worth reviewing: this uses
pprof.SetGoroutineLabels, notpprof.Do.pprof.Dodefers restoring the previous label set, and on a panic those defers run during unwindingbefore the runtime prints the traceback — so labels vanish exactly when needed.
runtime.Stackshows them under either API, so the broken version verifies fine by accident. A test guards it.
Cost measured: 28.65 ns / 104 B per goroutine. Opt out with
GODEBUG=tracebacklabels=0.🔧 Style, CI and two more bugs
Google Go style guide conformance: 22 sentinels
fmt.Errorf→errors.New; naked returns removed;%v→%won 6 error wraps (internal/coredepends onerrors.AsTypeworking);err.Error() != "EOF"→
errors.Is(err, io.EOF)on 8 state-file decode sites;sort.Slice→slices.SortStableFunc; aparameter shadowing the
maxbuiltin renamed;pkg/awspath constants actually used at all 18 callsites via
path.Join.buildCreateUserRequest/buildPutUserRequestde-duplicated (~150 near-identicallines, differing only in
ID), guarded by a characterization test written before the change.Log levels
fatal/panicnow work.config.Validatealways accepted them —logrusleftovers —but the logger handled neither, so they fell through to
infowith a misleading "unknown log level"warning. Now map to
slog.LevelError. Chosen over dropping them, which would break a deployed config.CI:
build.ymluploaded./coverage.out; the Makefile writesbuild/coverage.txt. That path never existed in CI. Fixed and verified from a clean tree..golangci.ymland a Lint job. Baseline is now 0 issues, down from 18, against astricter config. Two documented tunings:
govet'sshadowdisabled (fires on idiomaticif err := f(); err != nil), anderrcheckexcludesCloseon response bodies whilebodyclosestill enforces that bodies are closed.
idpscimcli aws users list --filternever worked. Registered on theaws usersgrouping command(no
RunE), so advertised where nothing consumed it and rejected asunknown flagwhere it was needed..gitignorewas hiding thecmd/source trees. Bareidpscim/idpscimclipatterns match at anydepth, so
cmd/idpscim/andcmd/idpscimcli/were ignored. Tracked files were immune — but any newfile there was invisible to
git statusand would be silently dropped from a commit. Now anchored.This also caused a wrong claim in an earlier commit on this branch, corrected here.
Test isolation: two tests called
os.Remove(os.TempDir()), attempting to delete shared/tmp.Three subtests in
pkg/aws/config_test.gopassed only becauseos.Setenvleaked credentials betweenthem — they asserted credentials came from the environment while their own setup pointed at a
credentials file. All now use
t.Setenv/t.TempDir()and pass individually as well as in file order.📚 Documentation
Three new guides with 10 mermaid diagrams, authored in markdown so they are reviewable in a diff:
reconciliation, hashing, state-file lifecycle, how SCIM membership is inferred, concurrency, failure modes.
attribute, testing conventions, state-file compatibility rules.
Also: every package comment expanded (
internal/core's describedinternal/deepcopy);.github/copilot-instructions.mdrewritten (it claimed "Database: PostgreSQL" — there is nodatabase) and now carries the architecture invariants;
State-File-example.md,Development.md,Configuration.md,idpscim.md,idpscimcli.mdand the README updated. 174 relative links verified.✅ Verification
go fixno changes ·gofmtclean ·betteralignno changes ·go vetclean ·golangci-lint0 issues · 11/11 packages green under-race -tags=unit·Lambda
linux/arm64 -tags lambda.norpccross-compile OK ·govulncheck0 called ·FUZZ_TIME=60s make fuzzclean on both state-file targets ·sam validate --lintpasses ·golden hashes verified (only the two intended changed).
🔜 Deliberately out of scope
oauth2/google.CredentialsFromJSONWithParams(the onlystaticcheckfinding) —discarded by the maintainer: credentials come from Secrets Manager and altering that path would
be a breaking change. A
TODO+ documented//nolintkeeps the deprecation visible and lint at 0.httptest-based test files ontohttptest.NewTestServer+testing/synctest(~4,800 lines, no production benefit).
📋 Reviewer checklist
fatal/panic→errorlog-level mapping is the desired callstscreds.StdinTokenProviderMFA path — CI cannot drive stdinv0.46.0