-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Use ephemeral ports for idp-fixture #40333
Conversation
This change removes the use of hardcoded port values for the idp-fixture in favor of the mapped ephemeral ports. This should prevent failures due to port conflicts in CI.
Pinging @elastic/es-core-infra |
We should also change
to
in |
@@ -17,11 +20,29 @@ testFixtures.useFixture ":x-pack:test:idp-fixture" | |||
|
|||
String outputDir = "${project.buildDir}/generated-resources/${project.name}" | |||
task copyIdpCertificate(type: Copy) { |
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.
Rename task to copyFiles
or something generic like that ?
into outputDir | ||
} | ||
project.sourceSets.test.output.dir(outputDir, builtBy: copyIdpCertificate) | ||
integTestCluster.dependsOn copyIdpCertificate | ||
|
||
task setupPorts { |
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.
Thanks for noticing and fixing this @jaymode !
The code to look up the ports is not necessary TestFixturesPlugin
already sets this up,
postProcessFixture
will have extension properties with the ports, so you just need something like this: postProcessFixture.doLast { println ext."test.fixtures.shibboleth-idp.tcp.443" }
Note that any testing task also gets passed a system property by the same name.
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.
thanks for that tip!
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 will unfortunately not work as is. The port is part of the SingleSignOnService
binding URL and that URL is set as the Destination
element value in the SAML Authentication Response. So, even if we set this to a random port outside the container, the Shibboleth IDP will still be configured with 4443
internally so from its perspective, the SingleSignOnService
HTTP-Redirect binding Location will still be
https://localhost:4443/idp/profile/SAML2/Redirect/SSO
and it will fail on an authentication request with a Destination set to
https://localhost:34567/idp/profile/SAML2/Redirect/SSO
Setting the Destination
in the authn request is only mandatory when the the request is signed ( otherwise optional ) but even if we were to change this default behavior , the tests would work only for a subset of use cases and then again something else might break.
Shibboleth IDP is not easy to run behind a reverse proxy :/ , I think we'd have to use something like httpd
or nginx
to do proper request rewriting so that both our saml realm and the Shibboleth IDP can be happy.
Alternatively, maybe @atorok knows if this is possible, we might be able to get around this if we can replace the random ephemeral port in the mounted volumes config files, so that shibboleth / tomcat use randomized ports internally also
The idp-metadata.xml gets re-written with the updated port, which is how it works. Maybe shibboleth handles this better now? Or is the test broken? The push from yesterday was missing some of my changes since I committed in a sub directory instead (DOH!). |
This is the idp-metadata.xml file that we copy to Shibboleth internally still knows of the old
|
The reason that this works is due to the way that Shibboleth gets the URL for comparison. It uses |
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.
LGTM
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.
LGTM
This change removes the use of hardcoded port values for the idp-fixture in favor of the mapped ephemeral ports. This should prevent failures due to port conflicts in CI.
This change removes the use of hardcoded port values for the idp-fixture in favor of the mapped ephemeral ports. This should prevent failures due to port conflicts in CI.
This change removes the use of hardcoded port values for the idp-fixture in favor of the mapped ephemeral ports. This should prevent failures due to port conflicts in CI.
This change removes the use of hardcoded port values for the
idp-fixture in favor of the mapped ephemeral ports. This should prevent
failures due to port conflicts in CI.