Copilot commented on code in PR #13032: URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3095010598
########## ui/src/views/extension/UpdateRegisteredExtension.vue: ########## @@ -0,0 +1,131 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +<template> + <div class="form-layout" v-ctrl-enter="handleSubmit"> + <a-form + :ref="formRef" + :model="form" + :loading="loading" + layout="vertical" + @finish="handleSubmit"> + <a-form-item name="details" ref="details"> + <template #label> + <tooltip-label :title="$t('label.details')" :tooltip="$t('message.add.extension.resource.details')"/> + </template> + <div style="margin-bottom: 10px">{{ $t('message.add.extension.resource.details') }}</div> + <details-input + v-model:value="form.details" /> + </a-form-item> + <div :span="24" class="action-button"> + <a-button @click="closeAction">{{ $t('label.cancel') }}</a-button> + <a-button :loading="loading" ref="submit" type="primary" @click="handleSubmit">{{ $t('label.ok') }}</a-button> + </div> + </a-form> + </div> +</template> + +<script> +import { ref, reactive, toRaw } from 'vue' +import { postAPI } from '@/api' +import TooltipLabel from '@/components/widgets/TooltipLabel' +import DetailsInput from '@/components/widgets/DetailsInput' + +export default { + name: 'UpdateRegisteredExtension', + components: { + TooltipLabel, + DetailsInput + }, + props: { + resource: { + type: Object, + required: true + }, + extensionResource: { + type: Object, + required: true + } + }, + data () { + return { + loading: false + } + }, + created () { + this.initForm() + }, + methods: { + initForm () { + this.formRef = ref() + this.form = reactive({ + details: this.extensionResource.details ? { ...this.extensionResource.details } : {} + }) + }, + handleSubmit (e) { + e.preventDefault() + if (this.loading) return + this.formRef.value.validate().then(() => { Review Comment: `handleSubmit` unconditionally calls `e.preventDefault()`, but this handler is also bound to `<a-form @finish="handleSubmit">`, where Ant Design Vue typically passes the validated form values (not a DOM event). This can throw at runtime (e.g. when submitting via Enter). Consider guarding with `if (e?.preventDefault) e.preventDefault()` (or splitting into separate handlers for click vs. finish). ########## engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java: ########## @@ -461,6 +469,28 @@ public void setDhcpProviders(final List<DhcpServiceProvider> dhcpProviders) { HashMap<Long, Long> _lastNetworkIdsToFree = new HashMap<>(); + /** + * Returns the full list of network elements to iterate when implementing, + * shutting down, or otherwise orchestrating a network. + * + * <p>The base list ({@link #networkElements}, wired by Spring) is extended + * at runtime with one transient {@link NetworkExtensionElement} per + * registered {@code NetworkOrchestrator} extension. This keeps the + * Spring bean list free from {@code NetworkExtensionElement} and allows + * dynamic discovery of extensions without a restart.</p> + */ + private List<NetworkElement> getNetworkElementsIncludingExtensions() { + List<Extension> extensions = extensionHelper.listExtensionsByType(Extension.Type.NetworkOrchestrator); + if (extensions == null || extensions.isEmpty()) { + return networkElements; + } + List<NetworkElement> combined = new ArrayList<>(networkElements); + for (Extension ext : extensions) { + combined.add(networkExtensionElement.withProviderName(ext.getName())); + } + return combined; + } Review Comment: `getNetworkElementsIncludingExtensions()` calls `extensionHelper.listExtensionsByType(...)` every time it’s invoked, but callers in this class now invoke it repeatedly within the same orchestration flow (multiple loops). This can translate into repeated DB/API work and inconsistent snapshots if extensions change mid-operation. Consider caching the combined list once per high-level operation (e.g. compute a local `elements` list and reuse it across loops) or adding an internal memoization with a short TTL. ########## ui/src/views/offering/AddNetworkOffering.vue: ########## @@ -732,6 +744,30 @@ export default { this.fetchServiceOfferingData() this.fetchIpv6NetworkOfferingConfiguration() this.fetchRoutedNetworkConfiguration() + this.fetchExtensionProviders() + }, + fetchExtensionProviders () { + // Load NetworkOrchestrator extensions that are registered to at least one + // physical network (i.e. have a corresponding NetworkServiceProvider entry). + // Only these can be selected as a provider when creating a network offering. + getAPI('listExtensions', { type: 'NetworkOrchestrator', state: 'Enabled' }).then(json => { + const allExts = (json.listextensionsresponse && json.listextensionsresponse.extension) || [] + if (allExts.length === 0) { + this.availableExtensionProviders = [] + return + } + // Filter to those which have at least one matching NSP (nsp name == extension name) + getAPI('listNetworkServiceProviders', {}).then(nspJson => { + const nsps = (nspJson.listnetworkserviceprovidersresponse && nspJson.listnetworkserviceprovidersresponse.networkserviceprovider) || [] + const nspNames = new Set(nsps.map(n => n.name)) + this.availableExtensionProviders = allExts.filter(e => nspNames.has(e.name)) + }).catch(() => { + // Fallback: show all enabled extensions + this.availableExtensionProviders = allExts + }) + }).catch(() => { + this.availableExtensionProviders = [] + }) Review Comment: `fetchExtensionProviders()` populates `availableExtensionProviders`, but the provider `<a-select>` options remain hardcoded (NSX/Netris). As a result, users cannot actually pick an extension-backed provider here, and `isExternalNetworkProvider`/`externalNetworkSupportedServicesMap` logic will never be exercised from the UI. Consider adding `<a-select-option>` entries for `availableExtensionProviders` (or otherwise wiring this list into the provider picker) and ensuring the selected value maps to the extension/NSP name expected by the API. ########## ui/src/views/extension/ExtensionResourcesTab.vue: ########## @@ -34,6 +34,13 @@ {{ text && $toLocaleDate(text) }} </template> <template v-if="column.key === 'actions'"> + <span style="margin-right: 5px"> + <tooltip-button + :tooltip="$t('label.action.update.extension.resource')" + type="default" + icon="edit-outlined" + @onClick="openUpdateModal(record)" /> + </span> Review Comment: The Update action is always shown, but the underlying API (`updateRegisteredExtension`) may not be available/enabled in all deployments (similar to how Unregister is guarded). Consider adding a `v-if` guard (e.g. `'updateRegisteredExtension' in $store.getters.apis`) so the UI doesn’t present a non-functional action. ########## ui/src/views/offering/AddVpcOffering.vue: ########## @@ -384,6 +397,25 @@ export default { this.fetchSupportedServiceData() this.fetchIpv6NetworkOfferingConfiguration() this.fetchRoutedNetworkConfiguration() + this.fetchExtensionProviders() + }, + fetchExtensionProviders () { + getAPI('listExtensions', { type: 'NetworkOrchestrator', state: 'Enabled' }).then(json => { + const allExts = (json.listextensionsresponse && json.listextensionsresponse.extension) || [] + if (allExts.length === 0) { + this.availableExtensionProviders = [] + return + } + getAPI('listNetworkServiceProviders', {}).then(nspJson => { + const nsps = (nspJson.listnetworkserviceprovidersresponse && nspJson.listnetworkserviceprovidersresponse.networkserviceprovider) || [] + const nspNames = new Set(nsps.map(n => n.name)) + this.availableExtensionProviders = allExts.filter(e => nspNames.has(e.name)) + }).catch(() => { + this.availableExtensionProviders = allExts + }) + }).catch(() => { + this.availableExtensionProviders = [] + }) Review Comment: `fetchExtensionProviders()` loads `availableExtensionProviders`, but there’s no corresponding UI wiring for selecting one of these extensions as the VPC offering provider (the provider select appears to be NSX/Netris-only). This makes `isExternalNetworkProvider` and `_buildExternalVpcServiceMap()` effectively unreachable from the UI. Consider adding provider `<a-select-option>` entries for `availableExtensionProviders` (or another selection mechanism) so the chosen provider name matches the extension/NSP name. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
