JENKINS-60482 Alternate Endpoint fix #439

troymohl · 2020-03-09T19:25:43Z

This fixes a bug in the EC2 Alternate URL endpoint configuration reported in https://issues.jenkins-ci.org/browse/JENKINS-60482. Additionally, small bug fix to fix cases if an empty endpoint got passed to the AmazonEC2 client initialization.

~~This PR has #438 as a parent, as my configuration required that fix to be in place in order to make this fix.~~
This has been fixed in master, so that PR was no longer needed.

five-iron · 2020-04-20T21:10:12Z

Any update on this PR? This is a nice QOL fix. Looks like it's just pending review from @res0nance.

res0nance · 2020-04-21T15:12:47Z

src/main/java/hudson/plugins/ec2/AmazonEC2Cloud.java

@@ -65,6 +60,8 @@
     */
    private String region;

+    private String altEC2Endpoint;


Is the endpoint not required for the AmazonEC2 Client object? I don't see it specified for the object used when spinning up instances etc.

I didn't change any of the existing code that checks for endpoint. I will look at switching the getEc2EndpointUrl logic to better handle where it's directly set and not use the region if the endpoint URL is configured. I think I did see a ticket where the region -> endpoint logic didn't work in C2S, but I won't have the ability to fully test that.

res0nance · 2020-04-21T15:13:56Z

src/main/java/hudson/plugins/ec2/util/AmazonEC2FactoryImpl.java

-        AmazonEC2 client = new AmazonEC2Client(credentialsProvider, EC2Cloud.createClientConfiguration(ec2Endpoint.getHost()));
-        client.setEndpoint(ec2Endpoint.toString());
-        return client;
+        if (ec2Endpoint != null && ec2Endpoint.toString().trim().length() > 0) {


Method should be annotated with CheckForNull

Added annotation

jkfernan · 2020-05-20T19:52:53Z

Any update on this PR? @res0nance Can this be merged?

troymohl · 2020-05-21T14:24:47Z

Update this PR to use the Alternate EC2 Endpoint if configured. I believe this change would also fix using in C2S where the endpoint is not easily derived from the region. I am unable to test this though.

GaToRAiD · 2020-05-26T13:28:21Z

What is the status of this PR? It is quite a pain going into the configuration settings.

GaToRAiD · 2020-05-28T14:11:16Z

@troymohl what is the status on this?

troymohl · 2020-05-28T14:57:49Z

@GaToRAiD
I have tested this on instances that I have access to, as well as rebasing against the master branch. I'm just an outside contributor that needed this bug fixed and wanted to contribute this back for others. I don't have any additional info on when/if this will be accepted into the main baseline.

jkfernan · 2020-06-04T13:59:13Z

@res0nance Can we get a code review on this? It would be great to get it merged. Thanks.

alecharp · 2020-06-18T09:17:44Z

@troymohl can you have a look at the merge conflict? Thanks.

GaToRAiD · 2020-08-24T13:44:44Z

Disregard, looks like it was already fixed in master.

rsandell · 2020-09-30T14:37:50Z

@res0nance @MRamonLeon I am confused about the status of this? @GaToRAiD mentions that it is already fixed in master, but I can't see the ticket mentioned in any release notes and I have users reporting it still being an issue?

MRamonLeon · 2020-10-01T08:20:33Z

@res0nance @MRamonLeon I am confused about the status of this? @GaToRAiD mentions that it is already fixed in master, but I can't see the ticket mentioned in any release notes and I have users reporting it still being an issue?

Maybe because of that @rsandell: #330 Did you mean this @GaToRAiD?

MRamonLeon · 2020-10-23T08:59:51Z

Disregard, looks like it was already fixed in master.

Could you tell us where it was fixed please @GaToRAiD? Thanks

mrmateo · 2020-10-28T15:24:33Z

@MRamonLeon / @rsandell - I think @GaToRAiD might have been referring to an edit in the original comment on this PR from @troymohl :

This has been fixed in master, so that PR was no longer needed.

If so, his comment there was only referring to a prerequisite fix that was needed for this and not the actual issue that this PR fixes. That prerequisite fix was merged and is in master.

@rsandell / @alecharp / @res0nance - Can one of you please take another look and either approve or deny this? It is impacting many users of this plugin so would be good to know if we can count on this fix being made soon or if we need to find other solutions.

Thanks!

res0nance · 2020-11-05T03:59:15Z

@mrmateo This is unfortunately impossible to review due to the conflicts

troymohl · 2020-11-09T17:27:57Z

I have rebased this against the latest master.

batmat · 2020-11-20T09:53:38Z

@mrmateo please do not hesitate to help review this, we welcome any help! :-)

MRamonLeon

Looking into it, please don't merge yet (despite the conflicts)

…-60482-alt-endpoint

MRamonLeon · 2020-11-23T16:29:56Z

@troymohl I've fixed the conflicts with master, but after reviewing the PR, I find we maybe go too far and an simpler approach can be used, if I understood your problem from your description in the Jira ticket. I've filed #537 with this approach where we just ensure the Alternate EC2 endpoint is persisted.

I've recorded a video to show how the behavior is, just in case you or someone else have some confusion about how the configuration page works (it's deserving a good refactoring, let's be honest 😉 )

Let me know if this approach fits your needs.

cc: @jkfernan @GaToRAiD

troymohl · 2020-11-23T16:38:18Z

@MRamonLeon This actually started with a similar change: d200d11

Based on: #439 (comment) I made the additional changes. I'm good with whatever the team decides. We can close this PR if the approach in #537 is preferred.

MRamonLeon · 2020-11-24T07:24:49Z

@MRamonLeon This actually started with a similar change: d200d11

Based on: #439 (comment) I made the additional changes. I'm good with whatever the team decides. We can close this PR if the approach in #537 is preferred.

@troymohl I'm happy merging this PR instead of mine if you come back to your original commit, d200d11

MRamonLeon · 2020-11-24T14:53:15Z

#537 merged. Closing this one.

MRamonLeon · 2020-11-24T14:54:49Z

@troymohl thank you for filing this PR and leveraging the fix of this issue. I look forward to seeing more contributions from you. 👏

troymohl mentioned this pull request Mar 10, 2020

Add ability to reuse existing build hosts backed by EC2 #442

Open

res0nance self-requested a review March 16, 2020 08:10

troymohl force-pushed the JENKINS-60482-alt-endpoint branch from 57ed44e to 8cb7ad2 Compare March 20, 2020 12:02

res0nance reviewed Apr 21, 2020

View reviewed changes

troymohl force-pushed the JENKINS-60482-alt-endpoint branch from 556fb89 to 7460bfd Compare May 21, 2020 14:23

troymohl force-pushed the JENKINS-60482-alt-endpoint branch from 7460bfd to 5a996b8 Compare June 22, 2020 11:57

troymohl added 5 commits November 9, 2020 07:27

Fix saving of EC2 connections using alternate URL configuration

d200d11

Add CheckForNull annotation

79f1cea

Use Alt EC2 Endpoint directly if set to make connections

94d4c8b

Update for rebase against master

ac97955

Cleanup after rebase

3af4126

troymohl force-pushed the JENKINS-60482-alt-endpoint branch from 5a996b8 to 3af4126 Compare November 9, 2020 13:01

batmat requested review from MRamonLeon and res0nance November 20, 2020 09:55

res0nance approved these changes Nov 20, 2020

View reviewed changes

res0nance requested a review from froblesmartin November 20, 2020 15:58

MRamonLeon requested changes Nov 23, 2020

View reviewed changes

Merge branch 'master' of github.com:jenkinsci/ec2-plugin into JENKINS…

b22cf51

…-60482-alt-endpoint

MRamonLeon mentioned this pull request Nov 23, 2020

[JENKINS-60482] Persist the alternate ec2 endpoint #537

Merged

MRamonLeon closed this Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-60482 Alternate Endpoint fix #439

JENKINS-60482 Alternate Endpoint fix #439

troymohl commented Mar 9, 2020 •

edited

Loading

five-iron commented Apr 20, 2020

res0nance Apr 21, 2020

troymohl Apr 24, 2020

res0nance Apr 21, 2020

troymohl Apr 24, 2020

jkfernan commented May 20, 2020 •

edited

Loading

troymohl commented May 21, 2020

GaToRAiD commented May 26, 2020

GaToRAiD commented May 28, 2020

troymohl commented May 28, 2020

jkfernan commented Jun 4, 2020

alecharp commented Jun 18, 2020

GaToRAiD commented Aug 24, 2020 •

edited

Loading

rsandell commented Sep 30, 2020

MRamonLeon commented Oct 1, 2020 •

edited

Loading

MRamonLeon commented Oct 23, 2020

mrmateo commented Oct 28, 2020

res0nance commented Nov 5, 2020

troymohl commented Nov 9, 2020

batmat commented Nov 20, 2020

MRamonLeon left a comment •

edited

Loading

MRamonLeon commented Nov 23, 2020 •

edited

Loading

troymohl commented Nov 23, 2020

MRamonLeon commented Nov 24, 2020

MRamonLeon commented Nov 24, 2020

MRamonLeon commented Nov 24, 2020

JENKINS-60482 Alternate Endpoint fix #439

JENKINS-60482 Alternate Endpoint fix #439

Conversation

troymohl commented Mar 9, 2020 • edited Loading

five-iron commented Apr 20, 2020

res0nance Apr 21, 2020

Choose a reason for hiding this comment

troymohl Apr 24, 2020

Choose a reason for hiding this comment

res0nance Apr 21, 2020

Choose a reason for hiding this comment

troymohl Apr 24, 2020

Choose a reason for hiding this comment

jkfernan commented May 20, 2020 • edited Loading

troymohl commented May 21, 2020

GaToRAiD commented May 26, 2020

GaToRAiD commented May 28, 2020

troymohl commented May 28, 2020

jkfernan commented Jun 4, 2020

alecharp commented Jun 18, 2020

GaToRAiD commented Aug 24, 2020 • edited Loading

rsandell commented Sep 30, 2020

MRamonLeon commented Oct 1, 2020 • edited Loading

MRamonLeon commented Oct 23, 2020

mrmateo commented Oct 28, 2020

res0nance commented Nov 5, 2020

troymohl commented Nov 9, 2020

batmat commented Nov 20, 2020

MRamonLeon left a comment • edited Loading

Choose a reason for hiding this comment

MRamonLeon commented Nov 23, 2020 • edited Loading

troymohl commented Nov 23, 2020

MRamonLeon commented Nov 24, 2020

MRamonLeon commented Nov 24, 2020

MRamonLeon commented Nov 24, 2020

troymohl commented Mar 9, 2020 •

edited

Loading

jkfernan commented May 20, 2020 •

edited

Loading

GaToRAiD commented Aug 24, 2020 •

edited

Loading

MRamonLeon commented Oct 1, 2020 •

edited

Loading

MRamonLeon left a comment •

edited

Loading

MRamonLeon commented Nov 23, 2020 •

edited

Loading