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

add a provider for google cloud dns #337

Merged
merged 1 commit into from
Sep 17, 2022
Merged

add a provider for google cloud dns #337

merged 1 commit into from
Sep 17, 2022

Conversation

orbatschow
Copy link
Contributor

closes #336

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry for the delay, I had quite a backlog of things 😉

The main change would be to do all this without the GCP SDK.
Isn't there some documentation on how to interact with their API over http for example?

go.mod Outdated Show resolved Hide resolved
docs/gcp.md Outdated Show resolved Hide resolved
internal/settings/errors/validation.go Outdated Show resolved Hide resolved
Comment on lines 104 to 112
dnsClient, err := clouddns.NewService(ctx)
if err != nil {
return nil, err
}

record, err := p.getRecord(dnsClient, recordType)
if err != nil && err != errors.ErrNotFound {
return nil, err
}
Copy link
Owner

Choose a reason for hiding this comment

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

I know this works fine, but I'm fighting hard to have the least dependency as possible.
Can these few calls be made without the sdk package?

Otherwise I'll look into it, feel free to let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qdm12

I can see, that the old code dnsClient, err := clouddns.NewService(ctx)

changed to clouddns.NewService(ctx, option.WithHTTPClient(client), option.WithCredentialsJSON(p.credentials))

I can't tell who made this change, but i can't authenticate properly when option.WithHTTPClient(client) is active.

I also searched for the documentation regarding WithHTTPClient option, but could not manage to find anything describing the difference. The official docs state, this is the way to properly authenticate: https://github.com/googleapis/google-cloud-go#authorization

But they don't mention the HTTP client option.

When I remove the WithHTTPClient option authorization is fully working, otherwise I receive this error:

ERROR getting record resource set: googleapi: Error 401: Request is missing required authentication credential. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.

I am happy to provide a PR that just removes the usage of the client, what do you think?

internal/settings/providers/gcp/provider.go Outdated Show resolved Hide resolved
internal/settings/settings.go Outdated Show resolved Hide resolved
Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

I rebased your branch and pushed changes. Notably:

  • I kept the GCP SDK Go dependency as it was (bumped it up though)
  • You need to place the content of your credentials json file in the config.json at the "credentials" key, as documented.

@qdm12 qdm12 merged commit 7c81592 into qdm12:master Sep 17, 2022
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.

Feature request: Support for Google Cloud DNS
2 participants