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

Updates k8sTest pkg to use netip.Addr #25325

Merged
merged 1 commit into from Jun 1, 2023
Merged

Updates k8sTest pkg to use netip.Addr #25325

merged 1 commit into from Jun 1, 2023

Conversation

danehans
Copy link
Contributor

@danehans danehans commented May 8, 2023 •
edited by christarazi

Updates the k8sTest pkg to use netip.Addr() instead of net.IP.

Related: #24246

@danehans danehans requested a review from a team as a code owner May 8, 2023 22:00
@danehans danehans requested a review from nebril May 8, 2023 22:00
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 8, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 8, 2023
nebril
nebril requested changes May 10, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Please see review comments, also there are more ParseIP uses in this file, please take a look at them too. After you push your changes, please re-request review from me.

test/k8s/service_helpers.go Outdated Show resolved Hide resolved
@danehans danehans force-pushed the issue_24246_k8s_test branch 2 times, most recently from 755aa1b to dcb708b Compare May 10, 2023 23:15
@danehans
Copy link
Contributor Author

@nebril thanks for the review and guidance. I've updated the PR so PTAL.

@danehans danehans requested a review from nebril May 11, 2023 00:19
@nebril nebril added the release-note/misc This PR makes changes that have no direct user impact. label May 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 11, 2023
nebril
nebril requested changes May 11, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

That looks much better, thank you for taking the time to fix it! I have one last request, can you change all other instances of ParseIP in this file? I don't think it makes sense to make a separate PRs per helper.

I see instances in curlClusteRIPFromExternalHost and testCurlFromPodWithSourceIPCheck.

@nebril nebril self-requested a review May 11, 2023 16:21
@danehans
Copy link
Contributor Author

@nebril I have updated the PR based on your feedback, PTAL.

@christarazi christarazi added kind/cleanup This includes no functional changes. area/CI Continuous Integration testing issue or flake labels May 15, 2023
@christarazi
Copy link
Member

/test

nebril
nebril approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM

@nebril
Copy link
Member

nebril commented May 16, 2023

The CI failure unfortunately looks legit: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2310/

/home/jenkins/workspace/Cilium-PR-K8s-1.26-kernel-net-next/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:515
Expected
    <netip.Addr>: ::ffff:192.168.56.13
to equal
    <netip.Addr>: fd04::13
/home/jenkins/workspace/Cilium-PR-K8s-1.26-kernel-net-next/src/github.com/cilium/cilium/test/k8s/service_helpers.go:251

It seems like source IP was IPv4, but for some reason Is4 returned false?

@nebril nebril self-requested a review May 16, 2023 07:30
@danehans
Copy link
Contributor Author

@nebril thanks for looking into the CI failure. It appears that an IPv4-mapped IPv6 address was being used instead of an IPv6 address. I updated the PR to ensure the address is IPv6 and not an IPv4-mapped IPv6 address.

@christarazi
Copy link
Member

/test

nebril
nebril requested changes May 17, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Thanks for trying to fix that!

I would suggest writing a small local test that would test this block of code with IPv4in6 addresses, so that you don't have to wait for a whole CI run with your next solution.

test/k8s/service_helpers.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member

christarazi commented May 18, 2023 •
edited

/test

Edit:

@christarazi
Copy link
Member

/test

1 similar comment
@christarazi
Copy link
Member

/test

@christarazi
Copy link
Member

/test-vagrant

@nebril
Copy link
Member

nebril commented May 23, 2023

/test-1.16-4.19

@nebril
Copy link
Member

nebril commented May 23, 2023

/test-1.26-net-next

@nebril
Copy link
Member

nebril commented May 23, 2023

test-1.26-net-next

@nebril
Copy link
Member

nebril commented May 23, 2023

/test-1.26-net-next

3 similar comments
@nbusseneau
Copy link
Member

/test-1.26-net-next

@nbusseneau
Copy link
Member

/test-1.26-net-next

@nbusseneau
Copy link
Member

/test-1.26-net-next

@nbusseneau
Copy link
Member

/test

@danehans
Copy link
Contributor Author

I see the same failing test as #25304 (comment). Commit df707c8
is a rebase.

@christarazi
Copy link
Member

christarazi commented May 23, 2023 •
edited

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Edit: #25524

@christarazi
Copy link
Member

@danehans Just a heads up, each time there's a push, the previous CI results are wiped away and we can't see the results. Pushing to resolve CI failures caused by code is fine, but there's no need to constantly rebase the PR.

@christarazi christarazi reopened this May 24, 2023
@danehans @christarazi
Updates k8sTest pkg to use netip.Addr
e33b951 
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@christarazi
Copy link
Member

/test

@danehans
Copy link
Contributor Author

@christarazi the build error appears to be unrelated to this PR.

@christarazi
Copy link
Member

I'm not sure why tests like gateway-api-conformance-test are not being triggered.

@christarazi christarazi mentioned this pull request May 25, 2023
Closed
@nebril
Copy link
Member

nebril commented May 26, 2023 •
edited

I think that k8s-1.26-kernel-net-next is the only job that actually runs the affected test, so this is safe to merge IMO, but let's wait to see whether we can get gateway api tests run so that future community PRs are not as badly affected by CI as this one.

@christarazi
Copy link
Member

Marking ready to merge as we have approving reviews and relevant CI has passed.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 26, 2023
@danehans
Copy link
Contributor Author

@christarazi thanks for helping to move this PR forward. Do you or someone else need to /approve for this PR to merge?

@julianwiedmann
Copy link
Member

/ci-eks

@julianwiedmann julianwiedmann merged commit 159be8f into cilium:main Jun 1, 2023
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake kind/cleanup This includes no functional changes. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@danehans @christarazi @nebril @nbusseneau @julianwiedmann