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

fix: liveness check (healthz) type asserts to wrong type #12353

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Dec 12, 2023

Fixes #12326

Motivation

The liveness check was passing the WorkflowController's Workflow Informer to some code which assumes that the Informer holds types of *Workflow but in fact the Controller's informer holds types of *Unstructured. The effect was that the code would panic on the type assertion here.

Modifications

Incorporate code which lists the Workflows from the Informer using *Unstructured type.

Verification

Both through a new unit test, plus through running the Controller and purposely blocking the reconciliation code so I could hit the healthz endpoint and cause the 500 error.

Beyond this PR

…type

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

LGTM. Great catch!

@juliev0 juliev0 merged commit 51ed591 into argoproj:main Dec 13, 2023
27 checks passed
@juliev0 juliev0 deleted the healthz-informer-type branch December 13, 2023 20:57
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Jan 7, 2024
sarabala1979 pushed a commit that referenced this pull request Jan 9, 2024
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
sarabala1979 pushed a commit that referenced this pull request Jan 10, 2024
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
sarabala1979 pushed a commit that referenced this pull request Jan 13, 2024
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
)

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of Workflow Informer in healthz liveness check causes Panics due to incorrect type
3 participants