Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3101128202
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -917,11 +917,13 @@ export default {
var providers = svc.provider
providers.forEach(function (provider, providerIndex) {
if (self.forVpc) { // *** vpc ***
- var enabledProviders = ['VpcVirtualRouter', 'Netscaler',
'BigSwitchBcf', 'ConfigDrive']
- if (self.lbType === 'internalLb') {
- enabledProviders.push('InternalLbVm')
+ // For VPC offerings, keep router-specific invalid providers
disabled,
+ // but allow extension/external providers to be selected.
+ if (provider.name === 'InternalLbVm') {
+ provider.enabled = self.lbType === 'internalLb' && svc.name
=== 'Lb'
+ } else {
+ provider.enabled = !['VirtualRouter', 'Nsx',
'Netris'].includes(provider.name)
}
Review Comment:
For VPC offerings, provider enablement was changed from an explicit
allowlist to a broad denylist (`!['VirtualRouter','Nsx','Netris']`). This can
unintentionally enable providers that are not valid for VPC services (i.e.,
providers that appear in `listSupportedNetworkServices` but cannot actually
back VPC offerings), leading to invalid serviceProviderList selections and API
failures later. Consider keeping the previous VPC-capable allowlist and only
additionally enabling dynamically discovered extension providers.
##########
ui/src/views/offering/AddVpcOffering.vue:
##########
@@ -520,82 +520,48 @@ export default {
provider: [{ name: 'VpcVirtualRouter' }]
})
} else {
- services.push({
- name: 'Dhcp',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'ConfigDrive' }
- ]
- })
- services.push({
- name: 'Dns',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'ConfigDrive' }
- ]
- })
- services.push({
- name: 'Lb',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'InternalLbVm' }
- ]
- })
- services.push({
- name: 'Gateway',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'BigSwitchBcf' }
- ]
- })
- services.push({
- name: 'StaticNat',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'BigSwitchBcf' }
- ]
- })
- services.push({
- name: 'SourceNat',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'BigSwitchBcf' }
- ]
- })
- services.push({
- name: 'NetworkACL',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'BigSwitchBcf' }
- ]
- })
- services.push({
- name: 'PortForwarding',
- provider: [{ name: 'VpcVirtualRouter' }]
- })
- services.push({
- name: 'UserData',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'ConfigDrive' }
- ]
- })
- services.push({
- name: 'Vpn',
- provider: [
- { name: 'VpcVirtualRouter' },
- { name: 'BigSwitchBcf' }
- ]
- })
- services.push({
- name: 'Connectivity',
- provider: [
- { name: 'BigSwitchBcf' },
- { name: 'NiciraNvp' },
- { name: 'Ovs' },
- { name: 'JuniperContrailVpcRouter' }
- ]
+ this.supportedServiceLoading = true
+ getAPI('listSupportedNetworkServices').then(json => {
+ const vpcServices = ['Dhcp', 'Dns', 'Lb', 'Gateway', 'StaticNat',
'SourceNat', 'NetworkACL', 'PortForwarding', 'UserData', 'Vpn', 'Connectivity',
'CustomAction']
+ services =
(json?.listsupportednetworkservicesresponse?.networkservice || [])
+ .filter(service => vpcServices.includes(service.name))
+ .map(service => {
+ const providerMap = {}
+ const providers = [...(service.provider || []), ...(service.name
=== 'Lb' ? [{ name: 'InternalLbVm' }] : [])]
+ .map(provider => {
+ const providerName = provider.name === 'VirtualRouter' ?
'VpcVirtualRouter' : provider.name
+ const enabled = providerName === 'InternalLbVm'
+ ? service.name === 'Lb'
+ : !['VirtualRouter', 'Nsx',
'Netris'].includes(providerName)
+ return {
+ name: providerName,
+ description: providerName,
+ enabled
+ }
+ })
+ .filter(provider => {
+ if (providerMap[provider.name]) {
+ return false
+ }
+ providerMap[provider.name] = true
+ return true
+ })
+ return {
+ ...service,
+ description: service.name,
+ provider: providers
+ }
+ })
+
+ this.supportedServices = []
+ if (this.networkmode === 'ROUTED') {
+ services = services.filter(service => !['SourceNat', 'StaticNat',
'Lb', 'PortForwarding', 'Vpn'].includes(service.name))
+ }
+ this.supportedServices = services
+ }).finally(() => {
+ this.supportedServiceLoading = false
})
+ return
Review Comment:
The async `listSupportedNetworkServices` branch doesn’t clear
`supportedServices` until the `then` runs, and it has no `catch`. If the API
call fails, the UI will leave stale `supportedServices` from the previous
provider selection and may also surface an unhandled promise rejection. Clear
`this.supportedServices` before firing the request and add a rejection handler
that resets the list (and ideally notifies the user).
--
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]
