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: Support KRM input #170

Closed
wants to merge 1 commit into from

Conversation

aabouzaid
Copy link
Contributor

@aabouzaid aabouzaid commented Jan 24, 2023

This is a quick PoC to support KRM as an input Kubeconform (the implementation is not final, but as mentioned in #160 just to show the idea).

To test the feature:

make local-build
cat fixtures/krm.yaml | ./bin/kubeconform -debug

The output will look like this:

2023/01/24 16:37:20 using schema found at https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/master-standalone/configmap-v1.json
2023/01/24 16:37:20 using schema found at https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/master-standalone/configmap-v1.json

This shows that Kubeconform validated the items inside the KRM resource named krm-function-input.

@aabouzaid aabouzaid mentioned this pull request Jan 24, 2023
@@ -46,6 +49,26 @@ func processResults(cancel context.CancelFunc, o output.Output, validationResult
return result
}

func parseStdin(stdin *os.File) *strings.Reader {
Copy link
Owner

Choose a reason for hiding this comment

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

would be awesome if this could take a reader and return a reader

rl, err := fn.ParseResourceList([]byte(content))
// If it's ResourceList, use the items as the new content.
if err == nil {
content = rl.Items.String()
Copy link
Owner

Choose a reason for hiding this comment

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

That's one part I'm worried about... there are some ridiculous optimisations to ensure we read stdin as a stream, and from previous tickets it looks like some people do indeed pass very large amounts of data via stdin... if we were to do that, this function would have to operate on a stream without buffering the whole thing

}

// Check if the stdin input is ResourceList.
rl, err := fn.ParseResourceList([]byte(content))
Copy link
Owner

Choose a reason for hiding this comment

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

This adds support for ResourceLists (I need to read about this) only if passed via stdin? This would not work if I ran kubeconform ./fixtures/krm.yaml? I think we would need some consistency there

@yannh
Copy link
Owner

yannh commented Jan 24, 2023

Lots of questions I'm sorry :D There is also a Spec for the ResourceList type itself here: https://github.com/kubernetes-sigs/kustomize/blob/master/cmd/config/docs/api-conventions/functions-spec.md do we want to validate both the ResourceList itself AND the resources?

@aabouzaid
Copy link
Contributor Author

@yannh this implementation is just a PoC so no need to give it much attention per se 😅

In general I find the current approach I proposed is not the best because:

  • It will complicate the main code.
  • It will be even more complex to support other KRM features like reading extra config.
  • It will increase the binary from 9 MB to 16 MB.

So I suggest splitting that functionality to a different code which will import kubeconform instead of being embedded in it.

The question now is, do you like to add it as a part of this repo (kubeconform), or I just create a separate repo for it?

@aabouzaid
Copy link
Contributor Author

Closing this PR in favor of #189

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.

2 participants