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.

Sign up for GitHub

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

Jump to bottom

workflows: use latest stable CLI in post-test information gathering #423

Merged
merged 1 commit into from
Jul 14, 2021
Merged

workflows: use latest stable CLI in post-test information gathering #423

merged 1 commit into from
Jul 14, 2021

Conversation

nbusseneau
Copy link
Member

The post-test information gathering steps were previously self-contained and became split between the actual workflow step and in-cluster jobs logs in #212.

In parallel, sysdump became a subcommand of the CLI and we switched to using it in post-test information gathering in #405.

However, we explicitly do not want to run PR-built cilium in the context of a pull_request_target-triggered privileged workflow, as it would allow an attacker to leak the repository's secrets.

Our proposal is to use the latest stable version of the CLI in the post-test information gathering step. This is acceptable as the purpose of this step is retrieving information, not testing the actual cilium commands run in the step -- this is done as part of the in-cluster jobs.

Should a workflow developer actually need to check that a PR editing sysdump or any other cilium command used in this step works as intended with the new changes, they can always switch to pull_request testing or edit the in-cluster scripts with additional testing.

@nbusseneau nbusseneau added the area/CI Continuous Integration testing issue or flake label Jul 13, 2021
@nbusseneau nbusseneau requested review from a team as code owners July 13, 2021 14:59
@nbusseneau nbusseneau requested a review from tklauser July 13, 2021 14:59
@nbusseneau nbusseneau temporarily deployed to ci July 13, 2021 15:00 Inactive
@nbusseneau nbusseneau requested a review from aanm July 13, 2021 15:00
@michi-covalent michi-covalent mentioned this pull request Jul 13, 2021
Merged
@nbusseneau nbusseneau temporarily deployed to ci July 13, 2021 15:47 Inactive
@nbusseneau
Copy link
Member Author

nbusseneau commented Jul 13, 2021 •
edited
Loading

Added a test commit with pull_request triggers and always() post-test condition to test all workflows with changes before merging.

@nbusseneau nbusseneau force-pushed the pr/workflow-use-cilium-stable-info-gathering branch from 6459793 to 3a6993a Compare July 13, 2021 16:01
@nbusseneau nbusseneau temporarily deployed to ci July 13, 2021 16:01 Inactive
tklauser
tklauser approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks!

aanm
aanm requested changes Jul 13, 2021
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

I'm probably missing something but if we are changing from pull_request_target to pull_request how will forks be able to run the conformance tests?

@nbusseneau
Copy link
Member Author

I'm probably missing something but if we are changing from pull_request_target to pull_request how will forks be able to run the conformance tests?

We are not, you're looking at a DO NOT MERGE test commit :D

@nbusseneau
Copy link
Member Author

@michi-covalent @tklauser all workflows have failed at the cilium sysdump call, hitting the workflow timeout (see how they all spend 20 minutes in the post-test gathering step). I think something's not working with cilium sysdump.

@michi-covalent
Copy link
Contributor

@michi-covalent @tklauser all workflows have failed at the cilium sysdump call, hitting the workflow timeout (see how they all spend 20 minutes in the post-test gathering step). I think something's not working with cilium sysdump.

yeah we need this commit ead8cc6 it's not release yet 🤯

@nbusseneau
Copy link
Member Author

Blocked by #428.

@nbusseneau nbusseneau added the dont-merge/blocked Another PR must be merged before this one. label Jul 13, 2021
@tklauser tklauser mentioned this pull request Jul 14, 2021
Closed
@tklauser tklauser force-pushed the pr/workflow-use-cilium-stable-info-gathering branch from 3a6993a to d1c586e Compare July 14, 2021 05:03
@tklauser tklauser temporarily deployed to ci July 14, 2021 05:04 Inactive
@tklauser
Copy link
Member

Rebased to re-trigger CI now that v0.8.5 is released.

@tklauser tklauser removed the dont-merge/blocked Another PR must be merged before this one. label Jul 14, 2021
@tklauser tklauser force-pushed the pr/workflow-use-cilium-stable-info-gathering branch from d1c586e to adfae43 Compare July 14, 2021 13:03
@tklauser tklauser temporarily deployed to ci July 14, 2021 13:03 Inactive
@tklauser
Copy link
Member

tklauser commented Jul 14, 2021 •
edited
Loading

Tests passed with ciliuim invoked correctly in collected in the "Post-test information gathering" step:

Removing the temporary test commit and merging to unblock other PRs which rely on collecting sysdumps.

@tklauser tklauser requested a review from aanm July 14, 2021 13:34
@nbusseneau @tklauser
workflows: use latest stable CLI in post-test information gathering
c429ce9 
The post-test information gathering steps were previously self-contained
and became split between the actual workflow step and in-cluster jobs
logs in #212.

In parallel, `sysdump` became a subcommand of the CLI and we switched to
using it in post-test information gathering in #405.

However, we explicitly do not want to run PR-built `cilium` in the
context of a `pull_request_target`-triggered privileged workflow, as it
would allow an attacker to leak the repository's secrets.

Our proposal is to use the latest stable version of the CLI in the post-
test information gathering step. This is acceptable as the purpose of
this step is retrieving information, not testing the actual `cilium`
commands run in the step -- this is done as part of the in-cluster jobs.

Should a workflow developer actually need to check that a PR editing
`sysdump` or any other `cilium` command used in this step works as
intended with the new changes, they can always switch to `pull_request`
testing or edit the in-cluster scripts with additional testing.

Signed-off-by: Nicolas Busseneau <[email protected]>
@tklauser tklauser force-pushed the pr/workflow-use-cilium-stable-info-gathering branch from adfae43 to c429ce9 Compare July 14, 2021 13:44
@tklauser tklauser temporarily deployed to ci July 14, 2021 13:44 Inactive
@tklauser
Copy link
Member

Rebased again to fix conflict after merging #426.

@tklauser tklauser merged commit 6abace2 into master Jul 14, 2021
@tklauser tklauser deleted the pr/workflow-use-cilium-stable-info-gathering branch July 14, 2021 14:40
@tklauser tklauser mentioned this pull request Jul 14, 2021
Merged
@nbusseneau nbusseneau mentioned this pull request Sep 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michi-covalent michi-covalent michi-covalent approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@tklauser tklauser tklauser approved these changes

@aanm aanm Awaiting requested review from aanm

Assignees

@aanm aanm

Labels
area/CI Continuous Integration testing issue or flake
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nbusseneau @michi-covalent @tklauser @aanm @jrajahalme