First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175

gfichtenholt · 2022-01-27T07:58:00Z

This is the first step toward implementing flux plugin: switch to using flux native data types instead of unstructured #4112 . In this PR I am using Flux's native data types up until the point I need to interact with k8s API server, which is done via dynamic.Interface so requires me to convert to/from unstructured. This should bring flux plug-in up-to parity with kapp-controller plugin in terms of handling native types and conversion to/from unstructured.

Next step for me will be getting rid of the dependency on dynamic.Interface probably in favor of https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/client#Client so I don't have to convert back and forth between unstructured and typed and just pass the typed objects all the way through the entire stack.
I am a little exhausted at the moment. This first step was very arduous and pain-staking, so I would like to take a little break, switch gears for a little while and work on other outstanding issues before coming back to do the next step

I am not sure who to ask to review this. Antonio can do this, since he's done similar work in kapp-controlller plugin. Michael is welcome to review as well. I always look forward to his feedback

flux plugin: switch to using flux native data types instead of unstructured vmware-tanzu#4112

antgamdia

Thanks a lot for the effort refactoring tons of lines. Tedious, but really worthy for devs.
+1, but before merging, please take into account this comment below:

It seems you (I mean, the IDE/git/whatever) reverted back somehow the changes in the code-generator submodule. I guess playing around with git submodule update --remote or similar commands should make the trick.

I personally also use Gitkraken and this is what you should view (note it is in v1.22):

If using vscode, you can get this info in the repo view (again, note the 1.22.6 branch)

So, please, ensure your PR is not changing the code under code-generator. Otherwise, we will be changing alternatively those files in each PR (mine in 1.22, yours in 1.18, back and forth :P)

antgamdia · 2022-01-28T10:11:34Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/watcher_cache.go

 }

 // this is effectively a cache SET operation
-func (c *NamespacedResourceWatcherCache) onAddOrModify(checkOldValue bool, unstructuredObj map[string]interface{}) (err error) {
+func (c *NamespacedResourceWatcherCache) onAddOrModify(checkOldValue bool, unstructuredObj unstructured.Unstructured) (err error) {


Looking forward to Go 1.18 and start using generics

antgamdia · 2022-01-28T10:24:12Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils.go

+	}
+}
+
+func NamespacedNameForUnstructured(unstructuredObj map[string]interface{}) (*types.NamespacedName, error) {


I wonder if using the built-in decoder in k8s UniversalDecoder() + DecodeInto(...) could simplify the logic [1]. I mean, I feel doing things like foo, found, err := unstructured.NestedString(unstructuredObj, "my", "path", "to, "foo") is not the best approach, but I don't have a more precise suggestion atm.

[1] https://stackoverflow.com/a/63338697

I wouldn't worry about it. This function is only called once in the entire code and even this 1 usage will go away (and the function foo), when I am done with the next step

antgamdia · 2022-01-28T10:35:11Z

cmd/kubeapps-apis/plugins/pkg/clientgetter/clientgetter.go

+// TODO: (gfichtenholt) we might want to consider some kind of
+// type Clients struct {
+//   kubernetes.Interface
+//   dynamic.Interface
+//   apiext.Interface
+// }


Yep, good idea. As we continue to add more and more clients, it's becoming quite misleading just to rely on the number of returned items (especially because of the differences between every plugin).

antgamdia · 2022-01-28T10:36:02Z

cmd/kubeapps-apis/plugins/pkg/clientgetter/clientgetter.go

@@ -106,7 +117,7 @@ func NewBackgroundClientGetter() ClientGetterWithApiExtFunc {
 func clientGetterHelper(config *rest.Config) (kubernetes.Interface, dynamic.Interface, apiext.Interface, error) {
 	typedClient, err := kubernetes.NewForConfig(config)
 	if err != nil {
-		return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client : %v", err)
+		return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client dur to: %v", err)


typo?

Suggested change

return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client dur to: %v", err)

return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client due to: %v", err)

oops, will fix

antgamdia · 2022-01-28T10:38:12Z

go.mod

+	k8s.io/api v0.23.0
+	k8s.io/apiextensions-apiserver v0.23.1
+	k8s.io/apimachinery v0.23.1
 	k8s.io/cli-runtime v0.22.6
-	k8s.io/client-go v0.22.6
+	k8s.io/client-go v0.23.0


As I mentioned in one of my PRs, it is just an automatic update when running go mod tidy (or similar) commands. However, given that we have fixed the version to 0.22.6 on the replace section, the effective dependency is not being upgraded though.

not sure if you're asking me to do something here?

antgamdia · 2022-01-28T10:53:42Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go

@@ -353,123 +377,121 @@ func (s *Server) newRelease(ctx context.Context, packageRef *corev1.AvailablePac
 	var values map[string]interface{}
 	if valuesString != "" {
 		values = make(map[string]interface{})
-		err = yaml.Unmarshal([]byte(valuesString), &values)
-		if err != nil {
+		if err = json.Unmarshal([]byte(valuesString), &values); err != nil {


Just curious, why did you replace the yaml dep with the json one here (and in updateRelease(...))

Don't remember exact reason. It's been a few days. This business with "Values" took especially long due to how Flux expects it (some kind of v1.JSON) and how we handle it. So somewhere in the (very long debugging process) I arrived at this. Possibly, I saw when I was working on this "values" business that it was JSON I was unmarshaling not YAML and to use encoding/json was clearer and it also happened to work :-)

antgamdia · 2022-01-28T10:54:30Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go

+func (s *Server) newFluxHelmRelease(chart *models.Chart, targetName types.NamespacedName, versionRef *corev1.VersionReference, reconcile *corev1.ReconciliationOptions, values map[string]interface{}) (*helmv2.HelmRelease, error) {
+	fluxRelease := &helmv2.HelmRelease{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       helmv2.HelmReleaseKind,
+			APIVersion: helmv2.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      targetName.Name,
+			Namespace: targetName.Namespace,
+		},
+		Spec: helmv2.HelmReleaseSpec{
+			Chart: helmv2.HelmChartTemplate{
+				Spec: helmv2.HelmChartTemplateSpec{
+					Chart: chart.Name,
+					SourceRef: helmv2.CrossNamespaceObjectReference{
+						Name:      chart.Repo.Name,
+						Kind:      sourcev1.HelmRepositoryKind,
+						Namespace: chart.Repo.Namespace,


Excelsior! Way much better than before

gfichtenholt · 2022-01-28T17:53:07Z

Thanks a lot for the effort refactoring tons of lines. Tedious, but really worthy for devs. +1, but before merging, please take into account this comment below:

Thanks Antonio

It seems you (I mean, the IDE/git/whatever) reverted back somehow the changes in the code-generator submodule. I guess playing around with git submodule update --remote or similar commands should make the trick.

argh, I haven't had time to figure out why this keeps on happening, but I will make sure this PR doesn't have these changes

…erator from my PR

gfichtenholt · 2022-01-28T18:18:27Z

Thanks a lot for the effort refactoring tons of lines. Tedious, but really worthy for devs. +1, but before merging, please take into account this comment below:

Thanks Antonio

It seems you (I mean, the IDE/git/whatever) reverted back somehow the changes in the code-generator submodule. I guess playing around with git submodule update --remote or similar commands should make the trick.

argh, I haven't had time to figure out why this keeps on happening, but I will make sure this PR doesn't have these changes

Update: almost got it. I tried a couple of commands, including the one you sent and well as https://stackoverflow.com/questions/39459467/remove-a-modified-file-from-pull-request

and it got rid of everything except 1 file:

Any advice how to remove this from my PR would be appreciated

…s.io/code-generator

…ers to it without breaking existing code

gfichtenholt added 2 commits January 26, 2022 23:34

first step towards issue

74b3744

flux plugin: switch to using flux native data types instead of unstructured vmware-tanzu#4112

Merge branch 'main' into further-fluxv2-plugin-features-21

990e77e

gfichtenholt self-assigned this Jan 27, 2022

gfichtenholt added this to In progress in Kubeapps via automation Jan 27, 2022

gfichtenholt linked an issue Jan 27, 2022 that may be closed by this pull request

flux plugin: switch to using flux native data types instead of unstructured #4112

Closed

gfichtenholt added 2 commits January 27, 2022 01:07

fix merge issues

175fcf0

minor fixes

ba47105

gfichtenholt requested review from absoludity and antgamdia January 27, 2022 10:21

antgamdia approved these changes Jan 28, 2022

View reviewed changes

antgamdia mentioned this pull request Jan 28, 2022

Migrate from go yaml.v2 to yaml.v3 #4178

Merged

gfichtenholt added 4 commits January 28, 2022 09:58

attempt to remove cmd/apprepository-controller/vendor/k8s.io/code-gen…

3b07e45

…erator from my PR

attempt #2

d507e69

Merge branch 'main' of github.com:kubeapps/kubeapps

b8a929d

Merge branch 'main' into further-fluxv2-plugin-features-21

3312373

gfichtenholt added 6 commits January 28, 2022 10:20

fix go.sum

18cc4e1

still fixing go.mod and go.sum after merge from upstream main

c21e73f

attempt to get the latest from cmd/apprepository-controller/vendor/k8…

23c583e

…s.io/code-generator

revert last change: didn't work

86a559d

Antonio's feedback

b2b3ffc

refactored clientgetter so that we can add more different client gett…

8dd43ec

…ers to it without breaking existing code

gfichtenholt merged commit 1c23836 into vmware-tanzu:main Jan 31, 2022

Kubeapps automation moved this from In progress to Done Jan 31, 2022

gfichtenholt deleted the further-fluxv2-plugin-features-21 branch January 31, 2022 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175

First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175

gfichtenholt commented Jan 27, 2022 •

edited

antgamdia left a comment

antgamdia Jan 28, 2022

antgamdia Jan 28, 2022

gfichtenholt Jan 28, 2022

antgamdia Jan 28, 2022

antgamdia Jan 28, 2022

gfichtenholt Jan 28, 2022

antgamdia Jan 28, 2022

gfichtenholt Jan 28, 2022

antgamdia Jan 28, 2022

gfichtenholt Jan 28, 2022

antgamdia Jan 28, 2022

gfichtenholt commented Jan 28, 2022

gfichtenholt commented Jan 28, 2022

	return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client dur to: %v", err)
	return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "unable to get typed client due to: %v", err)

First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175

First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175

Conversation

gfichtenholt commented Jan 27, 2022 • edited

antgamdia left a 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

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

gfichtenholt commented Jan 28, 2022

gfichtenholt commented Jan 28, 2022

gfichtenholt commented Jan 27, 2022 •

edited