Skip to content
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

fix: sidecar doesn't work properly for Microsoft ODBC Driver for SQL (#593) #772

Closed
wants to merge 1 commit into from

Conversation

raimdev
Copy link

@raimdev raimdev commented Mar 1, 2023

Reason for Change:

Proxy defines expires_in as a "number", while odbc drivers expects "string". Because
of this jdbc drivers are failing to parse retrieved token. A bit more details in this comment.

Internally we have replaced proxy to patched version and java jdbc driver started working
properly.

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

Please answer the following questions with yes/no:

Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

  • yes
  • no

Notes for Reviewers:

@raimdev raimdev requested a review from aramase as a code owner March 1, 2023 09:20
@@ -57,7 +57,7 @@ type token struct {
RefreshToken string `json:"refresh_token"`

// AAD returns expires_in as a string, ADFS returns it as an int
ExpiresIn json.Number `json:"expires_in"`
ExpiresIn string `json:"expires_in"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skythet As mentioned in the comment, this struct was vendor-ed from the go SDK: https://github.com/Azure/go-autorest/blob/b3899c1057425994796c92293e931f334af63b4e/autorest/adal/token.go#L1055-L1067. This has been the default behavior for a while.

What language SDK are you using? Also, another user opened #773, which seems to indicate the proxy sidecar is working fine with their ODBC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skythet ping! Did you get a chance to look at the comment?

@aramase
Copy link
Member

aramase commented May 1, 2023

Closing this due to inactivity. Feel free to reopen when you get a chance to look at the comments.

@aramase aramase closed this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants