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

Adapt to IPV6 envs #742

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Adapt to IPV6 envs #742

merged 3 commits into from
Mar 27, 2024

Conversation

raul-arabaolaza
Copy link
Contributor

@raul-arabaolaza raul-arabaolaza commented Mar 22, 2024

While running workflow-durable-task-step tests on a proprietary environment with only IPV6 networking enabled I found some parts of RJR that were not fully IPV6 compatible. This PR tries to fix those parts so RJR can be used in both IPV4 and IPV6 environments.

Testing done

Submitter checklist

Preview Give feedback

Usee `httpListenAddress` as default host

There are no guarantees `localhost` is properly configured, specially in IPV6
environments
@@ -671,7 +671,7 @@ public URL getUrl() throws MalformedURLException {
if (port == 0) {
throw new IllegalStateException("This method must be called after calling #startJenkins.");
}
return new URL("http://" + host + ":" + port + "/jenkins/");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This failed when host was an IPV6 ip

Copy link
Member

Choose a reason for hiding this comment

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

I think this line was the only necessary change. The other change caused test regressions in other parts, so I'm proposing a revert of them in #776

@raul-arabaolaza raul-arabaolaza marked this pull request as ready for review March 27, 2024 16:02
@raul-arabaolaza raul-arabaolaza changed the title Use IPV6 compatible URL constructor Adapt to IPV6 envs Mar 27, 2024
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks right but @PereBueno why did this not come up during testing of #712?

@jglick jglick added the bug label Mar 27, 2024
@jtnord
Copy link
Member

jtnord commented Mar 27, 2024

Looks right but @PereBueno why did this not come up during testing of #712?

Most likely system configuration.

Some hosts/systems/OSes call ::1 localhost-6 causing a miss-match between the name and the address when used before this change.

Other hosts have 2 entries for localhost, one for v4 and one for v6.

@jglick jglick merged commit 0138ccb into jenkinsci:master Mar 27, 2024
14 checks passed
Vlatombe added a commit to Vlatombe/jenkins-test-harness that referenced this pull request May 31, 2024
Only leaves the relevant part which was to switch to the
URL(String,String,String,String) constructor.

The change from localhost to ip caused some test failures in Kubernetes
context (It is invalid to provide an IP address as part of an ingress
host name)
Vlatombe added a commit to Vlatombe/jenkins-test-harness that referenced this pull request May 31, 2024
Reverts part of jenkinsci#742

Only leaves the relevant part which was to switch to the
URL(String,String,String,String) constructor.

The change from localhost to ip caused some test failures in Kubernetes
context (It is invalid to provide an IP address as part of an ingress
host name)
@Vlatombe Vlatombe mentioned this pull request May 31, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants