[WIP] fix: Require src namespace in apply-time-mutation #482

karlkfi · 2021-11-15T22:23:34Z

This change dissables implicit namespace resolution for
apply-time-mutation sourceRefs. This feature has been partially
broken since it was implimented and fixing it is not currently
possible. The sorting never worked properly because the namespace
was being infered at apply time, while sorting was being done
much earlier, before the tasks were scheduled.
Unfortunately, this makes testing and usage harder when the
namespace is not known in advance, requiring yaml templating.
Avoid logging yaml at log lvl 5 (use lvl 7)
Improve error messages when annotation read/write fails
Warn if dependencies cannot be sorted
Warn if dependency not in resource set

Fixes: #481

k8s-ci-robot · 2021-11-15T22:23:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: karlkfi
To complete the pull request process, please assign mortent after the PR has been reviewed.
You can assign the PR to them by writing /assign @mortent in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karlkfi · 2021-11-15T22:28:35Z

pkg/object/graph/depends.go

 		klog.V(3).Infof("adding vertex: %s", id)
 		g.AddVertex(id)
 		if mutation.HasAnnotation(obj) {
 			subs, err := mutation.ReadAnnotation(obj)
 			if err != nil {
-				// TODO: fail task if parse errors?
-				klog.V(3).Infof("failed to add edges from: %s: %s", id, err)
+				klog.Warningf("warning: failed to sort explicit dependency: %v",


klog.Warning prints directly to the user when using kapply or kpt, but for server-side usage it would just print to the service log.

Should we make a new WarningEvent instead, so the printer can handle it?

The Warning event would be a new top-level event type then? It seems like it would be only for the user to understand what is going on, as it seems difficult to use it for anything useful from code? Another option (perhaps in addition to a Warning event) would be to allow users to specify if invalid references should lead to an error or not. Although that would require that we can fit the behavior into the continue-on-error strategy, but this happens before we apply any of the resources.

So I think the question here is what should happen if either the annotation can't be read or it doesn't specify a namespace in a reference to a namespace-scoped resource. If we print an error or send an error event, then presumably we will go on with applying the resource right? I think considering the resource as failed/skipped in this situation might be more appropriate, and we can provide information back to the caller through the Error property on the ApplyEvent. It seems that applying the resource without doing the apply-time mutation almost certainly will result in a different outcome than the user expected. And by just considering that particular resource as failed/skipped, we will still continue applying other resources.

I've defined the problem and a couple proposals here: #484

test/e2e/mutation_test.go

- This change dissables implicit namespace resolution for apply-time-mutation sourceRefs. This feature has been partially broken since it was implimented and fixing it is not currently possible. The sorting never worked properly because the namespace was being infered at apply time, while sorting was being done much earlier, before the tasks were scheduled. - Unfortunately, this makes testing and usage harder when the namespace is not known in advance, requiring yaml templating. - Avoid logging yaml at log lvl 5 (use lvl 7) - Improve error messages when annotation read/write fails - Warn if dependencies cannot be sorted - Warn if dependency not in resource set

mortent · 2021-11-15T23:23:06Z

pkg/apply/mutator/apply_time_mutator.go

@@ -78,7 +78,7 @@ func (atm *ApplyTimeMutator) Mutate(ctx context.Context, obj *unstructured.Unstr

 		// Default source namespace to target namesapce, if namespace-scoped


We should probably fix the comment here.

mortent · 2021-11-15T23:47:24Z

pkg/object/graph/depends.go

 		klog.V(3).Infof("adding vertex: %s", id)
 		g.AddVertex(id)
 		if mutation.HasAnnotation(obj) {
 			subs, err := mutation.ReadAnnotation(obj)
 			if err != nil {
-				// TODO: fail task if parse errors?
-				klog.V(3).Infof("failed to add edges from: %s: %s", id, err)
+				klog.Warningf("warning: failed to sort explicit dependency: %v",


The Warning event would be a new top-level event type then? It seems like it would be only for the user to understand what is going on, as it seems difficult to use it for anything useful from code? Another option (perhaps in addition to a Warning event) would be to allow users to specify if invalid references should lead to an error or not. Although that would require that we can fit the behavior into the continue-on-error strategy, but this happens before we apply any of the resources.

k8s-ci-robot · 2022-01-08T10:52:32Z

@karlkfi: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-04-05T16:20:02Z

@karlkfi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
cli-utils-presubmit-master-stress	`51341e4`	link	true	`/test cli-utils-presubmit-master-stress`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2022-07-04T16:59:14Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-08-03T17:59:09Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

karlkfi · 2022-08-11T18:18:42Z

/remove-lifecycle rotten

k8s-triage-robot · 2022-11-09T18:31:37Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-12-09T19:02:26Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2023-01-08T19:48:23Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-01-08T19:48:27Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

karlkfi requested review from seans3 and mortent and removed request for seans3 November 15, 2021 22:23

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 15, 2021

k8s-ci-robot requested review from ash2k and Liujingfang1 November 15, 2021 22:23

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2021

karlkfi force-pushed the karl-dynamic-sorting branch from 670d92b to 9cebcd9 Compare November 15, 2021 22:25

karlkfi commented Nov 15, 2021

View reviewed changes

test/e2e/mutation_test.go Outdated Show resolved Hide resolved

karlkfi force-pushed the karl-dynamic-sorting branch from 9cebcd9 to 522b7b4 Compare November 15, 2021 22:53

karlkfi force-pushed the karl-dynamic-sorting branch from 522b7b4 to 51341e4 Compare November 15, 2021 23:05

mortent reviewed Nov 15, 2021

View reviewed changes

ash2k mentioned this pull request Nov 16, 2021

What is the security model of apply-time mutation feature? #483

Closed

karlkfi changed the title ~~fix: Require src namespace in apply-time-mutation~~ [WIP] fix: Require src namespace in apply-time-mutation Dec 8, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2022

karlkfi mentioned this pull request Mar 25, 2022

Bug: implicit namespace resolution in apply-time-mutation causes dependency sorting failure #481

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 4, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 3, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 11, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 9, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 9, 2022

k8s-ci-robot closed this Jan 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fix: Require src namespace in apply-time-mutation #482

[WIP] fix: Require src namespace in apply-time-mutation #482

karlkfi commented Nov 15, 2021 •

edited

Loading

k8s-ci-robot commented Nov 15, 2021

karlkfi Nov 15, 2021

mortent Nov 15, 2021

mortent Nov 16, 2021

karlkfi Nov 16, 2021

mortent Nov 15, 2021

mortent Nov 15, 2021

k8s-ci-robot commented Jan 8, 2022

k8s-ci-robot commented Apr 5, 2022

k8s-triage-robot commented Jul 4, 2022

k8s-triage-robot commented Aug 3, 2022

karlkfi commented Aug 11, 2022

k8s-triage-robot commented Nov 9, 2022

k8s-triage-robot commented Dec 9, 2022

k8s-triage-robot commented Jan 8, 2023

k8s-ci-robot commented Jan 8, 2023

		@@ -78,7 +78,7 @@ func (atm ApplyTimeMutator) Mutate(ctx context.Context, obj unstructured.Unstr

		// Default source namespace to target namesapce, if namespace-scoped

[WIP] fix: Require src namespace in apply-time-mutation #482

[WIP] fix: Require src namespace in apply-time-mutation #482

Conversation

karlkfi commented Nov 15, 2021 • edited Loading

k8s-ci-robot commented Nov 15, 2021

karlkfi Nov 15, 2021

Choose a reason for hiding this comment

mortent Nov 15, 2021

Choose a reason for hiding this comment

mortent Nov 16, 2021

Choose a reason for hiding this comment

karlkfi Nov 16, 2021

Choose a reason for hiding this comment

mortent Nov 15, 2021

Choose a reason for hiding this comment

mortent Nov 15, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 8, 2022

k8s-ci-robot commented Apr 5, 2022

k8s-triage-robot commented Jul 4, 2022

k8s-triage-robot commented Aug 3, 2022

karlkfi commented Aug 11, 2022

k8s-triage-robot commented Nov 9, 2022

k8s-triage-robot commented Dec 9, 2022

k8s-triage-robot commented Jan 8, 2023

k8s-ci-robot commented Jan 8, 2023

karlkfi commented Nov 15, 2021 •

edited

Loading