fix(controller): Tolerate malformed resources. Fixes #3677 #3680
Conversation
test/e2e/fixtures/when.go
Outdated
| start := time.Now() | ||
| logCtx := log.WithFields(log.Fields{"workflow": workflowName, "condition": condition, "timeout": timeout}) | ||
|
|
||
| fieldSelector := "" |
There was a problem hiding this comment.
This changes the behaviour of this func to make it more useful, you no longer need to know the name of the workflow you want to wait for. We alternatively wait for any argo-e2e workflow.
|
|
||
| func (s *MalformedResourcesSuite) TestMalformedWorkflowTemplate() { | ||
| s.Given(). | ||
| Exec("kubectl", []string{"apply", "-f", "testdata/malformed/malformed-workflowtemplate.yaml"}, fixtures.NoError). |
There was a problem hiding this comment.
I need to add a workflow that uses a malformed workflow template here
| var NoError = func(t *testing.T, output string, err error) { | ||
| assert.NoError(t, err, output) | ||
| } |
| } | ||
|
|
||
| // a drop-in replacement for `extwfv1.ClusterWorkflowTemplateInformer` that ignores malformed resources | ||
| func NewTolerantClusterWorkflowTemplateInformer(dynamicInterface dynamic.Interface, defaultResync time.Duration) extwfv1.ClusterWorkflowTemplateInformer { |
There was a problem hiding this comment.
Not sure what makes this "tolerant". Can you explain a bit?
| @@ -0,0 +1,28 @@ | |||
| package informer | |||
There was a problem hiding this comment.
This file name ends in .co.go. Typo?
| @@ -39,9 +44,10 @@ type Controller struct { | |||
| wfInformer cache.SharedIndexInformer | |||
| wfLister util.WorkflowLister | |||
| wfQueue workqueue.RateLimitingInterface | |||
| cronWfInformer extv1alpha1.CronWorkflowInformer | |||
| cronWfInformer informers.GenericInformer | |||
There was a problem hiding this comment.
The generated informers we currently use get "stuck" if a malformed resource in created. By "malformed" I mean that while they are correctly formatted as JSON or YAML, they cannot be the resource kind, e.g. because they have an array where an object is expected. This stops the inform working: the informer sits there repeatedly printing an error.
By using a dynamic informer (which is effectively the same as as an unstructured informer) we remove this problem and we control the parsing into the required type.
| return &Controller{ | ||
| wfClientset: wfclientset, | ||
| namespace: namespace, | ||
| managedNamespace: managedNamespace, | ||
| instanceId: instanceId, | ||
| cron: cron.New(), | ||
| restConfig: restConfig, | ||
| dynamicInterface: dynamicInterface, |
There was a problem hiding this comment.
for creating a generic informer
| Then(). | ||
| ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { | ||
| assert.Equal(t, "wellformed", metadata.Name) | ||
| assert.Equal(t, wfv1.NodeError, status.Phase) |
There was a problem hiding this comment.
we should also check the message makes sense to the user
| Then(). | ||
| ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { | ||
| assert.Equal(t, "wellformed", metadata.Name) | ||
| assert.Equal(t, wfv1.NodeError, status.Phase) |
There was a problem hiding this comment.
we should also check the message makes sense to the user
Checklist:
"fix(controller): Updates such and such. Fixes #1234".Fixes #3677
Summary
This PR replaces the standard Kubernetes informer with a dynamic informer, where the code ignores malformed resources.
Changes
The approach differs slightly for workflow templates and cron workflows. Cron is simpler to directly replace the informer with our own informer, keeping the changes simpler. With workflow templates, the code wraps the informers in a "tolerant" translation layer that ignores malformed resources for list operations.
Testing
This is tested using e2e tests.