Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Update hncconfig webhook to handle object overwriting and refactor forest sourceObjects field #1106

Merged
merged 2 commits into from
Sep 23, 2020
Merged

Update hncconfig webhook to handle object overwriting and refactor forest sourceObjects field #1106

merged 2 commits into from
Sep 23, 2020

Conversation

yiqigao217
Copy link
Contributor

@yiqigao217 yiqigao217 commented Sep 16, 2020 •
edited
Loading

Update forest field sourceObjects to store all the names of user-created
objects by GVK as long as there's an object reconciler for that GVK.

Change object reonciler behaviors dependent on the previous forest field
originalObjects, which only stores source objects for 'Propagate' types.
Add test cases for corner cases.

Add new hncconfig webhook rules and new test cases. Check if changing
type mode(s) to Propagate would cause overwriting source by looking
up the objectNames data. Deny the request and output all the conflicts
if it would cause overwriting.

Tested by "make test" and manually on a GKE cluster with multiple
objects with the same name in the same tree or different trees. Passed
all e2e tests too.

The webhook message would look like this:

Cannot update configuration because setting type "/v1, Kind=Secret" to 'Propagate' mode would overwrite user-created object(s):
  Object "my-creds2" in namespace "acme-org" would overwrite the one in "team-a"
  Object "my-creds2" in namespace "acme-org" would overwrite the one in "team-b"
  Object "my-creds" in namespace "bcme-org" would overwrite the one in "team-c"
  Object "my-creds2" in namespace "bcme-org" would overwrite the one in "team-c"
To fix this, please rename or remove the conflicting objects first.

Part of #1102, #1076

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2020
This was referenced Sep 16, 2020
Closed
Closed
@yiqigao217
Copy link
Contributor Author

/retest

@yiqigao217
Copy link
Contributor Author

/assign @adrianludwin
/assign @rjbez17
/cc @GinnyJI

adrianludwin
adrianludwin reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

This is my review for the first commit only. Overall I really like it; I've left some comments that could simplify it even further (and possibly identifies one bug).

incubator/hnc/internal/forest/namespaceobjects.go Outdated Show resolved Hide resolved
incubator/hnc/internal/forest/namespaceobjects.go Outdated Show resolved Hide resolved
incubator/hnc/internal/reconcilers/object.go Outdated Show resolved Hide resolved
incubator/hnc/internal/reconcilers/object.go Outdated Show resolved Hide resolved
adrianludwin
adrianludwin reviewed Sep 17, 2020
View reviewed changes
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig_test.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig_test.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig_test.go Outdated Show resolved Hide resolved
@yiqigao217
Copy link
Contributor Author

/retest

adrianludwin
adrianludwin reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

A few comments for the first commit - will come back and finish in a bit (just need to take a break)

incubator/hnc/internal/forest/namespaceobjects.go Outdated Show resolved Hide resolved
incubator/hnc/internal/reconcilers/object.go Show resolved Hide resolved
incubator/hnc/internal/reconcilers/object.go Outdated Show resolved Hide resolved
adrianludwin
adrianludwin reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Ok I'm now fully finished with the first commit :) On to the second commit

incubator/hnc/internal/forest/namespaceobjects.go Show resolved Hide resolved
incubator/hnc/internal/validators/hierarchy.go Show resolved Hide resolved
adrianludwin
adrianludwin reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Second commit lgtm apart from some relatively minor suggestions

incubator/hnc/internal/forest/forest.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Outdated Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig.go Show resolved Hide resolved
incubator/hnc/internal/validators/hncconfig_test.go Show resolved Hide resolved
@yiqigao217
Refactor forest sourceObjects and update object reconciler
99e96a4 
Refactor the `forest.namespace.sourceObjects` field to not only store
source objects with `Propagate` mode, but all the source objects as long
as there once is an object reconciler created for the type.

This changed field will provide data for handling source object name
conflicts even for types changing mode from others to `Propagate`. In
addition, since we will implement HNC exceptions soon, we will not simply
rely on whether a source object exists in this field, but will get all
the info on the fly from the instance itself to decide whether to
propagate or enqueue objects.

Update object reonciler dependencies on the old `originalObjects` field
with only source objects with `Propagate` mode. Check if the object
`shouldPropagate()` on the fly. Remove the check of whether the source
object is updated to ensure a mode change from `Remove` to `Propagate`
still enqueues an unchanged source's descendants for them to be created.
Note this will increase the number of enqueued objects but it should be
fine since we only write if there's a diff and read is fast from cache.

Update object reconciler to still not deleting a descendant object with
the same name as in an ancestor for types with 'Remove' mode. Add a test
case for that. The test failed before and passed after the change.

Consider there's an object overwriting conflict only if the conflicting
source in the ancestor can propagate, e.g. with no finalizers. Add a new
test case.

Tested by `make test`.
@yiqigao217
Update hncconfig webhook to avoid overwriting source
0b355cb 
Add new hncconfig webhook rules and new test cases. Check if changing
type mode(s) to `Propagate` would cause overwriting source by looking
up the `objectNames` data. Deny the request and output all the conflicts
of one type change if it would cause overwriting.

Tested by "make test" and manually on a GKE cluster with multiple
objects with the same name in the same tree or different trees.

The webhook message would look like this:
Cannot update configuration because setting type "/v1, Kind=Secret" to 'Propagate' mode would overwrite user-created object(s):
  Object "my-creds2" in namespace "acme-org" would overwrite the one in "team-a"
  Object "my-creds2" in namespace "acme-org" would overwrite the one in "team-b"
  Object "my-creds" in namespace "bcme-org" would overwrite the one in "team-c"
  Object "my-creds2" in namespace "bcme-org" would overwrite the one in "team-c"
To fix this, please rename or remove the conflicting objects first.
adrianludwin
adrianludwin reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold
/assign @rjbez17

Hey Ryan, this is a big PR with two commits. I'd recommend reviewing them individually:

  1. Commit # 1 is about always saving all source objects in the forest, even if we're not propagating objects. This enables...
  2. Commit # 2 which checks the forest for objects that are about to be overwritten if we change the mode to Propagate.

Note that # 2 will only have an effect if the reconciler is currently not in the Ignore mode for that type of object - which it typically will be. A future change will guide users to first change to the Remove mode, and only later change to the Propagate mode.

incubator/hnc/internal/validators/hncconfig_test.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, yiqigao217

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020
@yiqigao217
Copy link
Contributor Author

Update: Passed all e2e tests.

Initially I got:

Summarizing 3 Failures:

1.[Fail] Demo [It] Should test basic functionalities in demo 
/usr/local/google/home/yiqigao/go/src/sigs.k8s.io/multi-tenancy/incubator/hnc/pkg/testutils/testutils.go:110

2. [Fail] HNC should delete and create a new Rolebinding instead of updating it [It] Should delete and create a Rolebinding when HNC is undeployed - issue #798 
/usr/local/google/home/yiqigao/go/src/sigs.k8s.io/multi-tenancy/incubator/hnc/pkg/testutils/testutils.go:110

3. [Fail] When deleting CRDs [It] should not delete subnamespaces 
/usr/local/google/home/yiqigao/go/src/sigs.k8s.io/multi-tenancy/incubator/hnc/pkg/testutils/testutils.go:69

Ran 29 of 29 Specs in 1030.354 seconds
FAIL! -- 26 Passed | 3 Failed | 0 Pending | 0 Skipped

1 & 2 passed when I ran it manually. 3 passed by itself if it's a stand-alone test (with Fit).

The 1st demo failure also failed on master. I suspect there's a bug in the test that it should use RoleBinding instead of Role here: https://github.com/kubernetes-sigs/multi-tenancy/blob/f990be3264cac6edb7618880b859522d25d40989/incubator/hnc/test/e2e/demo_test.go#L55

/assign @adrianludwin
/cc @GinnyJI

@adrianludwin
Copy link
Contributor

Yes, @GinnyJI is looking into the e2e test failures on master. I'm comfortable that this change is good and doesn't break anything.

@rjbez17
Copy link

rjbez17 commented Sep 23, 2020

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2020
@k8s-ci-robot k8s-ci-robot merged commit 87da270 into kubernetes-retired:master Sep 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adrianludwin adrianludwin adrianludwin left review comments

@rjbez17 rjbez17 Awaiting requested review from rjbez17

@GinnyJI GinnyJI Awaiting requested review from GinnyJI

Assignees

@rjbez17 rjbez17

@adrianludwin adrianludwin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yiqigao217 @k8s-ci-robot @adrianludwin @rjbez17