Skip to content

Conversation

asartori86
Copy link
Collaborator

Summary

Add the field control_plane_id to the Konnect schema to support its usage via deck

supersedes #303

Full changelog

  • Add the field control_plane_id to the Konnect schema to support its usage via deck

Issues resolved

n/a

Documentation

dependent PR

Testing

Once confirmed that the changes are done in the correct places, I will add more tests

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

@Prashansa-K
Copy link
Collaborator

@asartori86 There are a few issues with this implementation.

You are adding the field control_plane_id in the dump config as well as writer config. As a result, when I run deck gateway dump command I get two fields in my konnect info as follows:

_format_version: "3.0"
_konnect:
  control_plane_id: 6afab00f-3367-41f8-bd5e-03e293b7a98f
  control_plane_name: default
  ... rest of the config ...

This causes the following issues:

  1. Try re-syncing the same dump file and it produces an error.
    Error: 1 errors occurred: reading file test-id.yaml: validating file content: 1 errors occurred: validation error: object="6afab00f-3367-41f8-bd5e-03e293b7a98f", err=_konnect: Additional property control_plane_id is not allowed
  2. As we are adding both id and name fields in the konnect info block in the dumped file, we would have to run a validation to ensure both identifiers belong to the same CP.
  3. If you have marked the CP name and id flags as mutually exclusive, you would have to retain the same behaviour in the konnect info block in my opinion. The written config and flag config can't have two different behaviours.
    As per our conversation here and the conventions we follow elsewhere in deck config files, we should error out only if the passed configs resolve two different CPs.
  4. We need to mark the flag konnect-runtime-group-name mutually exclusive to konnect-control-plane-id.
    Right now, I can run this deck gateway dump --konnect-control-plane-id=6afab00f-3367-41f8-bd5e-03e293b7a98f --konnect-runtime-group-name=testplane -o test.yaml. Here, I have passed an id of a different CP and no failure occurs.

We need to add integration tests for this change covering all such scenarios.

@asartori86
Copy link
Collaborator Author

Thanks @Prashansa-K for the review. I am a bit confused about the handling of both ID and name. AFIU, the name is also used to support additional logic, e.g., in pkg/file/reader.go:82

builder.isKonnect = dumpConfig.KonnectControlPlane != ""

If the user didn't specify the name but only the ID, we end up comparing the CP with the provided ID with the (probably present) CP named default. If we want to enforce the validity of the pair id-name when both are provided, we should refactor the logic behind the flag konnect-control-plane-name so that it can be left empty, allowing us to determine if the control plane name is set to "default" because the user explicitly specified it.

I could implement the above, but I need some guidance as I am pretty new to deck.

Given that konnect-control-plane-id is hidden for now and mutually exclusive with the *-name flags, the most straightforward approach would be to either dump the control plane name or the ID, not both. If the name is not specified in the config, will any logic break?

@asartori86 asartori86 force-pushed the feat/konnect-control-plane-id branch 2 times, most recently from 6ff4b32 to 7c9632c Compare July 9, 2025 13:47
Add the `control_plane_id` field to enable its usage in `decK`.
@asartori86 asartori86 force-pushed the feat/konnect-control-plane-id branch from 7c9632c to 2240d71 Compare July 9, 2025 13:51
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 28.45%. Comparing base (c62c194) to head (2240d71).

Files with missing lines Patch % Lines
pkg/dump/dump.go 0.00% 2 Missing ⚠️
pkg/file/writer.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
- Coverage   28.45%   28.45%   -0.01%     
==========================================
  Files         117      117              
  Lines       18809    18810       +1     
==========================================
  Hits         5352     5352              
- Misses      12857    12858       +1     
  Partials      600      600              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

3 participants