Refactor controller logic for getting RESTConfig to a remote cluster #3712

justinsb · 2023-01-05T00:15:52Z

We had two copies, rationalize and take the best of each.

Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters.

justinsb · 2023-01-05T16:19:25Z

/retest

I don't think this is related, though it is a pod-evaluator ~~bug~~ test failure...

mortent · 2023-01-05T16:50:19Z

Yeah, we have seen some flakes with the pod-evaluator test. I'm triggering another run.

mortent · 2023-01-05T20:43:25Z

porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go

@@ -202,30 +191,6 @@ func (r *RootSyncSetReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	return ctrl.Result{}, nil
 }

-func toCanonicalClusterRef(ref *v1alpha1.ClusterRef, rssNamespace string) (v1alpha1.ClusterRef, error) {


Are we making all four properties on the clusterRef mandatory in the API here? We use the clusterRef as the key in maps in this controller.

I wasn't sure here. I figured it would still work (?), it might just miss some potential to share caches. I would like to get this logic into kubebuilder-declarative-pattern - there are a few PRs in flight that support the idea of applying to a remote cluster that we could then just drop in. I'll try to get those merge-ready today!

I did start introducing a string-clusterKey that e.g. would identify unique clusters based on the URL, but then I spotted that we're not pruning with the canonicalClusterRef (r.pruneWatches(req.NamespacedName, rootsyncset.Spec.ClusterRefs)), so I figured the change here was probably more correct anyway?

Happy to upload the clusterKey WIP branch, but I would rather get this into a shared and smaller library, as I think it's useful everywhere and I also think it's quite tricky code and could benefit from being implemented in a minimal standalone library!

Moving the logic into a separate library like kubebuilder-declarative-pattern sounds good. The idea behind this was to avoid having to reason about incomplete clusterRefs, we should just make sure they are complete at all times. If we put it back, I can take a pass at simplifying it and make sure it doesn't break the functionality.

mortent · 2023-01-05T20:45:47Z

porch/controllers/remoterootsyncsets/pkg/remoteclient/getremoteclient.go

+	configControllerApiVersion = "configcontroller.cnrm.cloud.google.com/v1beta1"
+)
+
+type RemoteClientGetter struct {


We should figure out a place to put code that is shared between the controllers. It seems a bit strange to have this code under the remoterootsyncset controller when it is pretty generic and also used by the RootSyncSet controller.

FWIW, in the rollouts branch, I refactored this into a separate package clusterstore to provides a way to list clusters, get clients (dynamic and client.Client) given a cluster. Eventually the plan was clusterstore could be an abstraction that can work with KCC clusters, hub clusters etc.

https://github.com/GoogleContainerTools/kpt/tree/feature-rollouts/rollouts/pkg/clusterstore

Yes, agreed. I was planning to do that as a follow-on. I was even wondering if we should keep porch as a separate module, as it's a (minor) barrier to sharing with kpt to have to bump the versions. Although that's less relevant for this logic, which is very server-side!

I like pkg/clusterstore :-)

Yeah, I'm fine with this in a follow-up. I think we want to keep the modules separate and instead look into how we can make it easier to work with. At some point we will need to look into separate some of these components, for example splitting out some of the controllers that perhaps isn't tied to Porch.

mortent · 2023-01-05T20:46:26Z

porch/controllers/remoterootsyncsets/pkg/remoteclient/getremoteclient.go

+	if err != nil {
+		return nil, err
+	}
+	// accessToken, err := GetGcloudAccessToken(ctx)


Should we just delete this?

Good call - done!

justinsb · 2023-01-06T20:06:11Z

/retest

Test failure looks unrelated

mortent · 2023-01-06T22:42:23Z

porch/controllers/remoterootsyncsets/pkg/remoteclient/getremoteclient.go

+	configControllerApiVersion = "configcontroller.cnrm.cloud.google.com/v1beta1"
+)
+
+type RemoteClientGetter struct {


Yeah, I'm fine with this in a follow-up. I think we want to keep the modules separate and instead look into how we can make it easier to work with. At some point we will need to look into separate some of these components, for example splitting out some of the controllers that perhaps isn't tied to Porch.

mortent · 2023-01-06T22:46:11Z

porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go

@@ -202,30 +191,6 @@ func (r *RootSyncSetReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	return ctrl.Result{}, nil
 }

-func toCanonicalClusterRef(ref *v1alpha1.ClusterRef, rssNamespace string) (v1alpha1.ClusterRef, error) {


Moving the logic into a separate library like kubebuilder-declarative-pattern sounds good. The idea behind this was to avoid having to reason about incomplete clusterRefs, we should just make sure they are complete at all times. If we put it back, I can take a pass at simplifying it and make sure it doesn't break the functionality.

We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters.

justinsb · 2023-01-11T13:03:07Z

If we put it back, I can take a pass at simplifying it and make sure it doesn't break the functionality.

I wasn't entirely sure whether you were OK with merging this as-is? I think it's more correct than the previous logic, which was inconsistent in creation vs deletion, but I might be missing something!

* porch: don't save empty patches (#3695) * docs: fixes for some minor documentation typos (#3699) * docs: Update the kpt book with more details about namespaces and RBAC for porch (#3692) * Log enabled controllers and warn if no controllers are enabled (#3710) Because the default is to enable no controllers, it is easy to mistakenly start a no-op controller. * Extract out common parse-package logic (#3711) We had this code duplicated in a few places also. * refactor pod warmup to avoid vet warning (#3713) By refactoring the parallel operation into a separate function, it should be easier to read and we avoid a loop-closure go-tcha. * Bump json5 from 2.2.0 to 2.2.3 in /site (#3717) Bumps [json5](https://github.com/json5/json5) from 2.2.0 to 2.2.3. - [Release notes](https://github.com/json5/json5/releases) - [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md) - [Commits](json5/json5@v2.2.0...v2.2.3) --- updated-dependencies: - dependency-name: json5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * tests: add more logging around problematic test timeout (#3718) Trying to figure out why this test keeps timing out. * Refactor controller logic for getting RESTConfig to a remote cluster (#3712) We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters. * add a deletion approval flow with a validation webhook (#3678) * PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy (#3701) * e2e: add delay after registering Repository (#3741) I believe this will help avoid the "failed to list resources" error immediately after registering a repository. * licensescan: fix ignore handling (#3740) The previous logic did not work correctly. * licensescan: Add licenses for more libraries. (#3736) Updating our database with the latest libraries, as needed by some other projects sharing this DB! * Docs: Updating 'Developing in Go' (#3715) * licensescan: Fix missing pipe character in README (#3739) The command is not correct without it. * RemoteRootSyncSet: able to specify a packageRef to a package (#3734) This makes it easy to apply packages we create. * chore: Upgrade cli-utils to v0.34.0 (#3746) Upgrades cli-utils to v0.34.0 which contains an upgrade to Go v1.18 and Kubernetes v1.25 resources. This PR was origininally authored by rquitales #3642 * rollouts: added top level directory * rollouts: scaffolded the project using kubebuilder (#3689) * rollouts: added cluster discovery and selection (#3696) * Rollouts package discovery (#3697) * rollouts: added remoterootsync API (#3698) * rollouts: add package cluster matcher (#3700) * rollouts: add AllAtOnce strategy (#3703) * rollouts: allow packages to be discovered from multiple repositories (#3702) * rollouts: rename packages git source to github (#3708) * rollouts: allow the root directory of a repository to be synced (#3709) * rollouts: add caching for discovered packages (#3706) * rollouts: add rolling update strategy (#3714) * rollouts: added API for ProgressiveRolloutStrategy (#3716) * rollouts: refine package to cluster matcher (#3720) * rollouts: implement progressive strategy (#3719) * rollouts: update progressive strategy to pause after wave (#3721) * rollouts: added skeleton CLI (#3724) * rollouts: added skeleton CLI * added table display * rollouts: add rollout summary status (#3725) * rollouts: tidy up go.mod/sum (#3726) * rollouts: duplicate target fix (#3727) * rollouts: sort cluster status list (#3728) * rollouts: conditionally show wave status (#3729) * rollouts: CLI now supports displaying waves and progress counts (#3730) * rollouts: cli can now advance waves on progressive rollouts (#3731) * rollouts: enable server side throttling for cli (#3732) * rollouts: add container cluster watch (#3738) * rollouts: delete remote root sync when no longer needed (#3742) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: James Brook <jbrook@google.com> Co-authored-by: Morten Torkildsen <mortent@google.com> Co-authored-by: Justin Santa Barbara <justinsb@google.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: abangser <bangser.a@gmail.com> Co-authored-by: Christopher Fry <christopherfry@google.com>

* porch: don't save empty patches (kptdev#3695) * docs: fixes for some minor documentation typos (kptdev#3699) * docs: Update the kpt book with more details about namespaces and RBAC for porch (kptdev#3692) * Log enabled controllers and warn if no controllers are enabled (kptdev#3710) Because the default is to enable no controllers, it is easy to mistakenly start a no-op controller. * Extract out common parse-package logic (kptdev#3711) We had this code duplicated in a few places also. * refactor pod warmup to avoid vet warning (kptdev#3713) By refactoring the parallel operation into a separate function, it should be easier to read and we avoid a loop-closure go-tcha. * Bump json5 from 2.2.0 to 2.2.3 in /site (kptdev#3717) Bumps [json5](https://github.com/json5/json5) from 2.2.0 to 2.2.3. - [Release notes](https://github.com/json5/json5/releases) - [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md) - [Commits](json5/json5@v2.2.0...v2.2.3) --- updated-dependencies: - dependency-name: json5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * tests: add more logging around problematic test timeout (kptdev#3718) Trying to figure out why this test keeps timing out. * Refactor controller logic for getting RESTConfig to a remote cluster (kptdev#3712) We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters. * add a deletion approval flow with a validation webhook (kptdev#3678) * PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy (kptdev#3701) * e2e: add delay after registering Repository (kptdev#3741) I believe this will help avoid the "failed to list resources" error immediately after registering a repository. * licensescan: fix ignore handling (kptdev#3740) The previous logic did not work correctly. * licensescan: Add licenses for more libraries. (kptdev#3736) Updating our database with the latest libraries, as needed by some other projects sharing this DB! * Docs: Updating 'Developing in Go' (kptdev#3715) * licensescan: Fix missing pipe character in README (kptdev#3739) The command is not correct without it. * RemoteRootSyncSet: able to specify a packageRef to a package (kptdev#3734) This makes it easy to apply packages we create. * chore: Upgrade cli-utils to v0.34.0 (kptdev#3746) Upgrades cli-utils to v0.34.0 which contains an upgrade to Go v1.18 and Kubernetes v1.25 resources. This PR was origininally authored by rquitales kptdev#3642 * rollouts: added top level directory * rollouts: scaffolded the project using kubebuilder (kptdev#3689) * rollouts: added cluster discovery and selection (kptdev#3696) * Rollouts package discovery (kptdev#3697) * rollouts: added remoterootsync API (kptdev#3698) * rollouts: add package cluster matcher (kptdev#3700) * rollouts: add AllAtOnce strategy (kptdev#3703) * rollouts: allow packages to be discovered from multiple repositories (kptdev#3702) * rollouts: rename packages git source to github (kptdev#3708) * rollouts: allow the root directory of a repository to be synced (kptdev#3709) * rollouts: add caching for discovered packages (kptdev#3706) * rollouts: add rolling update strategy (kptdev#3714) * rollouts: added API for ProgressiveRolloutStrategy (kptdev#3716) * rollouts: refine package to cluster matcher (kptdev#3720) * rollouts: implement progressive strategy (kptdev#3719) * rollouts: update progressive strategy to pause after wave (kptdev#3721) * rollouts: added skeleton CLI (kptdev#3724) * rollouts: added skeleton CLI * added table display * rollouts: add rollout summary status (kptdev#3725) * rollouts: tidy up go.mod/sum (kptdev#3726) * rollouts: duplicate target fix (kptdev#3727) * rollouts: sort cluster status list (kptdev#3728) * rollouts: conditionally show wave status (kptdev#3729) * rollouts: CLI now supports displaying waves and progress counts (kptdev#3730) * rollouts: cli can now advance waves on progressive rollouts (kptdev#3731) * rollouts: enable server side throttling for cli (kptdev#3732) * rollouts: add container cluster watch (kptdev#3738) * rollouts: delete remote root sync when no longer needed (kptdev#3742) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: James Brook <jbrook@google.com> Co-authored-by: Morten Torkildsen <mortent@google.com> Co-authored-by: Justin Santa Barbara <justinsb@google.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: abangser <bangser.a@gmail.com> Co-authored-by: Christopher Fry <christopherfry@google.com>

mortent reviewed Jan 5, 2023

View reviewed changes

justinsb force-pushed the refactor_get_client branch 2 times, most recently from de4bc0d to 8cc90f1 Compare January 6, 2023 13:54

mortent approved these changes Jan 6, 2023

View reviewed changes

Refactor controller logic for getting RESTConfig to a remote cluster

1699967

We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters.

justinsb force-pushed the refactor_get_client branch from 8cc90f1 to 1699967 Compare January 10, 2023 22:48

justinsb merged commit 9b05920 into kptdev:main Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor controller logic for getting RESTConfig to a remote cluster #3712

Refactor controller logic for getting RESTConfig to a remote cluster #3712

justinsb commented Jan 5, 2023

justinsb commented Jan 5, 2023 •

edited

mortent commented Jan 5, 2023

mortent Jan 5, 2023

justinsb Jan 6, 2023

mortent Jan 6, 2023

mortent Jan 5, 2023

droot Jan 6, 2023

justinsb Jan 6, 2023

mortent Jan 6, 2023

mortent Jan 5, 2023

justinsb Jan 6, 2023

justinsb commented Jan 6, 2023

mortent Jan 6, 2023

mortent Jan 6, 2023

justinsb commented Jan 11, 2023

Refactor controller logic for getting RESTConfig to a remote cluster #3712

Refactor controller logic for getting RESTConfig to a remote cluster #3712

Conversation

justinsb commented Jan 5, 2023

justinsb commented Jan 5, 2023 • edited

mortent commented Jan 5, 2023

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

Choose a reason for hiding this comment

justinsb commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jan 11, 2023

justinsb commented Jan 5, 2023 •

edited