Add IPv6 validation within python and node apps#1535
Add IPv6 validation within python and node apps#1535jochenehret merged 8 commits intocloudfoundry:developfrom
Conversation
peanball
left a comment
peanball
left a comment
There was a problem hiding this comment.
The biggest request is to not make the app crash (sys.exit(1)) when the IPv6 test fails.
A more fine-grained response from the test endpoint with all the details (IPv4 tests success or failed, what IP was seen, same for v6 and) would be really helpful to see what failed when the test fails. Otherwise you just see "the test failed" without any indication why.
Please run go fmt on all Go files.
helpers/config/config_struct.go
Outdated
| IncludeV3 *bool `json:"include_v3"` | ||
| IncludeVolumeServices *bool `json:"include_volume_services"` | ||
| IncludeZipkin *bool `json:"include_zipkin"` | ||
| IncludeIpv6 *bool `json:"include_ipv6"` |
There was a problem hiding this comment.
I would prefer IncludeIPv6 (see also other acronyms such as IncludeHTTP2Routingm IncludeTCPRouting, etc).
assets/python/server.py
Outdated
| 'api64.ipify.org': "Dual stack" | ||
| } | ||
|
|
||
| PORT = '8080' |
There was a problem hiding this comment.
maybe this could be called DEFAULT_PORT, as it is the fallback when there is no PORT env variable (e.g. when debugging locally)
assets/python/server.py
Outdated
| logging.info("Test execution has completed. IPv6 validation is successful.") | ||
| else: | ||
| logging.error("Test execution has completed. IPv6 validation failed.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
This will just kill the server process outright. You might want to return a boolean here and check it where it's called.
It would be good to collect and return all results as well, so they can be passed on to the response output, so the curl client in the test can read the details also.
assets/python/server.py
Outdated
| self.end_headers() | ||
| message = "Hello python, world!" | ||
| tester = IPv6Tester(list(ENDPOINT_TYPE_MAP.keys())) | ||
| tester.test_all_addresses() |
There was a problem hiding this comment.
Here you could have a boolean and output information, and potentially even the status code (e.g. not 200) when the conditions of the test fail.
Additionally, the output of what was provided back from the egress calls, i.e. IPv4, IPv6 addresses, or what failed would be really nice for debugging when the test actually fails.
helpers/config/config_test.go
Outdated
| IncludeVolumeServices *bool `json:"include_volume_services,omitempty"` | ||
| IncludeZipkin *bool `json:"include_zipkin,omitempty"` | ||
| IncludeWindows *bool `json:"include_windows,omitempty"` | ||
| IncludeIpv6 *bool `json:"include_ipv6,omitempty"` |
There was a problem hiding this comment.
The indentation is off because tabs are used and above spaces are used. This can be avoided by using go fmt.
| const SkipFileBasedServiceBindingsBuildpackApp = `Skipping this test because config.IncludeFileBasedServiceBindings is set to 'false'.` | ||
| const SkipFileBasedServiceBindingsCnbApp = `Skipping this test because config.IncludeFileBasedServiceBindings and/or config.IncludeCNB are set to 'false'.` | ||
| const SkipFileBasedServiceBindingsDockerApp = `Skipping this test because config.IncludeFileBasedServiceBindings and/or config.IncludeDocker are set to 'false'.` | ||
| const SkipIncludeIpv6 = `Skipping this test because config.IncludeIpv6 is set to 'false'.` |
There was a problem hiding this comment.
I suggest calling this SkipIPv6, which makes it clearer what is actually skipped.
ipv6/ipv6.go
Outdated
|
|
||
| Eventually(func() string { | ||
| return helpers.CurlApp(Config, appName, "/ipv6-test") | ||
| }).Should(ContainSubstring("IPv6 tests executed")) |
There was a problem hiding this comment.
There is output for IPv4 tests succeeded, IPv6 tests succeeded, Dual stack tests succeeded. It would be good to have all of those as part of the response and not (just) the stdout of the test app.
They could then be explicit assertions in the test. This makes it easier to see what exactly failed and why, when something failed.
ipv6/ipv6.go
Outdated
| It examines that after making a request to a predifined route, | ||
| IPv6 tests are executed successfully */ | ||
|
|
||
| Eventually(func() string { |
There was a problem hiding this comment.
There should not be a need for the Eventually. It is needed above to make sure that the app is up and running, and retry if it's not yet. But at that point the app is up and running and you can for the call right away.
Accordingly you can then store the response and have multiple response.Should(ContainSubstring("... tests succeeded")) calls (see comment below).
Milena-Encheva
changed the title
ipv6/ipv6.go
Outdated
|
|
||
| Describe("Egress Capability in Apps", func() { | ||
| for _, stack := range Config.GetStacks() { | ||
| stack := stack |
|
[like] Heinrich, Oliver reacted to your message:
…________________________________
From: Jochen Ehret ***@***.***>
Sent: Tuesday, May 13, 2025 6:58:26 AM
To: cloudfoundry/cf-acceptance-tests ***@***.***>
Cc: Heinrich, Oliver ***@***.***>; Mention ***@***.***>
Subject: Re: [cloudfoundry/cf-acceptance-tests] Add IPv6 validation within python and node apps (PR #1535)
Merged #1535<#1535> into develop.
—
Reply to this email directly, view it on GitHub<#1535 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACVH4LB73VLHRHB3Z6BKTLD26GJZFAVCNFSM6AAAAAB3WLCSV2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJXGYZDSMZRHE2TSOA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
5 participants
Are you submitting this PR against the develop branch?
Yes.
All PR's to CATs should be submitted to develop and will be merged to main once they've passed acceptance.
What is this change about?
The change is related to IPv6 support validation. The main purpose of the newly created
ipv6test group is to test IPv6 egress calls using different buildpacks, including Java, Python, and others relevant to our ecosystem which will be added later on.Please provide contextual information.
https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0038-ipv6-dual-stack-for-cf.md
What version of cf-deployment have you run this cf-acceptance-test change against?
Please check all that apply for this PR:
Did you update the README as appropriate for this change?
If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?
_CATs should validate IPv6 egress calls after support of the protocol is provided.
How many more (or fewer) seconds of runtime will this change introduce to CATs?
Around 90 seconds per test.
What is the level of urgency for publishing this change?
Tag your pair, your PM, and/or team!
@oliver-heinrich