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

Fix SamlAuthenticationIT flakyness #103867

Merged
merged 39 commits into from
Jan 24, 2024

Conversation

breskeby
Copy link
Contributor

@breskeby breskeby commented Jan 3, 2024

No description provided.

@breskeby breskeby added >non-issue :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v8.13.0 labels Jan 3, 2024
@breskeby breskeby self-assigned this Jan 3, 2024
@breskeby breskeby added the WIP label Jan 9, 2024
@breskeby breskeby force-pushed the debug-saml-tests branch 4 times, most recently from 35ec975 to 07f55c3 Compare January 17, 2024 11:01
@breskeby breskeby force-pushed the debug-saml-tests branch 5 times, most recently from bd1c264 to a9a8809 Compare January 23, 2024 09:02
@@ -181,7 +185,7 @@ public void execute() {
}

final List<String> tags = parameters.getTags().get();
final boolean isCrossPlatform = parameters.getPlatform().get().equals(Architecture.current().dockerPlatform) == false;
final boolean isCrossPlatform = isCrossPlatform();
Copy link
Contributor Author

@breskeby breskeby Jan 23, 2024

Choose a reason for hiding this comment

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

Tweaked this to allow reusing this task and supporting multi platform images

Copy link
Contributor

@mark-vieira mark-vieira Jan 24, 2024

Choose a reason for hiding this comment

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

Ah, neat. I didn't know you could pass multiple platforms at once.

@@ -203,18 +207,34 @@ public void execute() {
tags.forEach(tag -> spec.args("--tag", tag));

parameters.getBuildArgs().get().forEach((k, v) -> spec.args("--build-arg", k + "=" + v));

if (parameters.getPush().getOrElse(false)) {
Copy link
Contributor Author

@breskeby breskeby Jan 23, 2024

Choose a reason for hiding this comment

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

Allow pushing directly from here. Makes it easier to push multiple images within a gradle build without the need of doing as part of a script after the build as we use to do for distribution images

@breskeby
Copy link
Contributor Author

unset DOCKER_REGISTRY_USERNAME DOCKER_REGISTRY_PASSWORD

docker buildx create --use
.ci/scripts/run-gradle.sh deployFixtureDockerImages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prep for setting up a ci pipeline for building and deploying docker based fixtures once per day https://github.com/elastic/elasticsearch-infra/pull/378

@@ -9,42 +9,14 @@

default-init-method="initialize"
default-destroy-method="destroy">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While debugging I noticed some unused metrics are setup. removed more output noise than startup time though


tasks.register("deployFixtureDockerImages") {
dependsOn tasks.withType(DockerBuildTask)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can move parts of this to an internal plugin once we do this fixture pre baking for more fixtures. We can evaluate existing fixtures in a later pr

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@breskeby breskeby marked this pull request as ready for review January 24, 2024 09:18
@breskeby breskeby requested a review from a team as a code owner January 24, 2024 09:18
@breskeby breskeby changed the title Debug SamlAuthenticationIT flakyness Fix SamlAuthenticationIT flakyness Jan 24, 2024
@@ -181,7 +185,7 @@ public void execute() {
}

final List<String> tags = parameters.getTags().get();
final boolean isCrossPlatform = parameters.getPlatform().get().equals(Architecture.current().dockerPlatform) == false;
final boolean isCrossPlatform = isCrossPlatform();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unnecessary to have this field now that this logic lives in the isCrosPlatform() method.


@Inject
public DockerBuildTask(WorkerExecutor workerExecutor, ObjectFactory objectFactory, ProjectLayout projectLayout) {
this.workerExecutor = workerExecutor;
this.markerFile = objectFactory.fileProperty();
this.dockerContext = objectFactory.directoryProperty();
this.buildArgs = objectFactory.mapProperty(String.class, String.class);
this.platform = objectFactory.property(String.class).convention(Architecture.current().dockerPlatform);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this change is going to have implications for the serverless build, we'll want to follow up with a fix there once this is merged.


tasks.register("deployFixtureDockerImages") {
dependsOn tasks.withType(DockerBuildTask)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

baseImages = ["openjdk:11.0.16-jre"]
noCache = BuildParams.isCi
tags = ["docker.elastic.co/elasticsearch-dev/idp-fixture:1.0"]
getPush().set(BuildParams.isCi)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be able to simplify this as push = BuildParams.isCI

// .expose(4443)
.build()
)
new ImageFromDockerfile("es-idp-testfixture").withDockerfileFromBuilder(builder -> builder.from(DOCKER_BASE_IMAGE).build())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use RemoteDockerImage here instead. That should avoid building essentially an "empty" Dockerfile that just extends from the remote one.


.build()
)
new ImageFromDockerfile("es-openldap-testfixture").withDockerfileFromBuilder(builder -> builder.from(DOCKER_BASE_IMAGE).build())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, we should use RemoteDockerImage.

@breskeby breskeby added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed WIP labels Jan 24, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@elasticsearchmachine elasticsearchmachine merged commit ab8ee60 into elastic:main Jan 24, 2024
15 checks passed
@breskeby breskeby deleted the debug-saml-tests branch January 24, 2024 18:44
henningandersen pushed a commit to henningandersen/elasticsearch that referenced this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants