Allow PCUI to support multiple Pcluster versions #418

hgreebe · 2025-05-02T14:17:36Z

Description

Allow PCUI to support managing clusters of different PCUI versions

Changes

BREAKING CHANGE With this change we are removing the support for ParallelCluster versions <= 3.5.0. This is an approved product decision. The technical reason behind this change is that supporting those older versions would have required an increase in complexity that we considered not worth it.
PCUI template accepts a comma separated list of versions to support
An API stack for each pcluster version is launched
API handler manages the mapping of pcluster version to api invoke url
Each API request contains a version parameter in the url
Updating the stack to remove or add support for different pcluster versions is supported
During cluster create the first page is now a version page that includes a dropdown menu to select a version
The official Images page includes a dropdown menu to select which version of images to show

How Has This Been Tested?

Manually Tested the following:
- Create PCUI stack with the version parameter filled in with multiple versions
- Validated that you could see info and modify clusters of the multiple supported versions
- Validated creating clusters of multiple versions
- Validated updating clusters of multiple versions
- Validated view the official images of different versions
- Validated that buttons such as Shell DCV and Stop Fleet work for only the supported versions
- Validated that clusters of unsupported versions are not editable
- VAlidated updating the stack by removing and adding supported versions
Modified unit tests to account for support of multiple versions

In order to increase the likelihood of your contribution being accepted, please make sure you have read both the Contributing Guidelines and the Project Guidelines

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

… stack for each version and handle the mapping of version to invoke url

… page

… load version when creating cluster from an existing cluster

gmarciani · 2025-05-02T16:59:59Z

since this PR is changing the frontend, could you please attach screenshot(s) that represent the most relervant changes?
for example, I'm thinking about the new wizard page for the version

gmarciani · 2025-05-02T17:07:37Z

api/PclusterApiHandler.py

@@ -32,13 +32,14 @@
 USER_POOL_ID = os.getenv("USER_POOL_ID")
 AUTH_PATH = os.getenv("AUTH_PATH")
 API_BASE_URL = os.getenv("API_BASE_URL")
-API_VERSION = os.getenv("API_VERSION", "3.1.0")
+API_VERSION = sorted(os.getenv("API_VERSION", "3.1.0").split(","), key=lambda x: [-int(n) for n in x.split('.')])
+DEFAULT_API_VERSION = API_VERSION[0]


Here we are setting the latest PC version as the default one.
The user should be able to control the default version, but with this solution is not.

Example: a user wants to try a new version of PC, but keeping the default version to the one that they consider the most stable.

Because for the home page where all of the clusters are displayed, it uses the list clusters API of the default version.
Currently, if the supported API version is larger than an unsupport version of one of the clusters, then you will be able to see some of its information, there will just be a warning saying that you can not edit the cluster. But is the supported API version it less than the unsupported version, you won't be able to see any cluster information and will getting an error message saying that the version is not supported.

The reason for the sorting/default version being the largest version is so that for example if the supported versions for 3.13.0 and 3.11.0, you will be able to see the information for a 3.12.0 cluster no matter the order you put in the supported versions.

Got it, thanks for the explanation. Please put a comment explaining why the default version must be the most recent one.

gmarciani · 2025-05-02T17:08:54Z

api/PclusterApiHandler.py

@@ -32,13 +32,14 @@
 USER_POOL_ID = os.getenv("USER_POOL_ID")
 AUTH_PATH = os.getenv("AUTH_PATH")
 API_BASE_URL = os.getenv("API_BASE_URL")
-API_VERSION = os.getenv("API_VERSION", "3.1.0")
+API_VERSION = sorted(os.getenv("API_VERSION", "3.1.0").split(","), key=lambda x: [-int(n) for n in x.split('.')])


what are the implications of this sorting?
Is it used only to determine the default version in the line below?

Also, I suggest to trim blank spaces from the string

I explained the sorting in the comment above

gmarciani · 2025-05-02T17:26:34Z

api/PclusterApiHandler.py

 API_USER_ROLE = os.getenv("API_USER_ROLE")
 OIDC_PROVIDER = os.getenv("OIDC_PROVIDER")
 CLIENT_ID = os.getenv("CLIENT_ID")
 CLIENT_SECRET = os.getenv("CLIENT_SECRET")
 SECRET_ID = os.getenv("SECRET_ID")
-SITE_URL = os.getenv("SITE_URL", API_BASE_URL)
+SITE_URL = os.getenv("SITE_URL")


Should't this be the API URL of the default version?

gmarciani · 2025-05-02T17:29:22Z

api/PclusterApiHandler.py

    else:
-        info_resp = sigv4_request("GET", API_BASE_URL, url)
+        info_resp = sigv4_request("GET", get_base_url(request.args.get("version")), url)


nit: avoid duplication. read the target version ones from the request

gmarciani · 2025-05-02T17:30:05Z

api/PclusterApiHandler.py

@@ -735,14 +744,19 @@ def _get_params(_request):
    params.pop("path")
    return params

+def get_base_url(v):


nit: v -> version

hgreebe · 2025-05-02T17:44:20Z

gmarciani · 2025-05-02T18:34:39Z

api/PclusterApiHandler.py

@@ -62,6 +63,14 @@
 if not JWKS_URL:
    JWKS_URL = os.getenv("JWKS_URL",
                         f"https://cognito-idp.{REGION}.amazonaws.com/{USER_POOL_ID}/" ".well-known/jwks.json")
+API_BASE_URL_MAPPING = {}
+
+if API_BASE_URL:


May you please wrap this logic into a dedicated function and unit test it?

gmarciani · 2025-05-02T18:36:59Z

frontend/locales/en/strings.json

+    "version": {
+      "label": "Cluster Version",
+      "title": "Version",
+      "placeholder": "Select your cluster version",


nit: unnecessayr to repeat "Select your" in a placeholder. Could simply be "Cluster version"

gmarciani · 2025-05-02T18:40:49Z

api/PclusterApiHandler.py

@@ -484,7 +493,7 @@ def get_dcv_session():


 def get_custom_image_config():
-    image_info = sigv4_request("GET", API_BASE_URL, f"/v3/images/custom/{request.args.get('image_id')}").json()
+    image_info = sigv4_request("GET", get_base_url(request.args.get("version")), f"/v3/images/custom/{request.args.get('image_id')}").json()


It's a good practice to specify the request param name version in a constant.

const ARG_VERSION="version" request.args.get(ARG_VERSION)

We did not respect this best practice elsewhere (eg: region), but we can start applying on new code.

gmarciani · 2025-05-02T18:41:18Z

api/PclusterApiHandler.py

@@ -233,9 +242,9 @@ def ec2_action():
 def get_cluster_config_text(cluster_name, region=None):
    url = f"/v3/clusters/{cluster_name}"
    if region:
-        info_resp = sigv4_request("GET", API_BASE_URL, url, params={"region": region})
+        info_resp = sigv4_request("GET", get_base_url(request.args.get("version")), url, params={"region": region})


What if the request is missing the version patameter?

the get_base_url function checks if the version has been passed in

frontend/src/__tests__/ListClusters.test.ts

frontend/src/old-pages/Images/OfficialImages/OfficialImages.tsx

gmarciani · 2025-05-06T13:09:50Z

api/PclusterApiHandler.py

@@ -32,13 +32,14 @@
 USER_POOL_ID = os.getenv("USER_POOL_ID")
 AUTH_PATH = os.getenv("AUTH_PATH")
 API_BASE_URL = os.getenv("API_BASE_URL")
-API_VERSION = os.getenv("API_VERSION", "3.1.0")
+API_VERSION = sorted(os.getenv("API_VERSION", "3.1.0").split(","), key=lambda x: [-int(n) for n in x.split('.')])
+DEFAULT_API_VERSION = API_VERSION[0]


Got it, thanks for the explanation. Please put a comment explaining why the default version must be the most recent one.

infrastructure/environments/demo-cfn-create-args.yaml

gmarciani · 2025-05-06T13:21:40Z

infrastructure/parallelcluster-ui.yaml

@@ -34,7 +35,7 @@ Parameters:
  Version:
    Description: Version of AWS ParallelCluster to deploy.
    Type: String
-    AllowedPattern: "^([0-9]+)\\.([0-9]+)\\.([0-9]+)$"
+    AllowedPattern: "^([0-9]+)\\.([0-9]+)\\.([0-9]+)(,([0-9]+)\\.([0-9]+)\\.([0-9]+))*$"


Is there a limit to the maximum number of versions that a user can specify?

Also, what if a user specifies the same version twice?

if the user specifies the sam number twice than it will just launch the api stack for that version once, because the api handler makes the list of versions a set

there is not maximum

infrastructure/parallelcluster-ui.yaml

gmarciani · 2025-05-06T13:23:31Z

infrastructure/parallelcluster-ui.yaml

@@ -161,17 +162,6 @@ Conditions:
      - !Not [!Equals [!Ref SNSRole, ""]]
  UseNewCognito:
    !Not [ Condition: UseExistingCognito]
-  UseNonDockerizedPCAPI:
-    !Not [ Condition: UseDockerizedPCAPI]
-  UseDockerizedPCAPI: !And


What if a user specifies version <=3.5.0?
This is a breaking change unless you manage <= 3.5.0 in another way (does not seems so).
Can we avoid this change? Why are we forced to introduce it?

gmarciani · 2025-05-06T13:25:14Z

infrastructure/parallelcluster-ui.yaml

+  Fn::ForEach::ParallelClusterApi:
+    - ApiVersion
+    - !Split [",", !Ref Version]
+    - ParallelClusterApi&{ApiVersion}:


Does CFN implicitly transform the dotted version with something else?
I'm asking because I assiume you cannot use dots within a resource name.

When you use the & symbol instead of the # symbol when referencing the variable, it automatically removes the dots

gmarciani · 2025-05-06T13:29:23Z

infrastructure/parallelcluster-ui.yaml

+    - ParallelClusterApi&{ApiVersion}:
+        Type: AWS::CloudFormation::Stack
+        Properties:
+          Parameters:


Why did you remove ImageBuilderSubnetId and ImageBuilderVpcId?

Because if we only support PC version >3.5.0, than all will use the NonDockerizedPCAPI, which has those two variables set to !Ref AWS::NoValue

gmarciani · 2025-05-06T13:32:08Z

infrastructure/parallelcluster-ui.yaml

+              reason = "Failed {}: {}".format(event["RequestType"], e)
+
+      Timeout: 300
+      MemorySize: 128


Seems a very low mem size. What's the maximum mem usage observed across 10 executions? Whatever it is, I suggest to double it. The impact on costs would be very low because this function is executed only on stack create/update/delete, but the potential impact in case of failures is bad.

Mem usage is was 86. I will increase it.

gmarciani · 2025-05-06T13:32:46Z

infrastructure/parallelcluster-ui.yaml

+                Action:
+                  - cloudformation:ListStacks
+                  - cloudformation:DescribeStacks
+                Resource: '*'


Can we scope these permissions down? I think we can set the resource to the current stack name, right?

If we cannot scope this down using stack name, what about scoping this down using tags as per https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-requesttag?

Why leaving * as resource? Isn't it possible to specify the stack arn with a wildcard only on the stackname?

infrastructure/parallelcluster-ui.yaml

gmarciani · 2025-05-22T13:59:09Z

infrastructure/parallelcluster-ui.yaml

-            Resource: !Sub
-              - arn:${AWS::Partition}:execute-api:${AWS::Region}:${AWS::AccountId}:${PCApiGateway}/*/*
-              - { PCApiGateway: !Select [2, !Split ['/', !Select [0, !Split ['.', !GetAtt [ ParallelClusterApi, Outputs.ParallelClusterApiInvokeUrl ]]]]] }
+            Resource: !Sub "arn:${AWS::Partition}:execute-api:${AWS::Region}:${AWS::AccountId}:*/*/*"


This is a security vulnerabikity. We must scope this policy down.
If injecting APIG ARNs does not work b/c of CFN limitations, then we can do it setting conditions on tags https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-resourcetag

This is a working solution to restrict the permissions using tags.

Add tag aws-parallelcluster-ui:stack-id equal to the PCUI stack id to the PCAPI nested stacks.

Add condition
Example:

ParallelClusterApi: Type: AWS::CloudFormation::Stack Properties: ... Tags: - Key: 'aws-parallelcluster-ui:stack-id' Value: !Ref AWS::StackId ParallelClusterApiGatewayInvoke: Type: AWS::IAM::ManagedPolicy Properties: .... PolicyDocument: Version: '2012-10-17' Statement: - Action: - execute-api:Invoke Effect: Allow Resource: !Sub "arn:${AWS::Partition}:execute-api:${AWS::Region}:${AWS::AccountId}:*/*/*" Condition: StringEquals: "aws:ResourceTag/aws-parallelcluster-ui:stack-id": !Ref 'AWS::StackId'

gmarciani · 2025-06-04T14:46:34Z

infrastructure/parallelcluster-ui.yaml

+                                              result = f"{result}{version}={output['OutputValue']},"
+
+                                              parsed_url = urlparse(output['OutputValue']).hostname.split('.')[0]
+                                              api_gateway_arns.append(f"arn:{get_partition(os.environ['AWS_REGION'])}:execute-api:{os.environ['AWS_REGION']}:{account_id}:{parsed_url}/*/*")


To address path traversal vulnerability, we recently made a change that must be reflected here (#422)

gmarciani · 2025-06-05T23:12:29Z

frontend/src/old-pages/Images/OfficialImages/OfficialImages.tsx

+    return (
+        <Select
+            expandToViewport={true}
+            selectedOption={{label: selectedVersion, value: selectedVersion}}


Thanks for adding version selection for images as well. For clusters we added tests to cover this change. What about images?

gmarciani · 2025-06-05T23:16:37Z

infrastructure/parallelcluster-ui.yaml

+                                              gateway = urlparse(output['OutputValue']).hostname.split('.')[0]
+                                              stage = output['OutputValue'].split('/')[3]
+                                              api_gateway_arns.append(f"arn:{get_partition(os.environ['AWS_REGION'])}:execute-api:{os.environ['AWS_REGION']}:{account_id}:{gateway}/{stage}/*")
+                                              print(f"API arn: {parsed_url}")          


what is parsed_url? What about adding the ARN determined on line 325 to the next print and get rid of this?

Sorry, that was leftover from testing.

The arns are listed in the log as the output so I will remove the print.

gmarciani · 2025-06-05T23:20:28Z

infrastructure/parallelcluster-ui.yaml

+                                      for output in stack_response['Stacks'][0]['Outputs']:
+                                          if output['OutputKey'] == 'ParallelClusterApiInvokeUrl':
+                                              # Construct the result string
+                                              result = f"{result}{version}={output['OutputValue']},"


with this concantenation you will end up with a trailing comma at the end of the result. Have you verified this does not have any side effect? Wha about getting rid of that?

It has not side effect because but I will remove the trailing comma

gmarciani · 2025-06-05T23:22:29Z

infrastructure/parallelcluster-ui.yaml

+                                      for output in stack_response['Stacks'][0]['Outputs']:
+                                          if output['OutputKey'] == 'ParallelClusterApiInvokeUrl':
+                                              # Construct the result string
+                                              result = f"{result}{version}={output['OutputValue']},"


codestyle: output['OutoputValue'] is repeated 4 times. what about improving readability assigning it to a more self explanatory variable api_url instead?

gmarciani · 2025-06-05T23:23:53Z

infrastructure/parallelcluster-ui.yaml

+              reason = "Failed {}: {}".format(event["RequestType"], e)
+
+      Timeout: 300
+      MemorySize: 256


I don;'t remember if we already discussed this topic. However, what is the maximum amount of memory consumed that you observed. Is 256 way enough?

Mem usage is was 86. so i think 256 should be more than enough.

gmarciani · 2025-06-05T23:25:30Z

infrastructure/parallelcluster-ui.yaml

+                Action:
+                  - cloudformation:ListStacks
+                  - cloudformation:DescribeStacks
+                Resource: '*'


If we cannot scope this down using stack name, what about scoping this down using tags as per https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-requesttag?

gmarciani

This Pr contains an approved breaking change: it breaks the support for PC <= 3.5.0. Let's highlight this in the PR description.

gmarciani · 2025-06-16T12:56:36Z

infrastructure/parallelcluster-ui.yaml

+                  - !Sub "arn:${AWS::Partition}:cloudformation:${AWS::Region}:${AWS::AccountId}:stack/*"
+                Condition:
+                  StringEquals:
+                    "aws:ResourceTag/parallelcluster:api-id": !Ref ApiGatewayRestApi


Where is this tag set?

line 217 or parallelcluster-ui.yaml

gmarciani · 2025-06-16T13:14:59Z

infrastructure/parallelcluster-ui.yaml

+          TemplateURL: !Sub https://${AWS::Region}-aws-parallelcluster.s3.${AWS::Region}.amazonaws.com/parallelcluster/${ApiVersion}/api/parallelcluster-api.yaml
+          TimeoutInMinutes: 30
+          Tags:
+            - Key: 'parallelcluster:api-id'


The tag name is misleading as it seems to refer to ParallelCluster API id, wheres the id we are injecting here is the ID of ParallelCluster UI API. Can you please rename the tag to parallelcluster-ui:api-id?

hgreebe added 10 commits May 1, 2025 11:07

Modify cloudformation template and api handler to create multiple api…

9865410

… stack for each version and handle the mapping of version to invoke url

Add version field to cluster create

f84a92d

Add dropdown menu to select the image version for the Official Images…

96dfef4

… page

Add a version page to create cluster with a version dropdown menu and…

d5a1384

… load version when creating cluster from an existing cluster

Fix location of the version dropdown menu on the official images page

9461617

Modify unit tests to account for multiple pcluster versions

897b3a5

Modify demo environment to work with multiple pcluster versions

98ed239

Fix cluster update

ff49a2b

fix frontend tests

760614a

Use set for version comparison

58eaa88

hgreebe changed the title ~~Develop~~ Allow PCUI to support multiple Pcluster versions May 2, 2025

hgreebe added 2 commits May 2, 2025 11:17

Add version field test

7d0ce72

Add backend test for invoke url mapping

709b5d6

hgreebe force-pushed the develop branch from c07e130 to 709b5d6 Compare May 2, 2025 15:40

Fix official images version dropdown menu

a1f5c23

hgreebe marked this pull request as ready for review May 2, 2025 15:54

gmarciani reviewed May 2, 2025

View reviewed changes

frontend/src/__tests__/ListClusters.test.ts Show resolved Hide resolved

gmarciani reviewed May 2, 2025

View reviewed changes

frontend/src/old-pages/Images/OfficialImages/OfficialImages.tsx Show resolved Hide resolved

hgreebe force-pushed the develop branch from 44ce3b4 to 31ea129 Compare May 5, 2025 17:09

Wrap url mapping logic in a function

4199108

hgreebe force-pushed the develop branch from 31ea129 to 4199108 Compare May 5, 2025 17:16

gmarciani reviewed May 6, 2025

View reviewed changes

gmarciani reviewed May 8, 2025

View reviewed changes

infrastructure/parallelcluster-ui.yaml Show resolved Hide resolved

gmarciani reviewed May 8, 2025

View reviewed changes

infrastructure/parallelcluster-ui.yaml Show resolved Hide resolved

hgreebe added 2 commits May 13, 2025 11:08

Add cluster version selection to custom image build.

40fd88c

fix create cluster from template

5529745

gmarciani reviewed May 22, 2025

View reviewed changes

hgreebe added 4 commits June 3, 2025 13:21

Scope down permissions

6d8dcb8

Fix formatting

5f5ae6f

Handle duplicate versions

d8169b1

Fix create version issue

c9e5652

gmarciani reviewed Jun 4, 2025

View reviewed changes

hgreebe and others added 2 commits June 4, 2025 12:07

Account for fix of path traversal vulnerability

aab0f60

Merge branch 'main' into develop

ac8981a

gmarciani reviewed Jun 5, 2025

View reviewed changes

hgreebe added 2 commits June 8, 2025 14:32

Add images unit tests

52a3ef6

Scope down ApiVersionMap permissions

2170bb6

hgreebe force-pushed the develop branch from bc75eb1 to 2170bb6 Compare June 9, 2025 23:06

gmarciani reviewed Jun 16, 2025

View reviewed changes

Change ui api id tag

94f9775

gmarciani approved these changes Jun 16, 2025

View reviewed changes

hgreebe merged commit eec445d into aws:main Jun 16, 2025
2 checks passed

Allow PCUI to support multiple Pcluster versions #418

Allow PCUI to support multiple Pcluster versions #418

Uh oh!

Conversation

hgreebe commented May 2, 2025 • edited by gmarciani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How Has This Been Tested?

Uh oh!

gmarciani commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hgreebe commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarciani May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hgreebe commented May 2, 2025 •

edited by gmarciani

Loading

gmarciani May 6, 2025 •

edited

Loading