Skip to content

Commit eec445d

Browse files
hgreebehgreebe
authored
Allow PCUI to support multiple Pcluster versions (#418)
* Modify cloudformation template and api handler to create multiple api stack for each version and handle the mapping of version to invoke url * Add version field to cluster create * Add dropdown menu to select the image version for the Official Images page * Add a version page to create cluster with a version dropdown menu and load version when creating cluster from an existing cluster * Fix location of the version dropdown menu on the official images page * Modify unit tests to account for multiple pcluster versions * Modify demo environment to work with multiple pcluster versions * Fix cluster update * fix frontend tests * Use set for version comparison * Add version field test * Add backend test for invoke url mapping * Fix official images version dropdown menu * Wrap url mapping logic in a function * Add cluster version selection to custom image build. * fix create cluster from template * Scope down permissions * Fix formatting * Handle duplicate versions * Fix create version issue * Account for fix of path traversal vulnerability * Add images unit tests * Scope down ApiVersionMap permissions * Change ui api id tag
1 parent 5703ff9 commit eec445d

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

43 files changed

+1211
-211
lines changed

‎api/PclusterApiHandler.py‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,14 @@
3232
USER_POOL_ID = os.getenv("USER_POOL_ID")
3333
AUTH_PATH = os.getenv("AUTH_PATH")
3434
API_BASE_URL = os.getenv("API_BASE_URL")
35-
API_VERSION = os.getenv("API_VERSION", "3.1.0")
35+
API_VERSION = sorted(set(os.getenv("API_VERSION", "3.1.0").strip().split(",")), key=lambda x: [-int(n) for n in x.split('.')])
36+
# Default version must be highest version so that it can be used for read operations due to backwards compatibility
37+
DEFAULT_API_VERSION = API_VERSION[0]
3638
API_USER_ROLE = os.getenv("API_USER_ROLE")
3739
OIDC_PROVIDER = os.getenv("OIDC_PROVIDER")
3840
CLIENT_ID = os.getenv("CLIENT_ID")
3941
CLIENT_SECRET = os.getenv("CLIENT_SECRET")
4042
SECRET_ID = os.getenv("SECRET_ID")
41-
SITE_URL = os.getenv("SITE_URL", API_BASE_URL)
4243
SCOPES_LIST = os.getenv("SCOPES_LIST")
4344
REGION = os.getenv("AWS_DEFAULT_REGION")
4445
TOKEN_URL = os.getenv("TOKEN_URL", f"{AUTH_PATH}/oauth2/token")
@@ -48,6 +49,7 @@
4849
AUDIENCE = os.getenv("AUDIENCE")
4950
USER_ROLES_CLAIM = os.getenv("USER_ROLES_CLAIM", "cognito:groups")
5051
SSM_LOG_GROUP_NAME = os.getenv("SSM_LOG_GROUP_NAME")
52+
ARG_VERSION="version"
5153

5254
try:
5355
if (not USER_POOL_ID or USER_POOL_ID == "") and SECRET_ID:
@@ -63,6 +65,19 @@
6365
JWKS_URL = os.getenv("JWKS_URL",
6466
f"https://cognito-idp.{REGION}.amazonaws.com/{USER_POOL_ID}/" ".well-known/jwks.json")
6567

68+
def create_url_map(url_list):
69+
url_map = {}
70+
if url_list:
71+
for url in url_list.split(","):
72+
if url:
73+
pair=url.split("=")
74+
url_map[pair[0]] = pair[1]
75+
return url_map
76+
77+
API_BASE_URL_MAPPING = create_url_map(API_BASE_URL)
78+
SITE_URL = os.getenv("SITE_URL", API_BASE_URL_MAPPING.get(DEFAULT_API_VERSION))
79+
80+
6681

6782
def jwt_decode(token, audience=None, access_token=None):
6883
return jwt.decode(
@@ -165,7 +180,7 @@ def authenticate(groups):
165180

166181
if (not groups):
167182
return abort(403)
168-
183+
169184
jwt_roles = set(decoded.get(USER_ROLES_CLAIM, []))
170185
groups_granted = groups.intersection(jwt_roles)
171186
if len(groups_granted) == 0:
@@ -191,7 +206,7 @@ def get_scopes_list():
191206

192207
def get_redirect_uri():
193208
return f"{SITE_URL}/login"
194-
209+
195210
# Local Endpoints
196211

197212

@@ -233,9 +248,9 @@ def ec2_action():
233248
def get_cluster_config_text(cluster_name, region=None):
234249
url = f"/v3/clusters/{cluster_name}"
235250
if region:
236-
info_resp = sigv4_request("GET", API_BASE_URL, url, params={"region": region})
251+
info_resp = sigv4_request("GET", get_base_url(request), url, params={"region": region})
237252
else:
238-
info_resp = sigv4_request("GET", API_BASE_URL, url)
253+
info_resp = sigv4_request("GET", get_base_url(request), url)
239254
if info_resp.status_code != 200:
240255
abort(info_resp.status_code)
241256

@@ -365,7 +380,7 @@ def sacct():
365380
user,
366381
f"sacct {sacct_args} --json "
367382
+ "| jq -c .jobs[0:120]\\|\\map\\({name,user,partition,state,job_id,exit_code\\}\\)",
368-
)
383+
)
369384
if type(accounting) is tuple:
370385
return accounting
371386
else:
@@ -484,7 +499,7 @@ def get_dcv_session():
484499

485500

486501
def get_custom_image_config():
487-
image_info = sigv4_request("GET", API_BASE_URL, f"/v3/images/custom/{request.args.get('image_id')}").json()
502+
image_info = sigv4_request("GET", get_base_url(request), f"/v3/images/custom/{request.args.get('image_id')}").json()
488503
configuration = requests.get(image_info["imageConfiguration"]["url"])
489504
return configuration.text
490505

@@ -596,9 +611,9 @@ def _get_identity_from_token(decoded, claims):
596611
identity["username"] = decoded["username"]
597612

598613
for claim in claims:
599-
if claim in decoded:
600-
identity["attributes"][claim] = decoded[claim]
601-
614+
if claim in decoded:
615+
identity["attributes"][claim] = decoded[claim]
616+
602617
return identity
603618

604619
def get_identity():
@@ -735,14 +750,20 @@ def _get_params(_request):
735750
params.pop("path")
736751
return params
737752

753+
def get_base_url(request):
754+
version = request.args.get(ARG_VERSION)
755+
if version and str(version) in API_VERSION:
756+
return API_BASE_URL_MAPPING[str(version)]
757+
return API_BASE_URL_MAPPING[DEFAULT_API_VERSION]
758+
738759

739760
pc = Blueprint('pc', __name__)
740761

741762
@pc.get('/', strict_slashes=False)
742763
@authenticated({'admin'})
743764
@validated(params=PCProxyArgs)
744765
def pc_proxy_get():
745-
response = sigv4_request(request.method, API_BASE_URL, request.args.get("path"), _get_params(request))
766+
response = sigv4_request(request.method, get_base_url(request), request.args.get("path"), _get_params(request))
746767
return response.json(), response.status_code
747768

748769
@pc.route('/', methods=['POST','PUT','PATCH','DELETE'], strict_slashes=False)
@@ -756,5 +777,5 @@ def pc_proxy():
756777
except:
757778
pass
758779

759-
response = sigv4_request(request.method, API_BASE_URL, request.args.get("path"), _get_params(request), body=body)
780+
response = sigv4_request(request.method, get_base_url(request), request.args.get("path"), _get_params(request), body=body)
760781
return response.json(), response.status_code

‎api/tests/test_pcluster_api_handler.py‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
from unittest import mock
2-
from api.PclusterApiHandler import login
2+
from api.PclusterApiHandler import login, get_base_url, create_url_map
3+
4+
class MockRequest:
5+
cookies = {'int_value': 100}
6+
args = {'version': '3.12.0'}
7+
json = {'username': 'user@email.com'}
38

49

510
@mock.patch("api.PclusterApiHandler.requests.post")
@@ -27,3 +32,14 @@ def test_login_with_no_access_token_returns_401(mocker, app):
2732
login()
2833

2934
mock_abort.assert_called_once_with(401)
35+
36+
def test_get_base_url(monkeypatch):
37+
monkeypatch.setattr('api.PclusterApiHandler.API_VERSION', ['3.12.0', '3.11.0'])
38+
monkeypatch.setattr('api.PclusterApiHandler.API_BASE_URL', '3.12.0=https://example.com,3.11.0=https://example1.com,')
39+
monkeypatch.setattr('api.PclusterApiHandler.API_BASE_URL_MAPPING', {'3.12.0': 'https://example.com', '3.11.0': 'https://example1.com'})
40+
41+
assert 'https://example.com' == get_base_url(MockRequest())
42+
43+
def test_create_url_map():
44+
assert {'3.12.0': 'https://example.com', '3.11.0': 'https://example1.com'} == create_url_map('3.12.0=https://example.com,3.11.0=https://example1.com,')
45+

‎frontend/locales/en/strings.json‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@
101101
"ariaLabel": "Info"
102102
},
103103
"cluster": {
104-
"editAlert": "This cluster cannot be edited as it was created using a different version of AWS ParallelCluster.",
104+
"editAlert": "This cluster cannot be edited as it was created using an incompatible version of AWS ParallelCluster.",
105105
"tabs": {
106106
"details": "Details",
107107
"instances": "Instances",
@@ -493,6 +493,18 @@
493493
"collapsedStepsLabel": "Step {{stepNumber}} of {{stepsCount}}",
494494
"steps": "Steps"
495495
},
496+
"version": {
497+
"label": "Cluster Version",
498+
"title": "Version",
499+
"placeholder": "Cluster version",
500+
"description": "Select the AWS ParallelCluster version to use for this cluster.",
501+
"help": {
502+
"main": "Choose the version of AWS ParallelCluster to use for creating and managing your cluster."
503+
},
504+
"validation": {
505+
"versionSelect": "You must select a version."
506+
}
507+
},
496508
"cluster": {
497509
"title": "Cluster",
498510
"description": "Configure the settings that apply to all cluster resources.",
@@ -1407,6 +1419,8 @@
14071419
},
14081420
"dialogs": {
14091421
"buildImage": {
1422+
"versionLabel": "Version",
1423+
"versionPlaceholder": "Select version",
14101424
"closeAriaLabel": "Close",
14111425
"title": "Image configuration",
14121426
"cancel": "Cancel",
@@ -1430,6 +1444,14 @@
14301444
"href": "https://docs.aws.amazon.com/parallelcluster/latest/ug/support-policy.html"
14311445
}
14321446
},
1447+
"actions": {
1448+
"versionSelect": {
1449+
"selectedAriaLabel": "Selected version",
1450+
"versionText": "Version {{version}}",
1451+
"placeholder": "Select a version"
1452+
},
1453+
"refresh": "Refresh"
1454+
},
14331455
"list": {
14341456
"columns": {
14351457
"id": "ID",

‎frontend/src/__tests__/CreateCluster.test.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const mockRequest = executeRequest as jest.Mock
88

99
describe('given a CreateCluster command and a cluster configuration', () => {
1010
const clusterName = 'any-name'
11+
const clusterVersion = 'some-version'
1112
const clusterConfiguration = 'Imds:\n ImdsSupport: v2.0'
1213
const mockRegion = 'some-region'
1314
const mockSelectedRegion = 'some-region'
@@ -37,11 +38,12 @@ describe('given a CreateCluster command and a cluster configuration', () => {
3738
clusterConfiguration,
3839
mockRegion,
3940
mockSelectedRegion,
41+
clusterVersion,
4042
)
4143
expect(mockRequest).toHaveBeenCalledTimes(1)
4244
expect(mockRequest).toHaveBeenCalledWith(
4345
'post',
44-
'api?path=/v3/clusters&region=some-region',
46+
'api?path=/v3/clusters&region=some-region&version=some-version',
4547
expectedBody,
4648
expect.any(Object),
4749
expect.any(Object),
@@ -55,6 +57,7 @@ describe('given a CreateCluster command and a cluster configuration', () => {
5557
clusterConfiguration,
5658
mockRegion,
5759
mockSelectedRegion,
60+
clusterVersion,
5861
false,
5962
mockSuccessCallback,
6063
)
@@ -71,12 +74,13 @@ describe('given a CreateCluster command and a cluster configuration', () => {
7174
clusterConfiguration,
7275
mockRegion,
7376
mockSelectedRegion,
77+
clusterVersion,
7478
mockDryRun,
7579
)
7680
expect(mockRequest).toHaveBeenCalledTimes(1)
7781
expect(mockRequest).toHaveBeenCalledWith(
7882
'post',
79-
'api?path=/v3/clusters&dryrun=true&region=some-region',
83+
'api?path=/v3/clusters&dryrun=true&region=some-region&version=some-version',
8084
expect.any(Object),
8185
expect.any(Object),
8286
expect.any(Object),
@@ -106,6 +110,7 @@ describe('given a CreateCluster command and a cluster configuration', () => {
106110
clusterConfiguration,
107111
mockRegion,
108112
mockSelectedRegion,
113+
clusterVersion,
109114
false,
110115
undefined,
111116
mockErrorCallback,
@@ -128,6 +133,7 @@ describe('given a CreateCluster command and a cluster configuration', () => {
128133
'Imds:\n ImdsSupport: v2.0',
129134
mockRegion,
130135
mockSelectedRegion,
136+
clusterVersion,
131137
)
132138

133139
expect(mockRequest).toHaveBeenCalledWith(
@@ -154,6 +160,7 @@ describe('given a CreateCluster command and a cluster configuration', () => {
154160
'Imds:\n ImdsSupport: v2.0\nTags:\n - Key: foo\n Value: bar',
155161
mockRegion,
156162
mockSelectedRegion,
163+
clusterVersion,
157164
)
158165

159166
expect(mockRequest).toHaveBeenCalledWith(
@@ -180,6 +187,7 @@ describe('given a CreateCluster command and a cluster configuration', () => {
180187
"Imds:\n ImdsSupport: v2.0\nTags:\n - Key: parallelcluster-ui\n Value: 'true'",
181188
mockRegion,
182189
mockSelectedRegion,
190+
clusterVersion,
183191
)
184192

185193
expect(mockRequest).not.toHaveBeenCalledWith(

‎frontend/src/__tests__/GetVersion.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ describe('given a GetVersion command', () => {
3030
describe('when the PC version can be retrieved successfully', () => {
3131
beforeEach(() => {
3232
const mockResponse = {
33-
version: '3.5.0',
33+
version: ['3.5.0', '3.6.0'],
3434
}
3535

3636
mockGet.mockResolvedValueOnce({data: mockResponse})
@@ -39,13 +39,13 @@ describe('given a GetVersion command', () => {
3939
it('should return the PC version', async () => {
4040
const data = await GetVersion()
4141

42-
expect(data).toEqual<PCVersion>({full: '3.5.0'})
42+
expect(data).toEqual<PCVersion>({full: ['3.5.0', '3.6.0']})
4343
})
4444

4545
it('should store the PC version', async () => {
4646
await GetVersion()
4747

48-
expect(setState).toHaveBeenCalledWith(['app', 'version'], {full: '3.5.0'})
48+
expect(setState).toHaveBeenCalledWith(['app', 'version'], {full: ['3.5.0', '3.6.0']})
4949
})
5050
})
5151

‎frontend/src/__tests__/ListClusters.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ const mockCluster1: ClusterInfoSummary = {
1414
const mockCluster2: ClusterInfoSummary = {
1515
clusterName: 'test-cluster-2',
1616
clusterStatus: ClusterStatus.CreateComplete,
17-
version: '3.8.0',
17+
version: '3.9.0',
1818
cloudformationStackArn: 'arn',
1919
region: 'region',
2020
cloudformationStackStatus: CloudFormationStackStatus.CreateComplete,

‎frontend/src/components/__tests__/useLoadingState.test.tsx‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ describe('given a hook to load all the data necessary for the app to boot', () =
7676
someKey: 'some-value',
7777
},
7878
app: {
79-
version: {full: '3.5.0'},
79+
version: {full: ['3.5.0', '3.7.0']},
8080
appConfig: {
8181
someKey: 'some-value',
8282
},
@@ -109,7 +109,7 @@ describe('given a hook to load all the data necessary for the app to boot', () =
109109
mockStore.getState.mockReturnValue({
110110
identity: null,
111111
app: {
112-
version: {full: '3.5.0'},
112+
wizard: {version: '3.5.0'},
113113
appConfig: {
114114
someKey: 'some-value',
115115
},
@@ -131,7 +131,9 @@ describe('given a hook to load all the data necessary for the app to boot', () =
131131
someKey: 'some-value',
132132
},
133133
app: {
134-
version: {full: '3.5.0'},
134+
wizard: {
135+
version: '3.5.0',
136+
},
135137
appConfig: null,
136138
},
137139
})

‎frontend/src/feature-flags/useFeatureFlag.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {featureFlagsProvider} from './featureFlagsProvider'
1313
import {AvailableFeature} from './types'
1414

1515
export function useFeatureFlag(feature: AvailableFeature): boolean {
16-
const version = useState(['app', 'version', 'full'])
16+
const version = useState(['app', 'wizard', 'version'])
1717
const region = useState(['aws', 'region'])
1818
return isFeatureEnabled(version, region, feature)
1919
}

0 commit comments

Comments
 (0)