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

.github: Add integration tests that installs cilium and queries it using Hubble CLI #873

Merged
merged 1 commit into from Jan 31, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Jan 25, 2023

I noticed we don't have any tests that actually hit cilium, so this should fix that.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jan 25, 2023
@chancez chancez added 🧹 kind/cleanup This includes no functional changes. 🤖 area/CI Impacts the testing / continuous integration testing of the project. labels Jan 25, 2023
@chancez chancez force-pushed the pr/chancez/integration_tests branch 4 times, most recently from b5a1744 to 348ee5d Compare January 25, 2023 22:37
@chancez chancez force-pushed the pr/chancez/integration_tests branch 8 times, most recently from 2a05fdb to cdae2f3 Compare January 25, 2023 23:59
@chancez chancez marked this pull request as ready for review January 26, 2023 00:03
@chancez chancez requested review from a team as code owners January 26, 2023 00:03
@chancez chancez requested review from nbusseneau and removed request for a team January 26, 2023 00:03
@chancez chancez added the release-note/ci This PR makes changes to the CI. label Jan 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jan 26, 2023
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for doing this!

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks @chancez !

@chancez chancez force-pushed the pr/chancez/integration_tests branch 3 times, most recently from 490e054 to f1d553f Compare January 26, 2023 18:26
@chancez
Copy link
Contributor Author

chancez commented Jan 26, 2023

Hmm I can't see to get hubble to return any flows. I wonder if there's something wrong with relay and hubble in the values I'm using.

@chancez chancez force-pushed the pr/chancez/integration_tests branch 2 times, most recently from 250a51d to 0730958 Compare January 27, 2023 01:15
@kaworu kaworu added the :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. label Jan 27, 2023
@kaworu
Copy link
Member

kaworu commented Jan 27, 2023

@chancez pushed a few commits on top of yours, tested locally with act -j integration-test and the integration tests succeed on my machine, no idea why it's failing in CI though :sad-parrot:

Also added the needs-rebase label because we will want to squash the commits before merging.

@chancez chancez force-pushed the pr/chancez/integration_tests branch from a070a2c to 05c00bf Compare January 27, 2023 18:37
@chancez
Copy link
Contributor Author

chancez commented Jan 27, 2023

Added more commands to check flows at the post-test info gathering step. It seems if we query hubble status (over port-forward) at the end, the nodes are connected via relay and relay is reporting flows.

If I then query for flows in the agent, it works, but if I query flows over the port-forward, it doesn't.

Collecting hubble status
+ ./hubble status
Healthcheck (via localhost:4245): Ok
Current/Max Flows: 1,593/4,095 (38.90%)
Flows/s: 4.76
Connected Nodes: 1/1
+ kubectl -n kube-system exec ds/cilium -c cilium-agent -- hubble status
Healthcheck (via unix:///var/run/cilium/hubble.sock): Ok
Current/Max Flows: 1,595/4,095 (38.95%)
Flows/s: 4.76
+ echo 'Checking flows 1'
Checking flows 1
+ ./hubble observe -n kube-system -o jsonpb
+ echo 'Checking flows 2'
Checking flows 2
+ kubectl -n kube-system exec ds/cilium -c cilium-agent -- hubble observe -n kube-system -o jsonpb
{"flow": .... .}

It's very strange. I thought it was relay, and it might be, but it's able to report back the status, so I'm not suspicious it's just the port-forward, or Hubble CLI itself.

@chancez
Copy link
Contributor Author

chancez commented Jan 27, 2023

Interesting.. with debug logs it indicates it's reading from stdin. I wonder if I broke it (again) with #874

258 time="2023-01-27T19:50:25Z" level=debug msg="Reading flows from stdin"

…ing hubble CLI

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/integration_tests branch from 1881d18 to 3cb88b2 Compare January 27, 2023 20:41
@chancez
Copy link
Contributor Author

chancez commented Jan 27, 2023

Okay, I took my work-around from timescape and added it. The key is adding this, which makes the commands running in the step think they're running in a TTY, preventing Hubble CLI from incorrectly assuming no-TTY == data being fed in via stdin.

        shell: 'script -q -e -c "bash --noprofile --norc -eo pipefail {0}"'

@chancez
Copy link
Contributor Author

chancez commented Jan 27, 2023

It's stuck waiting on build, but I think it's because I renamed that action to unit-test instead, in a different PR. If anyone can help fix that in the settings @gandro or @rolinh

@gandro
Copy link
Member

gandro commented Jan 30, 2023

It's stuck waiting on build, but I think it's because I renamed that action to unit-test instead, in a different PR. If anyone can help fix that in the settings @gandro or @rolinh

Not sure I follow. The workflow in question is renamed in this PR. We can ignore the required build in this PR for now and an admin can merge it once it's marked ready-to-merge.

But before this PR is merged, I don't think we should change the the required settings, because all other PRs would then be unable pass the requirements (until this one here is merged).

@chancez
Copy link
Contributor Author

chancez commented Jan 30, 2023

Not sure I follow. The workflow in question is renamed in this PR. We can ignore the required build in this PR for now and an admin can merge it once it's marked ready-to-merge.

Ah you're right, I was juggling two PRs and thought I renamed it in the other PR. That being said, this PR is ready @gandro

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM.

@chancez chancez added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. labels Jan 30, 2023
@gandro gandro merged commit 09478e0 into master Jan 31, 2023
@gandro gandro deleted the pr/chancez/integration_tests branch January 31, 2023 10:50
@gandro
Copy link
Member

gandro commented Jan 31, 2023

Thanks! Updated the required pipelines, all open PRs will have to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 area/CI Impacts the testing / continuous integration testing of the project. 🧹 kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants