Add `FieldExport` CRD and field export reconciler #75

RedbackThomson · 2022-02-25T22:52:12Z

Issue #, if available: aws-controllers-k8s/community#740

Description of changes:
Creates a new FieldExport CRD as outlined this proposal. Also creates a new reconciler that reconciles changes to FieldExport and changes to any ACK resource recognised by the current controller.

The path for reconciling a FieldExport:

Ensure the FieldExport Source resource group matches the current controller's group
Mark as managed (add finalizer)
Describe the source object
3a. If no source object is found, stop reconciling
Use the gojq library to parse the unstructured source object
Write to ConfigMap/Secret

The path when any ACK resource is updated:

Ensure the resource has the synced condition set to true
List and filter all FieldExport types in the namespace that reference the current resource as its Source
For each matching FieldExport do steps 3 and 4 from the previous path

The reconciler converts all primitive types into strings (ConfigMap is a string-string map, Secret is a string-byte[] map). If the jq query returns a list of results, we will only take the first result (this PR does not support exporting slices/maps). If the jq query returns a struct, we will not take any action, treating the query as a failure.

The reconciler will not delete any values from the ConfigMap or Secret. If the referenced field is removed from the object, the reconciler will no-op, rather than overwriting the data with an empty string. When the FieldExport is deleted, the ConfigMap or Secret will also be left untouched - meaning it contains whatever was last written to it.

Upon creating a FieldExport CR, the reconciler will initially attempt to export the field. If it fails, it will not attempt to retry. After the first sync, the reconciler will only wait until the ResourceVersion of the source object changes for it to trigger a new update.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jaypipes

Looking really good, @RedbackThomson! A few minor suggestions inline along with one major comment about the handling of Recoverable conditions...

jaypipes · 2022-03-02T16:43:30Z

apis/core/v1alpha1/identifiers.go

+// NamespacedTargetKubernetesResource provides all the values necessary to identify an ACK
+// resource of a given type (within the same namespace).
+type NamespacedTargetKubernetesResource struct {
+	metav1.GroupKind `json:""`


I think you should leave off the json:"" if you want to "inline" the GroupKind's fields.

Other way around. Including the json tag inlines it.

Not according to this: https://stackoverflow.com/questions/43077665/how-do-i-inline-fields-when-marshalling-json

Not sure what to say haha. The AdoptedResource CRD remains the same if I do it this way - you can check the diff to verify.

apis/core/v1alpha1/identifiers.go

jaypipes · 2022-03-02T16:45:50Z

apis/core/v1alpha1/identifiers.go

+}
+
+// FieldExportOutputSelector provides the values necessary to identify the
+// output path for a field export (within the same namespace).


If this is "(within the same namespace)", then a) the name of the struct should begin with Namespaced to align with the other namespace-scoped structs above like NamespacedTargetKubernetesResource and b) why does this struct have an optional Namespace field?

Yeah this comment is incorrect. The output can be into a separate namespace. The default is the same namespace (which is why namespace is optional).

jaypipes · 2022-03-02T16:47:04Z

apis/core/v1alpha1/adopted_resource.go

 type AdoptedResourceSpec struct {
 	// +kubebuilder:validation:Required
-	Kubernetes *TargetKubernetesResource `json:"kubernetes"`
+	Kubernetes *AdoptedResourceTarget `json:"kubernetes"`


this is going to break any existing ACK controller, no?

It's just been renamed. The only thing that changes are the docstrings

Existing AdoptedResource CRs in an existing installation of a controller would fail to be read into the controller, no? Since the over-wire format of the schema would be different...

Protobuf does not care about the field names. As long as I don't mess with the tag numbers, which I believe are auto-generated based on the order of the field, then I should be okay. Citation: https://stackoverflow.com/questions/45431685/protocol-buffer-does-changing-field-name-break-the-message

Protobuf does not care about the field names. As long as I don't mess with the tag numbers, which I believe are auto-generated based on the order of the field, then I should be okay. Citation: https://stackoverflow.com/questions/45431685/protocol-buffer-does-changing-field-name-break-the-message

You aren't changing the name of the field above, though... You're changing the data type.

I reached out to Ellis about this:

[Nick] I feel that if the CRD is identical, there shouldn't be a difference?
[Ellis] The API Server serializes it using the CRD
[Ellis] Exactly
[Ellis] The APIServer isn't aware of your go struct, just the CRD
[Ellis] We do this regularly in karpenter w/ our go types

So we don't need to worry about etcd serialization. Just that we are maintaining the same contract with the API server, which is the CRD. I have confirmed that the CRD remains untouched.

go.mod

pkg/runtime/field_export_reconciler.go

pkg/runtime/field_export_reconciler_test.go

pkg/runtime/log/resource.go

vijtrip2

Excellent work! Left some minor comments.

apis/core/v1alpha1/field_export.go

pkg/runtime/log/resource.go

pkg/runtime/field_export_reconciler.go

vijtrip2 · 2022-03-08T21:49:05Z

pkg/runtime/field_export_reconciler.go

 	from acktypes.AWSResource,
 	desired ackv1alpha1.FieldExport,
 ) error {
+	r.clearConditions(ctx, &desired)


Minor Suggestion: This will add extra patchStatus call to API server. I think conditions can be handled without this extra call by keeping a copy of desired in memory, updating that copy and then patch once at the end using desired as base.

The approach may look like:
a) Create latest by deepCopying desired.
b) clearConditions on latest in memory (do not patch)
c) Use latest inside the Sync function and set Apt conditions (Recoverable, Terminal, etc) in memory
d) PatchStatus at the very end, using desired as "base" .

The above approach will only make single call to APIServer for status patching. Currently there is one call for clearing condition and another per condition.

pkg/runtime/field_export_reconciler.go

pkg/types/field_export_reconciler.go

jaypipes

Getting there, @RedbackThomson thank you! Left a number of suggestions inline for ya.

jaypipes · 2022-03-10T16:43:46Z

apis/core/v1alpha1/adopted_resource.go

 type AdoptedResourceSpec struct {
 	// +kubebuilder:validation:Required
-	Kubernetes *TargetKubernetesResource `json:"kubernetes"`
+	Kubernetes *AdoptedResourceTarget `json:"kubernetes"`


Protobuf does not care about the field names. As long as I don't mess with the tag numbers, which I believe are auto-generated based on the order of the field, then I should be okay. Citation: https://stackoverflow.com/questions/45431685/protocol-buffer-does-changing-field-name-break-the-message

You aren't changing the name of the field above, though... You're changing the data type.

apis/core/v1alpha1/identifiers.go

pkg/requeue/requeue.go

pkg/runtime/field_export_reconciler.go

pkg/runtime/service_controller.go

pkg/types/field_export_reconciler.go

pkg/errors/field_export.go

pkg/runtime/field_export_reconciler.go

jaypipes

@RedbackThomson the whole fieldExportReconciler single reconciler struct for both source resource and FieldExport resources is still rubbing me the wrong way. Please see my comment inline about having two reconciler structs, one for FieldExports and another for source resources.

pkg/runtime/field_export_reconciler.go

jaypipes · 2022-03-16T14:53:20Z

pkg/runtime/field_export_reconciler.go

+	// Handle query errors
+	if err, ok := result.(error); ok {
+		if err != nil {
+			return nil, &terminalError{err: errors.Wrap(err, ackerr.FieldExportQueryFailed.Error())}


why not put terminalError into pkg/errors and actually define pkg/errors.FieldExportQueryFailed as wrapping terminalError?

pkg/runtime/field_export_reconciler.go

jaypipes

There's enough good here that I'm willing to address a few things about the fieldExportReconciler struct in followup PRs. 👍

jaypipes · 2022-03-18T21:02:55Z

/lgtm

ack-bot · 2022-03-18T21:03:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, RedbackThomson, vijtrip2

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:

~~OWNERS~~ [RedbackThomson,jaypipes,vijtrip2]

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

acornett21 · 2022-03-18T21:19:42Z

@RedbackThomson @jaypipes Sorry I'm late on this one, but is this a CRD that would be packaged with every controller just like the AdoptedResource correct? If so then mitigation work in the runtime and in the code-generator should be accounted for to not package this CRD. If I'm miss understanding this apologies.

RedbackThomson · 2022-03-18T21:21:57Z

@RedbackThomson @jaypipes Sorry I'm late on this one, but is this a CRD that would be packaged with every controller just like the AdoptedResource correct? If so then mitigation work in the runtime and in the code-generator should be accounted for to not package this CRD. If I'm miss understanding this apologies.

Correct. This CRD is handled exactly in the same way as AdoptedResource. The mitigation work we put in for OLM to not package it, I think we did that for the common CRD directory? Hopefully that code already removes this new CRD as well.

acornett21 · 2022-03-18T21:49:13Z

@RedbackThomson Looking again at this PR and the code we have here I think adding this would be covered by whats in place. Can you please take a look and confirm? Thanks!

RedbackThomson · 2022-03-18T23:26:55Z

@RedbackThomson Looking again at this PR and the code we have here I think adding this would be covered by whats in place. Can you please take a look and confirm? Thanks!

Exactly! I tried to future proof it, and I think this still holds.

acornett21 · 2022-03-21T14:59:29Z

Yeah, It looks that way, missed it on the first pass of looking at this PR.

ack-bot added the approved label Feb 25, 2022

ack-bot requested review from a-hilaly and jaypipes February 25, 2022 22:52

jaypipes requested changes Mar 2, 2022

View reviewed changes

vijtrip2 reviewed Mar 7, 2022

View reviewed changes

vijtrip2 reviewed Mar 8, 2022

View reviewed changes

pkg/runtime/field_export_reconciler.go Outdated Show resolved Hide resolved

vijtrip2 reviewed Mar 8, 2022

View reviewed changes

pkg/runtime/field_export_reconciler.go Outdated Show resolved Hide resolved

vijtrip2 reviewed Mar 8, 2022

View reviewed changes

vijtrip2 approved these changes Mar 9, 2022

View reviewed changes

pkg/runtime/field_export_reconciler.go Show resolved Hide resolved

pkg/runtime/field_export_reconciler.go Outdated Show resolved Hide resolved

pkg/types/field_export_reconciler.go Show resolved Hide resolved

vijtrip2 approved these changes Mar 9, 2022

View reviewed changes

jaypipes requested changes Mar 10, 2022

View reviewed changes

jaypipes reviewed Mar 10, 2022

View reviewed changes

pkg/types/field_export_reconciler.go Outdated Show resolved Hide resolved

jaypipes reviewed Mar 11, 2022

View reviewed changes

pkg/errors/field_export.go Show resolved Hide resolved

pkg/runtime/field_export_reconciler.go Show resolved Hide resolved

pkg/runtime/field_export_reconciler.go Show resolved Hide resolved

jaypipes requested changes Mar 16, 2022

View reviewed changes

Nicholas Thomson added 16 commits March 16, 2022 11:02

Create FieldExport CRD

6401fc1

Set up structure of field export reconciler

7babbaf

Set up field export reconciler structure

49d3c82

Add jq field path selector

7860a1c

Add mock

1ea062c

Change destination type to kind

4ee99ef

Make export type into enum

a47ed79

Move output enum validation onto type

eeae844

Add export to config map

787b505

Support only string values

7cdf724

Add support for writing secrets

556af44

Add resource update trigger

26bb76d

Add initial unit tests and mocks

f1a3acc

Added secret output tests

49efd66

Add export filter tests

50b03db

Fix mark managed after finding target group

e76c1d4

Nicholas Thomson added 13 commits March 16, 2022 11:02

Add gojq attribution

1b02824

Update minor code refactoring

4e14557

Fix failing unit tests

bbe7cb4

Separate create and bind for reconciler types

841db02

Update error handling and deletion protection

2efb0e5

Introduce terminal and norequeue error wrappers

cb1441e

Fix outdated comments

7435fe6

Update status patching to only occur once

daafc2a

Remove ensureConditions method

52392e2

Update comments and type names

743105f

Export errors into err package

442e616

Rely on nillable descriptor for switching

97aeb53

Use separate reconciler for ACK resources

bfa7ba4

RedbackThomson force-pushed the field-exporter branch from 45a7832 to bfa7ba4 Compare March 16, 2022 18:04

Rebuild mocks and fix unit test

ea3f327

jaypipes approved these changes Mar 18, 2022

View reviewed changes

ack-bot assigned jaypipes Mar 18, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2022

ack-bot merged commit 4c4d5cb into aws-controllers-k8s:main Mar 18, 2022

RedbackThomson deleted the field-exporter branch March 18, 2022 21:03

This was referenced Mar 22, 2022

K8s native binding design proposal aws-controllers-k8s/community#1183

Merged

Output fields consumable by pods, config maps, etc. aws-controllers-k8s/community#740

Closed

What to do with the data plane? aws-controllers-k8s/community#292

Closed

Add FieldExport CRD and field export reconciler #75

Add FieldExport CRD and field export reconciler #75

Uh oh!

Conversation

RedbackThomson commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vijtrip2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add `FieldExport` CRD and field export reconciler #75

Add `FieldExport` CRD and field export reconciler #75

RedbackThomson commented Feb 25, 2022 •

edited

Loading

RedbackThomson Mar 2, 2022 •

edited

Loading