-
Notifications
You must be signed in to change notification settings - Fork 77
feat: enable IPv6 support for factory resolver endpoints #951
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
| // | ||
| // In that case, we treat "/scm" as part of the fixed endpoint prefix, not as part of the | ||
| // repository path. | ||
| final String endpointWithoutScheme = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpointWithoutScheme => hostname ?
| try { | ||
| URI uri = URI.create(repositoryUrl); | ||
| if (uri.getScheme() != null && uri.getHost() != null) { | ||
| String serverUrl = uri.getScheme() + "://" + uri.getAuthority(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if uri.getAuthority() is null?
| if (uri.getScheme() != null && uri.getHost() != null) { | ||
| String serverUrl = uri.getScheme() + "://" + uri.getAuthority(); | ||
| // Remove path and query from the server URL | ||
| int pathIndex = repositoryUrl.indexOf(uri.getAuthority()) + uri.getAuthority().length(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move uri.getAuthority() to a local variable.
|
@olexii4 Please update the related tests with ip v6 examples. |
31df750 to
164262d
Compare
| public void verify(VerificationData verificationData) { | ||
| List<Invocation> invocations = verificationData.getAllInvocations(); | ||
| InvocationMatcher invocationMatcher = verificationData.getWanted(); | ||
| MatchableInvocation invocationMatcher = verificationData.getTarget(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were required for Mockito 5 compatibility. The test was using Mockito internal APIs that have breaking changes between Mockito 3.11.2 (old version) and 5.21.0 (new version):
Changes made:
Import change: org.mockito.internal.invocation.InvocationMatcher → org.mockito.invocation.MatchableInvocation
Mockito 5 moved this from internal to public API
API change: verificationData.getWanted() → verificationData.getTarget()
Method was renamed in Mockito 5's VerificationData interface
|
|
||
| EnvironmentContext.getCurrent().setSubject(new SubjectImpl("name", "id", "token1234", false)); | ||
| when(preferenceManager.find(anyObject(), anyObject())).thenReturn(preferences); | ||
| when(preferenceManager.find(anyString(), anyString())).thenReturn(preferences); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was required for Mockito 5 compatibility. The test was using a deprecated matcher that was removed in Mockito 5:
Change made:
Import: ArgumentMatchers.anyObject() → ArgumentMatchers.anyString()
Usage: preferenceManager.find(anyObject(), anyObject()) → preferenceManager.find(anyString(), anyString())
| KubernetesClient client = spy(kubernetesClient); | ||
| when(cheServerKubernetesClientFactory.create()).thenReturn(client); | ||
| kubernetesClient = kubernetesMockServer.createClient(); | ||
| when(cheServerKubernetesClientFactory.create()).thenReturn(kubernetesClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were required because Mockito 5 doesn't allow spy() on mock objects. This is a breaking change from Mockito 3.x:
Changes made:
- Removed import static org.mockito.Mockito.spy;
- Changed setup from:
kubernetesClient = spy(kubernetesMockServer.createClient());
KubernetesClient client = spy(kubernetesClient);
when(cheServerKubernetesClientFactory.create()).thenReturn(client);To:
kubernetesClient = kubernetesMockServer.createClient();
when(cheServerKubernetesClientFactory.create()).thenReturn(kubernetesClient);| }) | ||
| .when(subject) | ||
| .checkPermission(anyObject(), anyObject(), anyObject()); | ||
| .checkPermission(any(), any(), any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was required for Mockito 5 compatibility due to the removal of the deprecated anyObject() matcher:
Change made:
- Import: ArgumentMatchers.anyObject() → ArgumentMatchers.any()
- Usage: checkPermission(anyObject(), anyObject(), anyObject()) → checkPermission(any(), any(), any())
- Handle edge cases where URI authority parsing fails - Add defensive null checks for URI.getAuthority() - Support IPv6 addresses in server URL extraction - Add IPv6 test coverage for GitLab, GitHub and Azure DevOps parsers Signed-off-by: Oleksii Orel <oorel@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Update Mockito version and adjust affected tests to use Mockito 5 compatible APIs: - Replace InvocationMatcher with MatchableInvocation - Replace getWanted() with getTarget() - Replace anyObject() with any()/anyString() - Remove spy() on mock objects Signed-off-by: Oleksii Orel <oorel@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
b894630 to
14f521d
Compare
Updates the license header to fix the license-maven-plugin check failure. Assisted-by: Cursor AI Signed-off-by: Oleksii Orel <oorel@redhat.com>
Updates the license headers in AzureDevOpsURLParser and AzureDevOpsURLParserTest to fix the license-maven-plugin check failure. Assisted-by: Cursor AI Signed-off-by: Oleksii Orel <oorel@redhat.com>
Optimizations: - Reduce redundant pattern matching in AzureDevOpsURLParser.parse() by reusing matcher objects instead of creating new ones - Replace ArrayList-based array concatenation with System.arraycopy in ConcurrentCompositeLineConsumerTest.appendTo() for better performance - Remove unused ArrayList import Assisted-by: Cursor AI Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
@olexii4: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What does this PR do?
This change adds IPv6 support in Eclipse Che Server to enable operation in IPv6-only network environments. As Kubernetes platforms (particularly OpenShift Container Platform 4.20) migrate to IPv6, the factory resolver must handle IPv6 addresses correctly in URLs and API calls.
The implementation involved minimal, focused changes to URL validation patterns and server URL extraction, enabling full IPv6 support while maintaining complete backward compatibility with IPv4-only and dual-stack environments.
Background:
The Eclipse Che factory resolver previously failed to operate with IPv6 git server URLs due to:
http://[::1]:8080)IPv6 Bracket Notation:
IPv6 addresses in URLs require square brackets to distinguish the address from port numbers:
http://[2001:db8::1]:8080/repo.githttp://2001:db8::1:8080/repo.git(ambiguous)Changes:
AbstractGitlabUrlParser:
AbstractGithubURLParser:
AzureDevOpsURLParser:
Technical Details:
Java's URI.getHost() returns IPv6 addresses WITH brackets (e.g., "[2001:db8::1]"). When constructing regex patterns, these brackets must be escaped as
\[and\]because they are special regex characters. The fix uses:Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes: eclipse-che/che#23674
How to test this PR?
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedRelease Notes
Reviewers
Reviewers, please comment how you tested the PR when approving it.