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

Generic sync/suspend/inventory listing #4096

Merged
merged 20 commits into from
Oct 27, 2023
Merged

Generic sync/suspend/inventory listing #4096

merged 20 commits into from
Oct 27, 2023

Conversation

foot
Copy link
Contributor

@foot foot commented Oct 20, 2023

Proposal to re-use some of the endpoints here for other similar resources.

This would allow "kustomization-like" resources to also be sync'd, suspended and queried for an inventory.

The "contracts":

  • You can call suspend on any registered kind that has a spec.suspend field
    • Breaking the contract will result in an error about being unable to update a missing suspend field
  • You can sync any registered kind that follows the "set-annotation -> spec.conditions is updated" flow.
    • Breaking the contract will result in a timed out sync attempt
  • You can ask for the inventory on any registered kind that has a status.inventory in the format of a kustomization's.
    • Breaking the contract will result in an error about being unable to read a missing inventory field

API behaviour changes

If another kind is registered during startup with w/ the existing Kind registration system:

coreCfg.PrimaryKinds.Add("FooKind", foov1alpha1.GroupVersion.WithKind("FooKind"))

Then as well as the GetObject/ListObjects working, the following endpoints will now additionally work with a FooKind objects:

  • GET /v1/namespaces/{namespace}/objects/{kind}/{name}/inventory
  • PATCH /v1/suspend
  • PATCH /v1/sync

Possible issues

The error handling on these endpoints will change, they will no longer respond with syncing a FooKind is not supported, but will try and "sync" the FooKind. If you provide a kind that is not supported, like a ConfigMap, it will try and sync it..

Alternatives

Expose more helper codes and use that to create very similar endpoints instead:

  • GET /v1/namespaces/{namespace}/foos/{name}/inventory
  • PATCH /v1/foos/suspend
  • PATCH /v1/foos/sync

@foot foot marked this pull request as ready for review October 23, 2023 08:43
@foot foot requested a review from bigkevmcd October 23, 2023 09:23
core/fluxsync/adapters.go Outdated Show resolved Hide resolved
core/server/inventory.go Outdated Show resolved Hide resolved
core/server/inventory.go Outdated Show resolved Hide resolved
core/server/inventory.go Outdated Show resolved Hide resolved
core/server/inventory.go Outdated Show resolved Hide resolved
return nil, fmt.Errorf("failed converting inventory entry to map[string]interface{}: %+v", entry)
}
ref := &kustomizev1.ResourceRef{}
err := runtime.DefaultUnstructuredConverter.FromUnstructured(entry, ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure why you're converting the ref from Unstructured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code here a bit to convert the entire block into a ResourceInventory and then iterate through it.

The conversion uses the kustomizations json tags etc { v, id } so you don't have to dig them out ourselves here etc was the thinking.

core/server/sync.go Outdated Show resolved Hide resolved
core/server/inventory_test.go Outdated Show resolved Hide resolved
core/fluxsync/adapters.go Show resolved Hide resolved
core/fluxsync/adapters.go Show resolved Hide resolved
foot and others added 4 commits October 23, 2023 14:20
- Which is the case that breaks sometimes w/ reflection if not handled
  carefully w/ the adpator
- Move some tests around and revert Exposed fns to be internal
- Cleanup the ToReconcileable fn interface, doesn't error anymore, we
  don't use the list value it returns
- More explicit error handling for looking up inventory
core/server/inventory.go Outdated Show resolved Hide resolved
core/server/inventory_internal_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bigkevmcd bigkevmcd left a comment

Choose a reason for hiding this comment

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

Thanks!

@foot foot merged commit bd4bb27 into main Oct 27, 2023
17 checks passed
@foot foot deleted the generic-sync-suspend-inv branch October 27, 2023 07:14
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.

None yet

2 participants