fix(target): handle creating short form custom target #1875

aali309 · 2024-02-12T21:26:14Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

Fixes: #1867

Description of the change:

Handle bug when short form custom target i.e localhost:0 is created.

Motivation for the change:

mentioned by @cmah88: #1867 (comment)

How to manually test:

Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
...

aali309 · 2024-02-12T21:34:05Z

aali309 · 2024-02-15T01:20:32Z

create/stop/archive/delete and view report:

andrewazores · 2024-02-16T19:30:32Z

/build_test

github-actions · 2024-02-16T19:30:57Z

Workflow started at 2/16/2024, 2:30:57 PM. View Actions Run.

andrewazores · 2024-02-16T19:38:15Z

https://github.com/cryostatio/cryostat/actions/runs/7935345026

Looks like a unit test needs updating.

aali309 · 2024-02-16T22:45:15Z

/build_test

github-actions · 2024-02-16T22:45:37Z

Workflow started at 2/16/2024, 5:45:37 PM. View Actions Run.

github-actions · 2024-02-16T22:52:38Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-6eee362084702e032a4198b969dfb8fd36f89df8-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-6eee362084702e032a4198b969dfb8fd36f89df8-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6eee362084702e032a4198b969dfb8fd36f89df8-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6eee362084702e032a4198b969dfb8fd36f89df8-linux-arm64 sh smoketest.sh

github-actions · 2024-02-16T23:01:01Z

/build_test : At least one test failed ❌.
View Actions Run.

andrewazores · 2024-02-20T15:03:26Z

Looks like this is the root cause of the itest failures, I think the rest might be cascading failures because this failed and didn't clean up after itself:

Error:    CustomTargetsIT.shouldBeAbleToDefineTarget:218 
Expected: "localhost:0"
     but: was "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi"
Error:    CustomTargetsIT.shouldBeAbleToTestTargetConnection:117 
Expected: "localhost:0"
     but: was "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi"
Error:    CustomTargetsIT.targetShouldAppearInListing:306 
Expected: iterable with items [<{"jvmId":"IqeNoQhFIdrZibyy8qZ_Jld5i_LZmD9FgwXywciupeM=","labels":{},"alias":"io.cryostat.Cryostat","connectUrl":"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi","annotations":{"platform":{},"cryostat":{"JAVA_MAIN":"io.cryostat.Cryostat","REALM":"JDP","HOST":"cryostat-itests","PORT":"9091"}}}>, <{"jvmId":"IqeNoQhFIdrZibyy8qZ_Jld5i_LZmD9FgwXywciupeM=","labels":{},"alias":"self","connectUrl":"localhost:0","annotations":{"platform":{},"cryostat":{"REALM":"Custom Targets"}}}>] in any order
     but: not matched: <{"jvmId":"IqeNoQhFIdrZibyy8qZ_Jld5i_LZmD9FgwXywciupeM=","connectUrl":"service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi","alias":"self","labels":{},"annotations":{"platform":{},"cryostat":{"REALM":"Custom Targets"}}}>
Error:    CustomTargetsIT.targetShouldNoLongerAppearInListing:370 
Expected: <1>
     but: was <2>

aali309 · 2024-02-21T01:45:48Z

/build_test

github-actions · 2024-02-21T01:46:10Z

Workflow started at 2/20/2024, 8:46:10 PM. View Actions Run.

aali309 · 2024-02-21T01:51:39Z

/build_test

github-actions · 2024-02-21T01:51:58Z

Workflow started at 2/20/2024, 8:51:57 PM. View Actions Run.

github-actions · 2024-02-21T01:53:40Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-2d744627229ae4e34c919f676b5bf67cdd1d1dfa-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-2d744627229ae4e34c919f676b5bf67cdd1d1dfa-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-2d744627229ae4e34c919f676b5bf67cdd1d1dfa-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-2d744627229ae4e34c919f676b5bf67cdd1d1dfa-linux-arm64 sh smoketest.sh

github-actions · 2024-02-21T01:58:55Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-6d946bf446ef08aa9210e0c4ccbc501731f14060-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-6d946bf446ef08aa9210e0c4ccbc501731f14060-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6d946bf446ef08aa9210e0c4ccbc501731f14060-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6d946bf446ef08aa9210e0c4ccbc501731f14060-linux-arm64 sh smoketest.sh

github-actions · 2024-02-21T02:13:15Z

/build_test : At least one test failed ❌.
View Actions Run.

aali309 · 2024-02-21T02:49:48Z

/retest

github-actions · 2024-02-21T02:50:12Z

Workflow started at 2/20/2024, 9:50:12 PM. View Actions Run.

github-actions · 2024-02-21T02:52:18Z

/retest Integration: At least one test failed ❌.
View Actions Run.

github-actions · 2024-02-21T07:54:04Z

/build_test : At least one test failed ❌.
View Actions Run.

aali309 · 2024-02-21T11:01:40Z

/build_test

github-actions · 2024-02-21T11:02:04Z

Workflow started at 2/21/2024, 6:02:02 AM. View Actions Run.

andrewazores · 2024-02-21T14:06:14Z

src/test/java/itest/CustomTargetsIT.java

@@ -341,9 +349,11 @@ void shouldBeAbleToDeleteTarget()
                    }
                });

+        String jmxServiceUrl = "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi";
+        String encodedUrl = URLEncoder.encode(jmxServiceUrl, StandardCharsets.UTF_8);


Despite its name, URLEncoder is not actually the right tool for this job:

https://docs.oracle.com/javase/8/docs/api/java/net/URLEncoder.html

Utility class for HTML form encoding. This class contains static methods for converting a String to the application/x-www-form-urlencoded MIME format.

I think it's better to use URLEncodedUtils:

cryostat/src/test/java/itest/bases/StandardSelfTest.java

Line 47 in c2b26e1

URLEncodedUtils.formatSegments(

https://hc.apache.org/httpcomponents-client-4.5.x/current/httpclient/apidocs/org/apache/http/client/utils/URLEncodedUtils.html#formatSegments(java.lang.Iterable,%20java.nio.charset.Charset)

andrewazores · 2024-02-21T14:16:03Z

src/main/java/io/cryostat/net/web/http/api/v2/TargetsPostHandler.java

            if (StringUtils.isBlank(connectUrl)) {
                throw new ApiException(400, "\"connectUrl\" form parameter must be provided");
            }
+            // check incase custom target has short form connection url (i.e `localhost:0`,
+            // etc)
+            if (connectUrl.matches("localhost:\\d+")) {


This regex pattern should not match only localhost - it could be any hostname, or domain name, or IP address here instead.

We'll need to come up with some way to distinguish these "short forms" from fully formed URLs somehow, with a precise definition. I guess what is common to all these options is that they have a host/domain/IP and a port only, and there is no URL scheme/protocol, path, query, etc. So we know the characters : and / and any whitespace will not appear in the left hand side since these could only be from the scheme or path, and only numbers will appear on the right hand side for the port. Maybe something like this:

connectUrl = connectUrl.strip(); boolean isShortForm = connectUrl.matches("^[^\\s/:]+:\\d+$");

aali309 · 2024-02-21T18:07:02Z

/build_test

github-actions · 2024-02-21T18:07:25Z

Workflow started at 2/21/2024, 1:07:25 PM. View Actions Run.

github-actions · 2024-02-21T18:14:25Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-7e3af7d99352b63dc27c2c1a8d6cca7423ca029a-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-7e3af7d99352b63dc27c2c1a8d6cca7423ca029a-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-7e3af7d99352b63dc27c2c1a8d6cca7423ca029a-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-7e3af7d99352b63dc27c2c1a8d6cca7423ca029a-linux-arm64 sh smoketest.sh

github-actions · 2024-02-21T18:26:41Z

/build_test : At least one test failed ❌.
View Actions Run.

aali309 · 2024-02-21T19:16:24Z

Trying to figure out why the tests fail here but not locally. Even added print logs locally to see and everything is as expected.

[INFO] 
[WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 2
[INFO] 
[INFO] 
[INFO] --- maven-failsafe-plugin:3.2.5:verify (default-cli) @ cryostat ---
[INFO] 
[INFO] --- exec-maven-plugin:3.1.1:exec (capture-oci-logs) @ cryostat ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  11:32 min
[INFO] Finished at: 2024-02-21T14:04:17-05:00
[INFO] ------------------------------------------------------------------------
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------------< io.cryostat:cryostat >------------------------
[INFO] Building cryostat 2.5.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- exec-maven-plugin:3.1.1:exec (destroy-pod) @ cryostat ---
2ad719f36c3dd668e1c76bba80d429ba6d189903cfc0771466b88c110f523cc2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.124 s
[INFO] Finished at: 2024-02-21T14:04:26-05:00

Also logs show correct and expected output opposite from what is shown on the workflow errors!

[INFO] Running itest.CustomTargetsIT
+++ connectUrl shouldBeAbleToTestTargetConnection(): service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi
+++ expectedURL shouldBeAbleToTestTargetConnection(): service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi


+++ connectUrl shouldBeAbleToDefineTarget(): service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi
+++ expectedURL shouldBeAbleToDefineTarget(): service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi


+++ body targetShouldAppearInListing(): 
[{"jvmId":"p_WOMaNktMoWpUDs_Rzz81SUgkD3fElmm64rTQAc0dc=","connectUrl":"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi","alias":"io.cryostat.Cryostat","labels":{},"annotations":{"platform":{},"cryostat":{"HOST":"cryostat-itests","PORT":"9091","JAVA_MAIN":"io.cryostat.Cryostat","REALM":"JDP"}}},

{"jvmId":"p_WOMaNktMoWpUDs_Rzz81SUgkD3fElmm64rTQAc0dc=","connectUrl":"service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi","alias":"self","labels":{},"annotations":{"platform":{},"cryostat":{"REALM":"Custom Targets"}}}]

+++ selfCustom targetShouldAppearInListing(): {"connectUrl":"service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi","alias":"self","labels":{},"jvmId":"p_WOMaNktMoWpUDs_Rzz81SUgkD3fElmm64rTQAc0dc=","annotations":{"platform":{},"cryostat":{"REALM":"Custom Targets"}}}

+++ selfJDP targetShouldAppearInListing(): {"connectUrl":"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi","alias":"io.cryostat.Cryostat","labels":{},"jvmId":"p_WOMaNktMoWpUDs_Rzz81SUgkD3fElmm64rTQAc0dc=","annotations":{"platform":{},"cryostat":{"PORT":"9091","HOST":"cryostat-itests","REALM":"JDP","JAVA_MAIN":"io.cryostat.Cryostat"}}}

+++body in targetShouldNoLongerAppearInListing() : [{"jvmId":"p_WOMaNktMoWpUDs_Rzz81SUgkD3fElmm64rTQAc0dc=","connectUrl":"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi","alias":"io.cryostat.Cryostat","labels":{},"annotations":{"platform":{},"cryostat":{"HOST":"cryostat-itests","PORT":"9091","JAVA_MAIN":"io.cryostat.Cryostat","REALM":"JDP"}}}]

+++size targetShouldNoLongerAppearInListing(): 1
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.19 s -- in itest.CustomTargetsIT```

Also the encoded url looks good 
`+++:encodedUrl:  /service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Flocalhost:0%2Fjmxrmi
`

andrewazores · 2024-02-21T19:35:58Z

Could there be something like trailing newlines (or other invisible whitespace) characters that are breaking the comparison assertions?

aali309 · 2024-02-21T19:42:23Z

Could there be something like trailing newlines (or other invisible whitespace) characters that are breaking the comparison assertions?

I thought this would be the issue too, I checked but dint see any.. I will check again.

andrewazores · 2024-02-21T19:47:47Z

Try adding a .strip() call on both the expected and actual value in each assertion, ex. MatcherAssert.assertEquals(expected.strip(), actual.strip()). Should make it pretty quick and easy to test if that's what's going on.

aali309 · 2024-02-21T20:09:18Z

/build_test

github-actions · 2024-02-21T20:09:39Z

Workflow started at 2/21/2024, 3:09:38 PM. View Actions Run.

github-actions · 2024-02-21T20:16:48Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-831ee2395c69a5960965b6fed4f01aca76489300-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-831ee2395c69a5960965b6fed4f01aca76489300-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-831ee2395c69a5960965b6fed4f01aca76489300-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-831ee2395c69a5960965b6fed4f01aca76489300-linux-arm64 sh smoketest.sh

github-actions · 2024-02-21T21:12:09Z

/build_test : At least one test failed ❌.
View Actions Run.

aali309 · 2024-02-22T01:09:32Z

/build_test

github-actions · 2024-02-22T01:09:52Z

Workflow started at 2/21/2024, 8:09:52 PM. View Actions Run.

github-actions · 2024-02-22T01:16:26Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1875-6e115c28109cf407dc6eba319312ac119e60c0db-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1875-6e115c28109cf407dc6eba319312ac119e60c0db-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6e115c28109cf407dc6eba319312ac119e60c0db-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1875-6e115c28109cf407dc6eba319312ac119e60c0db-linux-arm64 sh smoketest.sh

github-actions · 2024-02-22T01:26:47Z

/build_test : At least one test failed ❌.
View Actions Run.

aali309 added fix safe-to-test labels Feb 12, 2024

andrewazores added the backport label Feb 12, 2024

aali309 changed the title ~~fix(target):handle creating short form custom target~~ fix(target): handle creating short form custom target Feb 12, 2024

aali309 force-pushed the createShortFormCustomTarget branch 2 times, most recently from 00cb5a5 to 6e42912 Compare February 15, 2024 01:14

aali309 requested a review from andrewazores February 16, 2024 18:48

andrewazores reviewed Feb 21, 2024

View reviewed changes

aali309 added 7 commits February 21, 2024 14:35

handle short form custom target

c752209

convert shortform to full JMXServiceURL in TargetPostHandler

5ee9314

mvn spotless:apply

49ee548

handle unit test failure and updated test

ce61785

encode JMX service URL

cbd36af

mvn spotless:apply

9d814da

resolve issues

69c274f

aali309 force-pushed the createShortFormCustomTarget branch from 7e3af7d to 69c274f Compare February 21, 2024 19:35

add .strip() to handle leading or trailing whitespaces

831ee23

drop variable name

6e115c2

aali309 closed this May 7, 2024

aali309 deleted the createShortFormCustomTarget branch May 7, 2024 01:56

fix(target): handle creating short form custom target #1875

fix(target): handle creating short form custom target #1875

Conversation

aali309 commented Feb 12, 2024

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

Motivation for the change:

How to manually test:

aali309 commented Feb 12, 2024

aali309 commented Feb 15, 2024

andrewazores commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

andrewazores commented Feb 16, 2024

aali309 commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

andrewazores commented Feb 20, 2024

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

andrewazores Feb 21, 2024

Choose a reason for hiding this comment

andrewazores Feb 21, 2024

Choose a reason for hiding this comment

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

aali309 commented Feb 21, 2024 • edited

andrewazores commented Feb 21, 2024

aali309 commented Feb 21, 2024

andrewazores commented Feb 21, 2024

aali309 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

aali309 commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

aali309 commented Feb 21, 2024 •

edited