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

feat: support Cloud SQL CAS instances. #850

Merged
merged 17 commits into from
Aug 8, 2024

Conversation

feng-zhe
Copy link
Collaborator

@feng-zhe feng-zhe commented Aug 2, 2024

The CAS instances will have a different way to verify the server identity. This CL supports that.

@feng-zhe feng-zhe requested a review from a team as a code owner August 2, 2024 17:58
@feng-zhe
Copy link
Collaborator Author

feng-zhe commented Aug 2, 2024

FYI, manually tested the auth proxy client with this PR against the CAS and non-CAS instances. It works well.

@jackwotherspoon
Copy link
Collaborator

Thanks @feng-zhe I'll take a deep look at this early next week 😄

Also renamed gotDNS to gotPSCDNS to make it clear that the DNS from PSC address.
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Could we add an e2e test for this feature as well? I assume we'd need a new Cloud SQL Instance with this CA mode configured correctly. @jackwotherspoon can you help with that?

internal/cloudsql/instance.go Outdated Show resolved Hide resolved
internal/cloudsql/instance.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hessjcg hessjcg left a comment

Choose a reason for hiding this comment

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

Nice start. This needs some restructuring to avoid redundant fields.

internal/cloudsql/instance.go Outdated Show resolved Hide resolved
internal/cloudsql/instance.go Outdated Show resolved Hide resolved
DBVersion string
DNSName string // Only set for CAS instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

DomainName (not DNSName)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure about this. Because this DNSName is directly from the dnsName of the ConnectSettings API here.

Copy link
Member

Choose a reason for hiding this comment

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

Probably better to stick with the wording from the ConnectSettings API. We could add a godoc comment explaining what this is used for though.

internal/cloudsql/instance.go Show resolved Hide resolved
internal/cloudsql/refresh.go Outdated Show resolved Hide resolved
internal/cloudsql/instance.go Show resolved Hide resolved
@hessjcg
Copy link
Contributor

hessjcg commented Aug 5, 2024

You may also want to incorporate the changes from #843. This will allow customers to configure the connector using a DNS name.

@feng-zhe
Copy link
Collaborator Author

feng-zhe commented Aug 5, 2024

You may also want to incorporate the changes from #843. This will allow customers to configure the connector using a DNS name.

Unfortunately, for CAS instances, there will be no DNS records for it. The only change would be the server CA certificates , the DNS name, and how we verify it. I feel #843 is a separate feature where we do have SRV records set up. WDYT? Thanks.

@feng-zhe
Copy link
Collaborator Author

feng-zhe commented Aug 6, 2024

Could we add an e2e test for this feature as well? I assume we'd need a new Cloud SQL Instance with this CA mode configured correctly. @jackwotherspoon can you help with that?

Thanks. Jack helped me and I have added some e2e tests for it. However, the testing PR #854 failed due to some password issue. Debugging it now. Thanks.

@feng-zhe feng-zhe requested a review from hessjcg August 6, 2024 23:03
@feng-zhe feng-zhe requested a review from enocom August 7, 2024 17:51
@feng-zhe
Copy link
Collaborator Author

feng-zhe commented Aug 7, 2024

@enocom Hi Eno, FYI, with @jackwotherspoon's help, the added e2e test has passed in #854. Thanks.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

A big milestone. Thanks Henry! 👏

Copy link
Collaborator

@jackwotherspoon jackwotherspoon left a comment

Choose a reason for hiding this comment

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

LGTM ✅ Thanks a ton Henry!

Copy link
Contributor

@hessjcg hessjcg left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work.

@feng-zhe feng-zhe merged commit 511fae4 into GoogleCloudPlatform:main Aug 8, 2024
11 checks passed
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.

4 participants