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

deps(refactor) Switch from stackit-dns-api-client-go to stackit-sdk-go #14

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

SimonKienzler
Copy link
Contributor

@SimonKienzler SimonKienzler commented Mar 1, 2024

This PR replaces the previously used https://github.com/stackitcloud/stackit-dns-api-client-go with the official https://github.com/stackitcloud/stackit-sdk-go in order to

  1. remove the dependency on a one-off client SDK that needs to be maintained separately and
  2. allow users to make use of features provided by the STACKIT SDK, such as additional authentication options.

The largest portion of the changes in terms of LoC are struct type switches, which should not be too much of an issue. However, as I was previously unfamiliar with the code base, I'd like to point reviewers to the following changes, which might be worth a closer look:

  • The NewStackitDNSProvider() function now takes variadic STACKIT config options as the last argument in order to override the client and authentication config for the unit tests.
  • I removed the token and baseURL fields from the provider's Config struct, as these values are only relevant to the STACKIT SDK, which gets them via the variadic config options now.
  • I also removed the required auth-token flag/env var setting, as the SDK client itself throws a panic when it is not properly set up with either a token or a key. Double-wrapping this with a check in the root command seemed unnecessary to me.
    • auth config (either token or key) are now handled in the Viper setup and then passed to the STACKIT client using ConfigOptions
  • The rewritten pager logic in both fetchRecords() and fetchZones() passes all unit tests, but might be worth a rigorous review nonetheless.
  • In the tests, the last test case in getApplyChangesBasicTestCases() was removed. This test case had a comment saying swagger client does not return an error when the response is invalid json, which is now obsolete. The behavior actually differs now between CREATE and DELETE/UPDATE, as the Create() call actually tries to parse the response. Therefore, this test case is of little use. Please advise if you prefer this to be handled differently.
  • I kept changes to the README short , and rather added a link to the SDK auth options in order to not duplicate soon-to-be outdated information.

@SimonKienzler
Copy link
Contributor Author

@PatrickKoss just FYI, we tested a pre-build of the code in this PR in our environment, and both the Key and the Token flow work as expected. For the token flow, it's almost a drop-in replacement, the upgrade path only consists of changing AUTH_TOKEN to STACKIT_SERVICE_ACCOUNT_TOKEN in the Deployment's env settings, as demonstrated in the updated README.

README.md Outdated
@@ -203,7 +205,7 @@ spec:
successThreshold: 1
timeoutSeconds: 5
env:
- name: AUTH_TOKEN
- name: STACKIT_SERVICE_ACCOUNT_TOKEN
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we support AUTH_TOKEN as well? I don´t feel like doing a major version upgrade from 0.x.x to 1.x.x yet. Since it is a breaking change we unfortunate need one. We can trick in some small logic like

os.SetEnv("STACKIT_SERVICE_ACCOUNT_TOKEN", authToken)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I backed off of the idea to basically "leak" the configuration options from the STACKIT SDK via Env Vars, and implemented a backwards-compatible way to support service account keys. The webhook now supports two auth types with cobra-native flags (auth-token as before and now also auth-key-path). The configuration of the STACKIT client is completely handled in code afterwards.

README.md Show resolved Hide resolved
@SimonKienzler
Copy link
Contributor Author

@PatrickKoss PTAL 😊

BTW, what's up with the failing check for a semantic PR title? I based the title on https://github.com/stackitcloud/external-dns-stackit-webhook/blob/main/.github/semantic.yml and thought it fulfills the requirements 🤔

Copy link
Collaborator

@PatrickKoss PatrickKoss left a comment

Choose a reason for hiding this comment

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

looks good to me

@PatrickKoss
Copy link
Collaborator

@PatrickKoss PTAL 😊

BTW, what's up with the failing check for a semantic PR title? I based the title on https://github.com/stackitcloud/external-dns-stackit-webhook/blob/main/.github/semantic.yml and thought it fulfills the requirements 🤔

its somehow broken :D That is something we need to fix #soon

@PatrickKoss PatrickKoss merged commit c09c70d into stackitcloud:main Mar 8, 2024
3 of 4 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.

2 participants