`kpt fn` commands includes `functionConfigs` and `Kptfile` by defaults as function input #2894

droot · 2022-03-15T00:09:59Z

This PR introduces following (breaking) changes:

kpt does not treat meta-resources (functionConfigs and Kptfile) in any special way. That means:
- Include functionConfigs and Kptfile by default in the function inputs in kpt fn commands.
- Apply all resources to the cluster in kpt live commands unless local-config annotation is specified
--include-meta-resources flag has been removed fromkpt fn eval and kpt fn source commands.
kpt fn render now includes functionConfigs and Kptfile as input to the functions like other kpt fn

Note: I will add more details about the rationale behind this breaking change. Also this change will depend on following enhancements before merging in main:

kpt should support a way to exclude KRM resources (especially Kptfile and local-configs) so that users who are impacted by this breaking change can use the exclude directive to migrate to this change.
Horizontal transforms (set-namespace, set-labels etc. should handle unrecognized resources such as Kptfile gracefully).

mengqiy · 2022-03-21T20:29:12Z

e2e/testdata/fn-eval/include-meta-resources-v1alpha1/.expected/config.yaml

@@ -16,5 +16,6 @@ runCount: 2
 testType: eval
 image: gcr.io/kpt-fn/set-namespace:v0.1.3
 includeMetaResources: true
+exitCode: 1


Actually this PR removes --includeMetaResources commandline flag, so instead of deleting this test, I re-purposed to test the error if someone specifies this flag. I should probably add comment explaining that.

mengqiy · 2022-03-21T20:34:23Z

e2e/testdata/fn-render/fn-success-with-stderr/.expected/diff.patch

+ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ # See the License for the specific language governing permissions and
+ # limitations under the License.
+-


This diff is not useful. We should to tweak it to get rid of this diff.

Thanks for pointing out. Fixed it.

mengqiy · 2022-03-21T20:37:18Z

e2e/testdata/fn-render/generator-subpkgs/.expected/diff.patch

+--- a/db/fn-config.yaml
+++ b/db/fn-config.yaml
+@@ -1,17 +1,3 @@
+-# Copyright 2021 Google LLC


It seems the license header become part of the diff. It's verbose and not useful.
I guess we don't want the license header to be part of the diff.

Removed the license header.

mengqiy · 2022-03-21T20:37:54Z

e2e/testdata/fn-render/modify-legacy-path-annotation/.expected/diff.patch

+ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ # See the License for the specific language governing permissions and
+ # limitations under the License.
+-


Same comment here and below

mengqiy · 2022-03-21T20:57:58Z

kpt should support a way to exclude KRM resources (especially Kptfile and local-configs) so that users who are impacted by this breaking change can use the exclude directive to migrate to this change.

This is important step. Otherwise, users may not have a clear way to upgrade.

Horizontal transforms (set-namespace, set-labels etc. should handle unrecognized resources such as Kptfile gracefully).

It will impact not only horizontal transformers but also any functions uses OpenAPI (e.g. kubeval).

We also need to verify if the comments in Kptfile can be preserved after going through KRM functions.

droot · 2022-03-22T16:23:13Z

This is important step. Otherwise, users may not have a clear way to upgrade.

This PR should be merged after this enhancement has been implemented. /cc @natasha41575

bharathkkb · 2022-03-23T23:36:52Z

Apply functionConfigs to the cluster in kpt live commands unless local-config annotation is specified

@droot I am wondering what is the usecase for this? Afaict in a cluster functionConfig would not mean anything and only seems useful in the context of a function.

droot · 2022-03-23T23:51:29Z

Apply functionConfigs to the cluster in kpt live commands unless local-config annotation is specified

@droot I am wondering what is the usecase for this? Afaict in a cluster functionConfig would not mean anything and only seems useful in the context of a function.

I should have worded it better (updated it in the PR description). Basically from kpt live standpoint, the model is very simple: A KRM resource is either local or not. If it is local mark it as local-config else it is assumed to be applied to the cluster. From function's perspective, input can come from the items in the resource list, or functionConfig field in resourceList. It shouldn't be be treated in any special manner if it is specified as configPath in the pipeline. The current model around meta-resources leads to lot of confusion and restriction in kpt fn commands (and edge cases as well), and so far we haven't seen any benefits.

natasha41575 · 2022-03-25T18:37:45Z

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go

@@ -57,8 +57,6 @@ func GetEvalFnRunner(ctx context.Context, parent string) *EvalFnRunner {
 		&r.Exec, "exec", "", "run an executable as a function")
 	r.Command.Flags().StringVar(
 		&r.FnConfigPath, "fn-config", "", "path to the function config file")
-	r.Command.Flags().BoolVarP(
-		&r.IncludeMetaResources, "include-meta-resources", "m", false, "include package meta resources in function input")


If we want to print a helpful message when they are using this flag, we can't remove these lines. As you asked, I created a draft PR (#2939) for how to gracefully let users know this is no longer available, you can integrate that code into this PR.

Yes, re-introduced include-meta-resources as hidden flag and throwing an error as suggested in your PR. Thanks.

droot · 2022-03-25T21:42:57Z

@mortent Can you take a look at the kpt live specific changes ? Thanks.

pkg/live/rgpath.go

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go

thirdparty/cmdconfig/commands/cmdsource/cmdsource.go

yuwenma

Overall LGTM. Some minor comments about backward compatible handling

droot · 2022-03-29T23:10:28Z

Overall LGTM. Some minor comments about backward compatible handling

Thanks for the review Yuwen. Addressed the comments.

droot · 2022-03-31T16:34:28Z

Thanks everyone for reviewing!

* refactor starlark function * use kpt before kptdev/kpt#2894 merged * fix a test

nadworny · 2022-08-25T08:52:44Z

hi @droot
I'm a bit confused if this part has been implemented?
kpt should support a way to exclude KRM resources (especially Kptfile and local-configs) so that users who are impacted by this breaking change can use the exclude directive to migrate to this change.

If so, how can I make the kpt fn source to exclude certain resources, in my case the local-config ones?

droot · 2022-08-25T16:10:34Z

hi @droot I'm a bit confused if this part has been implemented? kpt should support a way to exclude KRM resources (especially Kptfile and local-configs) so that users who are impacted by this breaking change can use the exclude directive to migrate to this change.

If so, how can I make the kpt fn source to exclude certain resources, in my case the local-config ones?

We added mechanism to exclude resources on the basis of labels, annotations, group/version/kind/namespace/name in kpt fn eval and kpt fn render. For example, https://kpt.dev/book/04-using-functions/02-imperative-function-execution?id=specifying-exclusions, https://kpt.dev/book/04-using-functions/01-declarative-function-execution?id=specifying-exclusions

I don't think we added to kpt fn source.

If you are combining kpt fn source with kpt fn eval, then you can use the exclude support in kpt fn eval. Can you share how how you are using kpt fn source or your use-case, I might be able to suggest a solution ?

nadworny · 2022-08-25T19:42:23Z

Thanks for the quick reply.

The use case is following:

We are using terraform to apply k8s resources generated by the kpt
First we get the package and render it
After we use kpt fn source to generate the yaml files which are then read by the terraform
Since resources of kind Kptfile or SetLabels aren't valid CRDs in our cluster, we want to exclude them completely

If I understand it correctly, the function exclude capability mentioned above will simply not apply transformations to the excluded resources but will still pass the whole pkg to the next pipe.

droot · 2022-08-25T23:28:58Z

Thanks for the quick reply.

The use case is following:

We are using terraform to apply k8s resources generated by the kpt

First we get the package and render it

After we use kpt fn source to generate the yaml files which are then read by the terraform

Since resources of kind Kptfile or SetLabels aren't valid CRDs in our cluster, we want to exclude them completely

If I understand it correctly, the function exclude capability mentioned above will simply not apply transformations to the excluded resources but will still pass the whole pkg to the next pipe.

Thanks for the sharing the use-case. The best solution will be for kpt fn commands to provide more flexbility around filtering resources especially for -o unwrap option where people are piping to other tools. Sharing a workaround here:

It requires using another tool called kubectl-slice. kubectl-slice can filter out resources on the basis of comma separates names.

kpt fn source -o unwrap|kubectl-slice --exclude-name kptfile.kpt.dev,mariadb --stdout

Hope this helps.

nadworny · 2022-08-26T06:36:42Z

I wasn't aware of kubectl-slice - nice tool!

I have found another workaround using yq:
kpt fn source . | yq '.items[] |= del(select(.kind == "Kptfile"))'

Thanks for your help 👍

droot force-pushed the fn-eval-refactor branch from abbe5d4 to aa14451 Compare March 15, 2022 20:36

droot marked this pull request as ready for review March 15, 2022 22:05

droot changed the title ~~WIP: include functionConfigs by defaults~~ include functionConfig by defaults in kpt fn commands Mar 15, 2022

droot requested review from natasha41575, mengqiy and yuwenma March 17, 2022 21:47

droot changed the title ~~include functionConfig by defaults in kpt fn commands~~ include functionConfig and Kptfile by defaults in kpt fn commands Mar 17, 2022

mengqiy reviewed Mar 21, 2022

View reviewed changes

droot changed the title ~~include functionConfig and Kptfile by defaults in kpt fn commands~~ kpt fn commands includes functionConfigs and Kptfile by defaults as function input Mar 23, 2022

droot force-pushed the fn-eval-refactor branch from f70af2d to 7faaccc Compare March 23, 2022 20:39

bharathkkb mentioned this pull request Mar 24, 2022

Add local-config anno to all functionConfigs and kptfiles GoogleCloudPlatform/blueprints#174

Closed

mengqiy approved these changes Mar 24, 2022

View reviewed changes

natasha41575 reviewed Mar 25, 2022

View reviewed changes

droot added 13 commits March 25, 2022 14:08

include functionConfigs by defaults

1c86a8d

removed functionConfig specific tests

518d609

fixed a few tests

4a93a4e

fixed a typo

45ed239

adjusted the tests to new behavior

cfa917b

adjusted porch for new behavior

40997f9

adjusted the live tests for new functionConfig behavior

f8535b2

commented out unused function

4bda688

added test case for kpt fn render to include meta resources

cbb5424

corrected the local config annotations in test manifests

67f55f6

made functionConfig and Kptfile input to the function by default

885cef7

fixed a test

eeb9749

fixed porch test

b494d56

droot added 3 commits March 25, 2022 14:08

added MatchAllKRM glob and remove metaresourcefiltering

657c8b2

addressed review comments

e1f9de7

re-enabled include-meta-resources flag with error message

c150641

droot force-pushed the fn-eval-refactor branch from 7faaccc to c150641 Compare March 25, 2022 21:41

droot requested a review from mortent March 25, 2022 21:42

yuwenma reviewed Mar 29, 2022

View reviewed changes

pkg/live/rgpath.go Outdated Show resolved Hide resolved

yuwenma reviewed Mar 29, 2022

View reviewed changes

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Outdated Show resolved Hide resolved

yuwenma reviewed Mar 29, 2022

View reviewed changes

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Show resolved Hide resolved

yuwenma reviewed Mar 29, 2022

View reviewed changes

thirdparty/cmdconfig/commands/cmdsource/cmdsource.go Show resolved Hide resolved

yuwenma approved these changes Mar 29, 2022

View reviewed changes

addressed typos and minor cleanup

61543bd

bharathkkb approved these changes Mar 30, 2022

View reviewed changes

mortent approved these changes Mar 31, 2022

View reviewed changes

droot merged commit eb171fc into kptdev:main Mar 31, 2022

droot mentioned this pull request Mar 31, 2022

e2e test: updated golden file for save-fn/override-fn fn eval test #2955

Merged

mengqiy pushed a commit to mengqiy/kpt-functions-catalog that referenced this pull request Apr 4, 2022

use kpt before kptdev/kpt#2894 merged

1dba7eb

mengqiy pushed a commit to GoogleContainerTools/kpt-functions-catalog that referenced this pull request Apr 4, 2022

refactor starlark function (#778)

414d2b7

* refactor starlark function * use kpt before kptdev/kpt#2894 merged * fix a test

yuwenma pushed a commit to GoogleContainerTools/kpt-functions-catalog that referenced this pull request May 4, 2022

refactor starlark function (#778)

9ae99f2

* refactor starlark function * use kpt before kptdev/kpt#2894 merged * fix a test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`kpt fn` commands includes `functionConfigs` and `Kptfile` by defaults as function input #2894

`kpt fn` commands includes `functionConfigs` and `Kptfile` by defaults as function input #2894

droot commented Mar 15, 2022 •

edited

Loading

mengqiy Mar 21, 2022

droot Mar 23, 2022

mengqiy Mar 21, 2022

droot Mar 23, 2022

mengqiy Mar 21, 2022

droot Mar 23, 2022

mengqiy Mar 21, 2022

droot Mar 23, 2022

mengqiy commented Mar 21, 2022

droot commented Mar 22, 2022

bharathkkb commented Mar 23, 2022

droot commented Mar 23, 2022

natasha41575 Mar 25, 2022 •

edited

Loading

droot Mar 25, 2022

droot commented Mar 25, 2022

yuwenma left a comment •

edited

Loading

droot commented Mar 29, 2022

droot commented Mar 31, 2022

nadworny commented Aug 25, 2022 •

edited

Loading

droot commented Aug 25, 2022

nadworny commented Aug 25, 2022 •

edited

Loading

droot commented Aug 25, 2022

nadworny commented Aug 26, 2022 •

edited

Loading

kpt fn commands includes functionConfigs and Kptfile by defaults as function input #2894

kpt fn commands includes functionConfigs and Kptfile by defaults as function input #2894

Conversation

droot commented Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Mar 21, 2022

droot commented Mar 22, 2022

bharathkkb commented Mar 23, 2022

droot commented Mar 23, 2022

natasha41575 Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Mar 25, 2022

yuwenma left a comment • edited Loading

Choose a reason for hiding this comment

droot commented Mar 29, 2022

droot commented Mar 31, 2022

nadworny commented Aug 25, 2022 • edited Loading

droot commented Aug 25, 2022

nadworny commented Aug 25, 2022 • edited Loading

droot commented Aug 25, 2022

nadworny commented Aug 26, 2022 • edited Loading

`kpt fn` commands includes `functionConfigs` and `Kptfile` by defaults as function input #2894

`kpt fn` commands includes `functionConfigs` and `Kptfile` by defaults as function input #2894

droot commented Mar 15, 2022 •

edited

Loading

natasha41575 Mar 25, 2022 •

edited

Loading

yuwenma left a comment •

edited

Loading

nadworny commented Aug 25, 2022 •

edited

Loading

nadworny commented Aug 25, 2022 •

edited

Loading

nadworny commented Aug 26, 2022 •

edited

Loading