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(konnect): add client configuration #3455

Merged
merged 6 commits into from
Jan 31, 2023
Merged

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Jan 30, 2023

What this PR does / why we need it:

  • Adds Konnect flags (default-false --konnect-sync-enabled feature flag along with Konnect Admin API client-specific configuration)
  • Adds a constructor for Konnect Admin API (NewKongClientForKonnectRuntimeGroup) that is to be used in the next PRs
  • Refactors code around TLS client certificates to not duplicate code handling regular Admin API and Konnect configs

Which issue this PR fixes:

Part of #3437.

Notes to reviewers:

Even though the PR adds Konnect Admin Admin API client implementation, it does not use it yet. Next PRs are going to bring more tests and integrate the Konnect client into the dataplane loop. It's to keep the size of the PRs reasonable.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@czeslavo czeslavo self-assigned this Jan 30, 2023
@czeslavo czeslavo added this to the KIC v2.9.0 milestone Jan 30, 2023
@czeslavo czeslavo marked this pull request as ready for review January 30, 2023 22:00
@czeslavo czeslavo requested a review from a team as a code owner January 30, 2023 22:00
@czeslavo czeslavo force-pushed the konnect-client-config branch 2 times, most recently from 1516a2a to a8cae48 Compare January 30, 2023 22:29
@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 73.8% // Head: 73.6% // Decreases project coverage by -0.3% ⚠️

Coverage data is based on head (a1f7141) compared to base (de3499f).
Patch coverage: 56.9% of modified lines in pull request are covered.

❗ Current head a1f7141 differs from pull request most recent head 3c1e696. Consider uploading reports for the commit 3c1e696 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3455     +/-   ##
=======================================
- Coverage   73.8%   73.6%   -0.3%     
=======================================
  Files        115     118      +3     
  Lines      13971   14067     +96     
=======================================
+ Hits       10322   10356     +34     
- Misses      2993    3051     +58     
- Partials     656     660      +4     
Impacted Files Coverage Δ
internal/adminapi/konnect.go 0.0% <0.0%> (ø)
internal/cmd/rootcmd/run.go 21.0% <0.0%> (ø)
internal/dataplane/kong_client.go 87.1% <ø> (ø)
internal/manager/utils/kongconfig/root.go 49.5% <ø> (ø)
internal/adminapi/client.go 13.3% <15.3%> (-42.5%) ⬇️
internal/adminapi/tls.go 58.6% <58.6%> (ø)
internal/adminapi/kong.go 60.0% <60.0%> (ø)
internal/manager/setup.go 52.6% <61.5%> (-1.9%) ⬇️
internal/admission/server.go 42.0% <76.9%> (+1.1%) ⬆️
internal/manager/config_validation.go 90.6% <88.2%> (-9.4%) ⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

Generally good, only some small comments reamains.

internal/adminapi/client.go Show resolved Hide resolved
internal/adminapi/tls.go Show resolved Hide resolved
internal/adminapi/tls.go Show resolved Hide resolved
internal/manager/config_validation.go Show resolved Hide resolved
internal/adminapi/kong.go Outdated Show resolved Hide resolved
internal/adminapi/kong.go Outdated Show resolved Hide resolved
internal/adminapi/kong.go Outdated Show resolved Hide resolved
internal/adminapi/tls.go Outdated Show resolved Hide resolved
internal/adminapi/client.go Show resolved Hide resolved
internal/adminapi/client.go Show resolved Hide resolved
internal/manager/config_validation.go Show resolved Hide resolved
internal/adminapi/kong.go Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
@czeslavo czeslavo requested a review from pmalek January 31, 2023 15:08
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
@czeslavo czeslavo requested a review from pmalek January 31, 2023 15:39
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

👍 ( I already feel the pain resolving conflicts: #3421)

Just make sure to resolve all the conversations also from @randmonkey

@czeslavo
Copy link
Contributor Author

czeslavo commented Jan 31, 2023

I already feel the pain resolving conflicts: #3421

Oh oh, sorry for the inconvenience. 🙁

Just make sure to resolve all the conversations also from @randmonkey

@czeslavo czeslavo enabled auto-merge (squash) January 31, 2023 15:55
@czeslavo czeslavo merged commit dfd8961 into main Jan 31, 2023
@czeslavo czeslavo deleted the konnect-client-config branch January 31, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants