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: add Konnect client & move tls cert extract to util #3469

Merged
merged 8 commits into from
Feb 9, 2023

Conversation

randmonkey
Copy link
Contributor

@randmonkey randmonkey commented Feb 2, 2023

What this PR does / why we need it:

  • Add a Konnect client to call APIs not inside Kong client. Currently node APIs.
  • upload status of node for KIC itself in Konnect

Which issue this PR fixes:

expected to fix #3438

Special notes for your reviewer:

This PR will only include updating nodes for KIC itself to Konnect to avoid it being too large. works for uploading status of managed kong gateway nodes are tracked in issue #3501.

Includes extracting code for getting TLS cert into internal/util/tls package. If this change is useful, I'll consider move the change to a separate PR.

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

@randmonkey randmonkey requested a review from a team as a code owner February 2, 2023 10:10
@randmonkey randmonkey marked this pull request as draft February 2, 2023 10:11
@randmonkey randmonkey added the do not merge let the author merge this, don't merge for them. label Feb 2, 2023
@randmonkey randmonkey added this to the KIC v2.9.0 milestone Feb 2, 2023
@randmonkey randmonkey added the area/feature New feature or request label Feb 2, 2023
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Base: 73.8% // Head: 72.8% // Decreases project coverage by -1.1% ⚠️

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

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

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3469     +/-   ##
=======================================
- Coverage   73.8%   72.8%   -1.1%     
=======================================
  Files        116     119      +3     
  Lines      13883   14099    +216     
=======================================
+ Hits       10258   10272     +14     
- Misses      2962    3167    +205     
+ Partials     663     660      -3     
Impacted Files Coverage Δ
internal/adminapi/konnect.go 0.0% <0.0%> (ø)
internal/adminapi/tls.go 100.0% <ø> (+41.3%) ⬆️
internal/konnect/client.go 0.0% <0.0%> (ø)
internal/konnect/node_agent.go 0.0% <0.0%> (ø)
internal/manager/run.go 45.1% <0.0%> (-2.9%) ⬇️
internal/util/tls/tls_cert.go 55.5% <55.5%> (ø)
internal/adminapi/kong.go 61.1% <100.0%> (+1.1%) ⬆️
...nternal/controllers/gateway/udproute_controller.go 71.6% <0.0%> (-2.2%) ⬇️
internal/dataplane/parser/parser.go 90.0% <0.0%> (-0.7%) ⬇️
... and 3 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.

internal/konnect/client.go Outdated Show resolved Hide resolved
internal/util/tls/tls.go Outdated Show resolved Hide resolved
internal/adminapi/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Fixed Show fixed Hide fixed
@randmonkey randmonkey removed the do not merge let the author merge this, don't merge for them. label Feb 7, 2023
@randmonkey randmonkey marked this pull request as ready for review February 7, 2023 08:14
pmalek
pmalek previously requested changes Feb 7, 2023
internal/util/tls/tls_cert.go Outdated Show resolved Hide resolved
internal/util/tls/tls_cert.go Outdated Show resolved Hide resolved
internal/manager/run.go Outdated Show resolved Hide resolved
internal/konnect/protocol.go Outdated Show resolved Hide resolved
internal/konnect/node_agent.go Show resolved Hide resolved
internal/konnect/node_agent.go Outdated Show resolved Hide resolved
internal/konnect/node_agent.go Show resolved Hide resolved
internal/konnect/node_agent.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/client.go Outdated Show resolved Hide resolved
internal/konnect/node_agent.go Outdated Show resolved Hide resolved
internal/manager/run.go Outdated Show resolved Hide resolved
internal/util/tls/tls_cert.go Outdated Show resolved Hide resolved
@randmonkey randmonkey force-pushed the feat/kic_konnect_node_client branch 3 times, most recently from 9cc6489 to 0dfc564 Compare February 8, 2023 06:55
@czeslavo
Copy link
Contributor

czeslavo commented Feb 8, 2023

IMO from the functional perspective this PR might be already merged. We'd just need to create tracking issues for all the TODOs left here and there (handling the task shutdown, parsing error response, etc.).

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 also feel that we can merge this but I'd prefer to remove all those TODOs with references to issues created in this repo.

internal/konnect/client.go Outdated Show resolved Hide resolved
@randmonkey
Copy link
Contributor Author

I also feel that we can merge this but I'd prefer to remove all those TODOs with references to issues created in this repo.

put the followups into 2 issues:

czeslavo
czeslavo previously approved these changes Feb 9, 2023
@randmonkey randmonkey enabled auto-merge (squash) February 9, 2023 13:36
@randmonkey randmonkey merged commit f6611e3 into main Feb 9, 2023
@randmonkey randmonkey deleted the feat/kic_konnect_node_client branch February 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/feature New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Register KIC and data-plane nodes in Runtime Group's Node API
3 participants