-
Notifications
You must be signed in to change notification settings - Fork 0
CXH-2350: surface workspace-token sync limits and fix PAT setup docs #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
al-conductorone
wants to merge
12
commits into
main
Choose a base branch
from
cxh-2350-pat-mode-limits-visibility-and-docs
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
4ba9f92
warn on unreachable account plane under workspace-token auth; fix PAT…
al-conductorone 7fb00d7
address review: log account-plane notice at debug, not warn
al-conductorone 00886b1
split the notice level by cause: debug for PAT, warn for OAuth-probe …
al-conductorone 0d75f95
keep the account-plane notice at warn; lock it with a test
al-conductorone c94df9d
CXH-2350: bump sync-test action to @v4
al-conductorone 1ee47a2
map oauth2 token-retrieval failures to Unauthenticated
al-conductorone d4147cd
Merge remote-tracking branch 'origin/main' into cxh-2350-pat-mode-lim…
al-conductorone 049a30d
map oauth2 5xx to retryable status and attach account-probe cause
al-conductorone be08f38
map oauth2 429 to ResourceExhausted
al-conductorone f61cec3
map oauth2 429 to Unavailable, not ResourceExhausted
al-conductorone c98b5ca
fail validation under OAuth when the account API is unreachable
al-conductorone 100cfb0
fix review findings: dedupe manifest key, prune dead field-group entr…
al-conductorone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| package connector | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "io" | ||
| "net/http" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/conductorone/baton-databricks/pkg/databricks" | ||
| "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" | ||
| "go.uber.org/zap" | ||
| "go.uber.org/zap/zapcore" | ||
| ) | ||
|
|
||
| // rolesTransport answers the assignable-roles calls Validate makes. failAccount | ||
| // makes the account-plane check (host "accounts.*") fail so isAccAPIAvailable stays | ||
| // false while the workspace check still succeeds. | ||
| type rolesTransport struct{ failAccount bool } | ||
|
|
||
| func (t rolesTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| if t.failAccount && strings.HasPrefix(req.URL.Host, "accounts.") { | ||
| return &http.Response{ | ||
| StatusCode: http.StatusInternalServerError, | ||
| Header: http.Header{"Content-Type": []string{"application/json"}}, | ||
| Body: io.NopCloser(strings.NewReader(`{"message":"boom"}`)), | ||
| Request: req, | ||
| }, nil | ||
| } | ||
| return &http.Response{ | ||
| StatusCode: http.StatusOK, | ||
| Header: http.Header{"Content-Type": []string{"application/json"}}, | ||
| Body: io.NopCloser(strings.NewReader(`{"roles":[]}`)), | ||
| Request: req, | ||
| }, nil | ||
| } | ||
|
|
||
| func captureLogs(ctx context.Context, buf *bytes.Buffer) context.Context { | ||
| core := zapcore.NewCore( | ||
| zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), | ||
| zapcore.AddSync(buf), | ||
| zapcore.DebugLevel, | ||
| ) | ||
| return ctxzap.ToContext(ctx, zap.New(core)) | ||
| } | ||
|
|
||
| func newValidateConnector(t *testing.T, auth databricks.Auth, tr http.RoundTripper) *Databricks { | ||
| t.Helper() | ||
| client, err := databricks.NewClient( | ||
| context.Background(), &http.Client{Transport: tr}, | ||
| "cloud.databricks.com", "accounts.cloud.databricks.com", | ||
| "acct-1", "", auth, nil, | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| return &Databricks{client: client, workspaces: []string{"ws1"}} | ||
| } | ||
|
|
||
| // levelFor scans the captured JSON log lines for the first entry whose message | ||
| // contains want and returns its level. Empty string means no such entry. | ||
| func levelFor(t *testing.T, buf *bytes.Buffer, want string) string { | ||
| t.Helper() | ||
| for _, line := range strings.Split(buf.String(), "\n") { | ||
| if line == "" { | ||
| continue | ||
| } | ||
| var entry struct { | ||
| Level string `json:"level"` | ||
| Msg string `json:"msg"` | ||
| } | ||
| if err := json.Unmarshal([]byte(line), &entry); err != nil { | ||
| continue | ||
| } | ||
| if strings.Contains(entry.Msg, want) { | ||
| return entry.Level | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| const accountUnreachableMsg = "account API unreachable" | ||
|
|
||
| // CXH-2350: dropping the whole account plane is a customer-visible degradation, so the | ||
| // startup notice must be visible. It logs at warn, not debug (a debug line is invisible at | ||
| // the default info level, which is the silent degradation the ticket was filed to fix). | ||
| func TestValidateWorkspaceTokenLogsAtWarn(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| ctx := captureLogs(context.Background(), buf) | ||
| d := newValidateConnector(t, databricks.NewTokenAuth([]string{"ws1"}, []string{"tok"}), rolesTransport{}) | ||
|
|
||
| if _, err := d.Validate(ctx); err != nil { | ||
| t.Fatalf("Validate: %v", err) | ||
| } | ||
|
|
||
| if got := levelFor(t, buf, accountUnreachableMsg); got != "warn" { | ||
| t.Errorf("token-auth notice logged at %q, want %q", got, "warn") | ||
| } | ||
| } | ||
|
|
||
| // Under OAuth a failed account check is a fixable misconfiguration, so Validate | ||
| // fails rather than silently dropping account-level data. | ||
| func TestValidateOAuthAccountCheckFailureReturnsError(t *testing.T) { | ||
| d := newValidateConnector(t, &databricks.NoAuth{}, rolesTransport{failAccount: true}) | ||
|
|
||
| if _, err := d.Validate(context.Background()); err == nil { | ||
| t.Fatal("Validate: want error on OAuth account check failure, got nil") | ||
| } | ||
| } | ||
|
|
||
| // When the account API is reachable (non-token auth), the notice must not fire at all. | ||
| func TestValidateAccountReachableNoNotice(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| ctx := captureLogs(context.Background(), buf) | ||
| d := newValidateConnector(t, &databricks.NoAuth{}, rolesTransport{}) | ||
|
|
||
| if _, err := d.Validate(ctx); err != nil { | ||
| t.Fatalf("Validate: %v", err) | ||
| } | ||
|
|
||
| if got := levelFor(t, buf, accountUnreachableMsg); got != "" { | ||
| t.Errorf("notice fired (level %q) when account API was reachable", got) | ||
| } | ||
| } |
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Bug:
sync-test@v4adds an "Auth Error" step that runs the connector with invalidated credentials and requires exit code 16 (Unauthenticated) or 7 (PermissionDenied); this connector exits 2 (Unknown), so the bump makes thetestjob fail deterministically (run log:Connector exited 2 with invalid credentials; expected Unauthenticated (16) or PermissionDenied (7)).Root cause is
pkg/connector/connector.go:126(and the sibling error paths at :138/:146) returning a barefmt.Errorf, which gRPC maps toUnknown. Wrap credential failures with a status code — e.g.uhttp.WrapErrors(codes.Unauthenticated, "databricks-connector: failed to list workspaces", err)— or hold the bump until that's done.