Skip to content
This repository was archived by the owner on Dec 20, 2025. It is now read-only.

fix(core): Always use clusters parameter for ECS server group URLs#10171

Merged
edgarulg merged 2 commits intospinnaker:masterfrom
shlomodaari:fix/servergroup-url-ondemand-ecs
Apr 21, 2025
Merged

fix(core): Always use clusters parameter for ECS server group URLs#10171
edgarulg merged 2 commits intospinnaker:masterfrom
shlomodaari:fix/servergroup-url-ondemand-ecs

Conversation

@shlomodaari
Copy link
Contributor

Fix ECS Server Group URLs When onDemandClusterThreshold is Exceeded

Problem

When viewing ECS server groups in Spinnaker and the onDemandClusterThreshold is exceeded, the generated URLs fail to navigate to the correct server group. This happens because:

  1. ECS provider requires using the clusters parameter for server group navigation
  2. The current implementation uses the standard q, acct, and reg parameters which don't work properly for ECS when on-demand caching is disabled

This results in broken links for ECS server groups in the UI when the number of caches exceeds the threshold.

Solution

This PR modifies the ServerGroupsUrlBuilder.build method to use a different URL format specifically for ECS server groups:

// For ECS provider, use clusters parameter format
if (input.provider === 'ecs' || (input.provider && input.provider.includes('ecs'))) {
// Extract cluster name from server group name (remove version suffix)
const serverGroupParts = input.serverGroup.split('-');
let clusterName = input.serverGroup;
if (serverGroupParts.length > 1 && serverGroupParts[serverGroupParts.length - 1].match(/^v\d+$/)) {
clusterName = serverGroupParts.slice(0, -1).join('-');
}

// Use clusters parameter with account:clusterName format
return UrlBuilderUtils.buildUrl(href, { clusters: ${input.account}:${clusterName} });
}

// For other providers, use standard parameters
return UrlBuilderUtils.buildUrl(href, { q: input.serverGroup, acct: input.account, reg: input.region });

Testing

Testing was performed in an environment where the onDemandClusterThreshold was exceeded.

Before the fix:
URL: /#/applications/myapp/serverGroups?q=my-ecs-service-v001&acct=my-account&reg=us-west-2
Result: Empty view, no server group details displayed

After the fix:
URL: /#/applications/myapp/serverGroups?clusters=my-account:my-ecs-service
Result: Successfully displays the ECS server group details

The fix has been verified with various ECS server group naming patterns, including those with and without version suffixes.

Impact

This change:

  • Fixes navigation to ECS server groups when onDemandClusterThreshold is exceeded
  • Maintains backward compatibility for all other cloud providers
  • Improves user experience by ensuring links work correctly in all scenarios

@christosarvanitis
Copy link
Member

thanks @shlomodaari! I think this bug is not only on ECS though. It must be in the AWS as well (since the ECS cluster view is basically the AWS asg with conventions).
Possibly it is valid for other providers like GCP or Kubernetes.

@shlomodaari
Copy link
Contributor Author

Hi @christosarvanitis,
Yes, I plan to check more providers today before this is merged! I wrote it internally but forgot to mention this as a comment on the PR.

PLEASE don't merge this PR until we confirm other providers!

@shlomodaari
Copy link
Contributor Author

FYI: The good part is that with this fix it should work when onDemandClusterThreshold is exceeded and also when it is disabled

Copy link
Contributor

@edgarulg edgarulg left a comment

Choose a reason for hiding this comment

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

Thanks for working on this fix. The implementation looks good to me but I agree with @christosarvanitis this issue might be present in other providers as well.

I am leaving my approve because the initial issue is being fixed but we need to be sure other providers are not affected the same way.

@shlomodaari
Copy link
Contributor Author

Okay, this is fixing now ECS and AWS!
Need to confirm k8s next!

@edgarulg edgarulg merged commit 3f2418e into spinnaker:master Apr 21, 2025
2 checks passed
@edgarulg
Copy link
Contributor

@mergify backport release-1.38.x release-1.37.x release-1.36.x

@mergify
Copy link
Contributor

mergify bot commented Apr 21, 2025

backport release-1.38.x release-1.37.x release-1.36.x

✅ Backports have been created

Details

@edgarulg edgarulg added ready to merge Reviewed and ready for merge backport-candidate Add to PRs to designate release branch patch candidates. labels Apr 21, 2025
mergify bot pushed a commit that referenced this pull request Apr 21, 2025
…10171)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)
@mergify mergify bot added the auto merged Merged automatically by a bot label Apr 21, 2025
mergify bot pushed a commit that referenced this pull request Apr 21, 2025
…10171)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)
mergify bot pushed a commit that referenced this pull request Apr 21, 2025
…10171)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)
edgarulg pushed a commit that referenced this pull request Apr 21, 2025
…10171) (#10175)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)

Co-authored-by: Shlomo Daari <104773977+shlomodaari@users.noreply.github.com>
edgarulg pushed a commit that referenced this pull request Apr 21, 2025
…10171) (#10174)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)

Co-authored-by: Shlomo Daari <104773977+shlomodaari@users.noreply.github.com>
edgarulg pushed a commit that referenced this pull request Apr 21, 2025
…10171) (#10173)

* fix(core): Always use clusters parameter for ECS server group URLs

* fix(core): Extend server group URL fix to handle both ECS and AWS providers

(cherry picked from commit 3f2418e)

Co-authored-by: Shlomo Daari <104773977+shlomodaari@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto merged Merged automatically by a bot backport-candidate Add to PRs to designate release branch patch candidates. ready to merge Reviewed and ready for merge target-release/1.39

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants