feat(convert): convert to KIC #1050
Conversation
Most things in |
There was a problem hiding this comment.
Discussed with @Tieske on zoom earlier today.
We concluded that there are two blockers / next steps for this PR:
- the implementation needs to avoid creating the deck <-> KIC dep cycle,
- more alignment (an approved product brief / design doc) is needed.
go.mod
Outdated
| @@ -19,6 +19,7 @@ require ( | |||
| github.com/imdario/mergo v0.3.16 | |||
| github.com/kong/go-apiops v0.1.21 | |||
| github.com/kong/go-kong v0.46.0 | |||
| github.com/kong/kubernetes-ingress-controller/v2 v2.11.1 | |||
There was a problem hiding this comment.
This line is effectively adding a module dependency cycle between KIC and decK that we cannot make happen.
Since KIC uses decK as a library, it won't be possible for decK to import KIC as a module. A viable resolution for this would be:
- introduction of an "umbrella" project over the existing decK that would decouple "deck-as-a-library" from "deck-as-a-tool" or
- a massive refactor of deck or KIC.
There was a problem hiding this comment.
The reason for this dependency is to make use of the API Schema definitions for KongClusterPlugin, KongConsumer, KongIngress, KongPlugin, etc. Would it be possible to extract the API Schema definitions into its own module that is imported by both deck and KIC? Similarly to what k8s does here: https://github.com/kubernetes/api
There was a problem hiding this comment.
Essentially, extract type definitions under https://github.com/Kong/kubernetes-ingress-controller/tree/main/pkg/apis/configuration into its own module.
There was a problem hiding this comment.
The eng/PM decision reached is that we'll unblock deck2kic's dependency on KIC by implementing the proposal tracked by #1060.
Both styles (annotations-based and KongIngress-based) are supported in this implementation. By default it will generate an annotation-based output with no |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1050 +/- ##
==========================================
+ Coverage 5.58% 21.52% +15.93%
==========================================
Files 36 53 +17
Lines 3884 5385 +1501
==========================================
+ Hits 217 1159 +942
- Misses 3650 4135 +485
- Partials 17 91 +74 ☔ View full report in Codecov by Sentry. |
|
@mflendrich @GGabriele @battlebyte I have cherry-picked the #1117 changes in here, rebased and fixed up. Please give this another review. This should be close to merging now I think. |
Oh, this is what prompted that split? We were probably fine without that, the API packages are pretty isolated from the rest of the code and don't import any of the parser code that loads deck. The parser code does import the API, but that's just a duplicate leaf, not a cycle. Rest of my review was mostly just looking over output-expected.yamls, since AFAIK the code's already been reviewed and we're mostly concerned with the end output. Dunno if we'd block this as-ism but we'll eventually need KIC 3.x upgrades and should add some additional tests to confirm that the config is valid. Development concurrent with KIC 3.x bit us:
Aside details, it looks like we just have expected output tests, not integration. Human review of large generated outputs will be error-prone, so I think we'd be better off tossing configuration into an actual instance to see if it's accepted. We have an existing KIC E2E test that'd serve as a good starting point because it runs the admission webhook and is basically self-contained (the test function spawns its own Kubernetes cluster and installs KIC+Kong; it doesn't require any additional setup), though we'd probably want to simplify the CI workflow a bit. With that we can have the test exec a |
There was a problem hiding this comment.
the API packages are pretty isolated from the rest of the code and don't import any of the parser code that loads deck. The parser code does import the API, but that's just a duplicate leaf, not a cycle.
This can work in the context of low-level package mechanics but stops working when we consider the larger-scale dependency hell problem. If we allowed KIC version X to pull in deck version Y pulling in KIC version Z, this would mean an (effectively) irreversible need to make coordinated changes in lockstep (between separately governed projects) from this point onwards. The opportunity cost of a dependency hell that we've avoided is the real win here, not "code compiles but it otherwise wouldn't".
To the point of the review:
- Now that the deck<->KIC issue is resolved, my reason for "changes requested" is gone.
- As Travis pointed out, KIC's API has evolved -
KongIngressandkongCredTypeare deprecated as of KIC 3.0. This means that the proposed implementation will generate configuration that is considered deprecated and may cause validation warnings (+ will stop working in a future version of KIC). I think this issue is serious enough to fix in this PR, but I'll leave it to the APIOps folks so that we don't have too many cooks in the kitchen. - Not necessarily for this PR: Travis's point about testing. Our k8s integration testing experience is that with new k8s versions and KIC/Kong versions you'll be bumping into the "configuration that was working has stopped working" class of errors that will be missed (like we could have missed the KIC 3.0 API changes in the bullet point above). My (soft) review recommendation is what Travis says: instead of "golden output" testing, have an integration test involving KIC.
TL;DR:
- the dependency blocker is gone, I'm removing my "changes requested"
- did not review for code style/correctness
- non-blocking (but very important) recommendation to upgrade the API to KIC 3.0+
- non-blocking suggestion to consider integration testing
mflendrich
dismissed
their stale review
the blocker (deck<->KIC dependency) is gone
|
@czeslavo I believe @battlebyte has made the requested changes. Could you take a final look and ✅ if you're happy? |
|
Ah, I just noticed no additional commits on the branch. I couldn't see |
|
See #1218 for the implementation of all feedback |
|
@czeslavo @battlebyte @GGabriele @mflendrich Then we can finally merge. |
|
This is the final command reference: |
|
@battlebyte The only one of these I would consider altering is: Instead WDYT about |
|
@rspurgeon yes, that is something I considered as well. I'm fine changing it, no problem. Any thoughts on that @mheap ? |
|
Big +1 for that change
…On Mon, Feb 19, 2024 at 3:56 PM Jordi Fernandez ***@***.***> wrote:
@rspurgeon <https://github.com/rspurgeon> yes, that is something I
considered as well. I'm fine changing it, no problem. Any thoughts on that
@mheap <https://github.com/mheap> ?
—
Reply to this email directly, view it on GitHub
<#1050 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAON6RLXYADT436IGVDANTYUNY2FAVCNFSM6AAAAAA52AF2VKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJSG42TQOBQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@rspurgeon @mheap changed applied. New command reference: |
|
@GGabriele @czeslavo Could you please take a look? I'd like to get this released with the other changes on |
cmd/file_kong2kic.go
Outdated
| logbasics.Initialize(log.LstdFlags, verbosity) | ||
|
|
||
| // if cmdKong2KicVersion is not 2 or 3 return an error | ||
| if cmdKong2KicVersion != "2" && cmdKong2KicVersion != "3" { |
There was a problem hiding this comment.
since you already have a validateInput helper function, it would make sense to consolidate all input validation there.
cmd/file_kong2kic.go
Outdated
|
|
||
| // | ||
| // | ||
| // Define the CLI data for the openapi2kong command |
There was a problem hiding this comment.
| // Define the CLI data for the openapi2kong command | |
| // Define the CLI data for the kong2kic command |
kong2kic/types.go
Outdated
| KongUpstreamPolicies []kicv1beta1.KongUpstreamPolicy `json:"upstreamPolicies,omitempty" yaml:",omitempty"` | ||
| } | ||
|
|
||
| func (k KICContent) marshalKICContentToYaml() ([]byte, error) { |
There was a problem hiding this comment.
are marshalKICContentToJSON and marshalKICContentToYaml the same except for the format used? If so, we should consolidate them into a single one:
func (k KICContent) marshalKICContentToFormat(format string) ([]byte, error) {
var output []byte
const (
yamlSeparator = "---\n"
)
for _, kongIngress := range k.KongIngresses {
kongIngresses, err := SerializeObjectDroppingFields(kongIngress, format)
if err != nil {
return nil, err
}
output = append(output, kongIngresses...)
if format == file.YAML {
output = append(output, []byte(yamlSeparator)...)
}
}
...
...
}
| var c []byte | ||
| var err error | ||
|
|
||
| switch format { |
There was a problem hiding this comment.
Isn't it possible to do the following instead? (untested)
switch yamlOrJSON {
case file.YAML:
c, err = MarshalKongToKICYaml(content, string(format))
case file.JSON:
c, err = MarshalKongToKICJson(content, string(format))
}
if err != nil {
return err
}
kong2kic/writer.go
Outdated
| var err error | ||
|
|
||
| switch format { | ||
| // case YAML: |
kong2kic/writer.go
Outdated
|
|
||
| if filename == "-" { | ||
| if _, err := fmt.Print(string(c)); err != nil { | ||
| return fmt.Errorf("writing file: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("writing file: %w", err) | |
| return fmt.Errorf("writing to stdout: %w", err) |
|
Thanks @GGabriele, all your suggestions applied in commit d49ecd0. |
Much cleaner, thanks Jordi! |
There was a problem hiding this comment.
I left some comments. It's a massive volume of code to review and I'm not sure how deep we should go with the review here.
Note: Given this is not integration-tested, I wouldn't be very comfortable with advertising the functionality as something ready to use in prod. Are we going to implement tests that apply the resulting manifests to a K8s cluster with KIC running, ensuring all the routes work as expected before releasing it? If not, I think this should be advertised as an experimental feature.
|
RE: Integration tests, I'm comfortable with manual testing for the initial release. We should add integration tests as a followup |
|
Thanks @czeslavo , missing continues fixed. |
New command "deck file kong2kic" Kong configuration file to Kong Ingress Controller manifests.
Currently, only ingress controller manifests are supported. Output format is always YAML.
Jira: APIOPS-8