Add a Refresh button in All listing view in each K8s resource

1. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +713,48 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +        $this->messageUser($this->t('Updated @name in @cloudContext.', ['@name' => $entity_type_name_capital_plural, '@cloudContext' => $cloud_context]));
   

   Please use @cloud_context as the internal string identifier.

2. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -136,197 +136,337 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +  public function updateNodeList($cloud_context = '') {
   +    if (empty($cloud_context)) {
   ...
   +    }
   +    else {
   ...
   +    }
   

   ApiController::updateAllEntityList() code can be refactored into ApiController::updateEntityList().

3. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -136,197 +136,337 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +  public function updateDaemonSetList($cloud_context = '') {
   +    if (empty($cloud_context)) {
   +      return $this->updateAllEntityList('ingress', 'ingresses');
   +    }
   +    else {
   +      return $this->updateEntityList('daemon_set', 'daemon_sets', $cloud_context);
   +    }
   

   Could you please double-check this code?

@liuchanggang

Thank you for the update. I tested the patch and it looks good to me from the point view of the functionality, but could you please refactor the status message? See also my comment at #3162214-5.

@liuchanggang

One more thing,

+++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
@@ -573,6 +576,48 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+        $this->messageUser($this->t('Updated @name in @cloud_context.', ['@name' => $entity_type_name_capital_plural, '@cloud_context' => $cloud_context]));

Please change 'Updated @name in ...' to 'Updated @name of ...'.

+++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
@@ -573,6 +578,52 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+          Url::fromUri('internal:/clouds/k8s/' . $cloud_context . '/' . $entity_type_name)

Can you change to:

Url::fromUri("internal:/clouds/k8s/${cloud_context}/${entity_type_name}")

+++ b/modules/cloud_service_providers/k8s/src/Service/K8sBatchOperations.php
@@ -180,7 +182,12 @@ class K8sBatchOperations {
+      \Drupal::messenger()->addWarning(t('Unable to retrieve CPU and Memory usage of nodes of @name. Please install @$metrics_server_link to K8s.', [
+        '@name' => $cloud_config_link,

@liuchanggang

Thank you for updating the patch.

1. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -550,7 +552,10 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      private function updateEntityList($entity_type_name, $entity_type_name_plural, $cloud_context) {
   

   Can we use CloudContentEntityTrait::getDisplayLabels() instead of passing two parameters such as $entity_type_name and $entity_type_name_plural? Currently if the entity name has two words e.g. cron jobs, then cron_jobs is displayed. We expect cron jobs in that case.

2. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -550,7 +552,10 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if (empty($cloud_context)) {
   
   @@ -573,6 +578,59 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +        $this->messageUser($this->t('Updated @resource_link of @cloud_config_link cloud service provider.', ['@resource_link' => $resource_link, '@cloud_config_link' => $cloud_config_link]));
   ...
   +        $this->messageUser($this->t('Unable to update @resource_link of @cloud_config_link cloud service provider.', ['@resource_link' => $resource_link, '@cloud_config_link' => $cloud_config_link]), 'error');
   

   @resource_link and @cloud_config_link should be %resource_link and %cloud_config_link (Bold and Italic).

@liuchanggang

Thank you for updating the patch.

1. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -549,20 +549,25 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    $resource_name = $this->entityTypeManager
   +      ->getStorage('k8s_' . $entity_type_name)
   +      ->getEntityType()
   +      ->getPluralLabel();
   +    $update_method_name = 'update' . str_replace(' ', '', ucwords($resource_name, ' '));
   

   Can you change this code to use CloudContentEntityTrait::getDisplayLabels()? Please refer to the patch at #3162214-11
   and match the variable names as possible.

2. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +578,59 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    $resource_name = $this->entityTypeManager
   +      ->getStorage('k8s_' . $entity_type_name)
   +      ->getEntityType()
   +      ->getPluralLabel();
   +    $update_method_name = 'update' . str_replace(' ', '', ucwords($resource_name, ' '));
   

   Ditto.

3. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +578,59 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      $cloud_config_id = $cloud_config->id();
   +      $cloud_config_link = Link::fromTextAndUrl(
   +        $cloud_config->getName(),
   +        Url::fromUri("internal:/admin/structure/cloud_config/${cloud_config_id}")
   +      )->toString();
   

   We need to validate the permission before adding the link. Please refer to the patch at #3162214-11 and match the variable names as possible.

@liuchanggang

Thank you for updating the patch:

1. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +575,61 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      $account = \Drupal::currentUser();
   +      $permission = 'list k8s ' . str_replace('_', ' ', $entity_type_name);
   +      if ($account->hasPermission($permission)
   ...
   +        $resource_link = Link::fromTextAndUrl(
   +          $labels['plural'],
   +          Url::fromUri("internal:/clouds/k8s/${cloud_context}/${entity_type_name}")
   +        )->toString();
   +      }
   

   Do we need the condition
   $account->hasPermission('view all cloud service providers') in this block? (because this logic handles $resource_link, not handling $cloud_config_link)

2. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +575,61 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      $cloud_config_id = $cloud_config->id();
   +      $cloud_config_link = Link::fromTextAndUrl(
   +        $cloud_config->getName(),
   +        Url::fromUri("internal:/admin/structure/cloud_config/${cloud_config_id}")
   +      )->toString();
   

   We need $account->hasPermission('view all cloud service providers') here but it is not enough to check the permissions. FYI, could you please refer to https://git.drupalcode.org/project/cloud/-/blob/3.x/modules/cloud_servic...?

   I suggest we should do refactoring to move from ApiController::getCloudConfigLink() to CloudConfig::getCloudConfigLink(). (So we need to refactor OpenStack module, too)

@liuchanggang

Thank you for the update.

1. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +575,70 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +        && $account->hasPermission('view all cloud service providers')) {
   

   I don't think we need to have this condition since each resource and cloud service provider are different entities. I guess OpenStack code should be refactored.

2. +++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
   @@ -573,6 +575,70 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      if ($account->hasPermission('view all cloud service providers')
   +        && ($account->hasPermission('view published cloud service providers')
   +        || $account->hasPermission('view own published cloud service providers')
   +        || $account->hasPermission('view unpublished cloud service providers')
   +        || $account->hasPermission('view own unpublished cloud service providers'))) {
   +        $cloud_config_link = $cloud_config->getCloudConfigLink();
   +      }
   

   This can be fine but can you replace these condition to AccessCheckTrait::allowedIfCanAccessCloudConfig() or some other way?

@xiaohua-guan

What do you think?

@yas @liuchanggang

About the permission, I don't think it should be checked in the function updateAllEntityList. I think the permission should be defined in k8s.route.yml.

For example, About the route below,

entity.k8s_node.list_update.all:
  path: '/clouds/k8s/node/update'
  defaults:
    _controller: '\Drupal\k8s\Controller\ApiController::updateNodeList'
  requirements:
    _permission: 'view k8s node'

The permission of the route below can be modified like this if you want to add a new permission to restrict.

  requirements:
    _permission: 'view k8s node,view all cloud service providers'

In addition, the code below is OK, but it will be better to use route to create URL if you consider the possibility of changing URL of a route in the future.

Url::fromUri("internal:/clouds/k8s/${cloud_context}/${entity_type_name}")

TO

Url::fromRoute(
  "view.${entity_type_name}.all",
  [
    'cloud_context' => $cloud_context,
  ]
)

@liuchanggang

Thank you for the update. For the better user experience, I understand that you kept this logic ; that is, if the user does have the permission, the Cloud Orchestrator displays the status message w/ the link to the cloud service provider however I want to double-check if @xiaohua-guan agrees to this code or not.

+++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
@@ -573,6 +577,69 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+      $account = \Drupal::currentUser();
+      if ($this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view all cloud service providers')
+        && ($this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view published cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view own published cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view unpublished cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view own unpublished cloud service providers'))) {
+        $cloud_config_link = $cloud_config->getCloudConfigLink();
+      }

@yas @liuchanggang

Firstly, I think the code like below is not correct.

+    _permission: 'edit k8s network policy+view all cloud service providers'

The code means that somebody has permission "edit k8s network policy" OR "view all cloud service providers", but the relationship should be AND.

I don't think the code below is necessary, because the person refreshing the resources should has the permission "view all cloud service providers" which is defined in k8s.routing.yml.

+++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
@@ -573,6 +577,69 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+      $account = \Drupal::currentUser();
+      if ($this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view all cloud service providers')
+        && ($this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view published cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view own published cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view unpublished cloud service providers')
+        || $this->allowedIfCanAccessCloudConfig($cloud_config, $account, 'view own unpublished cloud service providers'))) {
+        $cloud_config_link = $cloud_config->getCloudConfigLink();
+      }

@liuchanggang

+++ b/modules/cloud_service_providers/k8s/src/Controller/ApiController.php
@@ -573,6 +575,62 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+        K8sService::clearCacheValue();
...
+      K8sService::clearCacheValue();

As we discussed, let's move those two K8sService::clearCacheValue(); to outside of the foreach() loop

I think the patch should be fine in the other portions.

@xiaohua-guan

What do you think?

@yas @liuchanggang

It looks good to me now. So I changed the status to RTBC.

@xiaohua-guan

Thank you for your review. I'll merge the patch to 8.x-2.x and 3.x and close this issue as Fixed.

I'll merge to 8.x-1.x, too for the patch at #3163932

Merged the patch and closing this issue marked as Fixed.