Feat: ai gateway file convert#2137
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2137 +/- ##
==========================================
- Coverage 36.69% 35.95% -0.75%
==========================================
Files 78 82 +4
Lines 7429 7827 +398
==========================================
+ Hits 2726 2814 +88
- Misses 4457 4748 +291
- Partials 246 265 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Print warnings to stderr | ||
| if len(warnings) > 0 { | ||
| for _, warning := range warnings { | ||
| fmt.Fprintf(os.Stderr, "Warning: %v\n", warning) |
There was a problem hiding this comment.
This would break json output, right?
Are we concerned about that right now?
There was a problem hiding this comment.
I see that we are using YAML everywhere for now. We can revisit this later. Let's add a TODO comment on top so that we don't miss this when we add JSON support.
There was a problem hiding this comment.
Ideally - yes, we should - but i'll get to this once all PRs have been merged.
| return fmt.Errorf("failed to read source file: %w", err) | ||
| } | ||
|
|
||
| converted, warnings, err := convert.Convert(sourceContent, convert.Options{ |
There was a problem hiding this comment.
nitpick: Can we import this as ai2kong and call ai2kong.Convert()?
It will be more verbose and avoid the confusing repetition.
| } | ||
|
|
||
| func validateAi2KongFlags(_ *cobra.Command, _ []string) error { | ||
| if convertSourceFile == "" { |
There was a problem hiding this comment.
We can take only one file at a time for conversion, right?
If yes, can we add a validation for that too.
There was a problem hiding this comment.
For flag type of string - cobra follows "last value" wins - so additional validation is not needed. (same in openapi2kong)
| if docMap, ok := doc.(map[string]interface{}); ok { | ||
| if infoMap, ok := docMap["_info"].(map[string]interface{}); ok { | ||
| // _info exists, update select_tags | ||
| infoMap["select_tags"] = []string{"managed-by:deck-ai"} |
There was a problem hiding this comment.
This can over-write the existing select-tags.
There was a problem hiding this comment.
This is converting an ai gateway file -> deck, the ai gateway file won't have select tags at global level
| } else { | ||
| // _info doesn't exist, create it with select_tags | ||
| docMap["_info"] = map[string]interface{}{ | ||
| "select_tags": []string{"managed-by:deck-ai"}, |
There was a problem hiding this comment.
managed-by:deck-aishould be a constant- Elsewhere, the convention I have seen is
managed_by. Should we follow that or is this one a request from product?
There was a problem hiding this comment.
Will be made constant - this was a suggestion, not a request. I'll update it to managed_by.
| cmd := &cobra.Command{ | ||
| Use: "ai2kong", | ||
| Short: "Generate Kong configuration from AI Gateway configuration", | ||
| Long: `This command takes an AI Gateway 2.0 entity model and converts it to a standard decK state file`, |
There was a problem hiding this comment.
Since we are adding a tag ourselves for our internal working, I think we should explicitly call that out in the description. It will help endusers to know that the tag addition is expected and they shouldn't remove it.
| } | ||
|
|
||
| // Parse the converted YAML to add select_tags to _info | ||
| var doc interface{} |
There was a problem hiding this comment.
nit: I feel L64 to L81 can be abstracted out in a different function.
| defer outFile.Close() | ||
| outputWriter = outFile | ||
| } else { | ||
| outputWriter = os.Stdout |
There was a problem hiding this comment.
This can be the default IMO rather than adding a nesting here with else.
| } | ||
|
|
||
| // marshalToYAML encodes v as YAML using a two-space indent. | ||
| func marshalToYAML(v interface{}) ([]byte, error) { |
There was a problem hiding this comment.
Any reason for a separate func for this?
| err = yaml.Unmarshal([]byte(output), ¤tOutput) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, expectedOutput, currentOutput) |
There was a problem hiding this comment.
Should we consider syncing the output to an ai gateway for validity?
We need to support the decomposed flow as well.
There was a problem hiding this comment.
I was planning to do that in the tests for sync and dump in the chained PR
| var ( | ||
| convertSourceFile string | ||
| convertOutputFile string | ||
| managedByTag = "managed_by:deck-ai" |
There was a problem hiding this comment.
renamed this in the chained PR and made it a const - 25fdbde
| } | ||
|
|
||
| for _, f := range files { | ||
| content, err := os.ReadFile(f) |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
| content, err := os.ReadFile(f) | |
| if strings.Contains(f, "..") { | |
| return nil, fmt.Errorf("invalid file path") | |
| } | |
| content, err := os.ReadFile(f) |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| with: | ||
| egress-policy: audit | ||
| - name: Checkout repository | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
GitHub Action actions/checkout persist Git credentials in workflow - low severity
actions/checkout v2 and above persist the default GITHUB_TOKEN in the repository's local git config when persist-credentials is not set to false, during the workflow run. Subsequent workflow steps or third-party actions can read this token from git configuration, increasing the risk of credential theft or misuse within the pipeline. In order to limit the attack surface when external actions are compromised, ensure persist-credentials is set to false.
Show fix
Remediation: Set persist-credentials: false on actions/checkout steps that do not need to push commits back to the repository. Only keep persist-credentials: true when the workflow explicitly performs authenticated git push operations.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
* feat: support diff and dump * style: lint-fix * fix: error out if syncing to konnect * fix: use positional arg for ai sync * fix: written format version in dump * fix: add go.work and go.work.sum to gitignore * chore: remove go.work and go.work.sum from tracking * tests: add integration tests for sync and dump * ci: add integration test workflow for ai-gateway * fix: rename tag and rebase with file ai2kong * tests: use new rc, add more tests * fix: avoid adding global tags in ai gateway format * fix: address feedback * chore: bump go-kong and go-database-reconciler * fix: optimise checking for ai select tag * fix: remove db propagation delay flag and add more tests for tag matching * Feat/support json and multi file sync (#2158) * feat: support json in ai2kong, ai sync and ai dump * feat: support syncing multiple files * style: refactor based on self review
17e8b7c to
f7f5841
Compare
|
@Prashansa-K need review on 3 new commits - |
Summary
In this PR, we are adding support for a new command
deck file ai2kongwhich accepts a AI gateway declarative config, and converts it to kong 3.x format declarative config to be used with on prem AI Gateway.Implements
#2148