cloudrun: GKE client to cloudrun server test #154
Conversation
bin/run_test_server_c6n.py
Outdated
| # Help. | ||
| ./run.sh ./bin/run_test_server_cloud_run.py --help | ||
|
|
||
| # Run modes. |
There was a problem hiding this comment.
Remove these since we are not taking any args. Only have a single run mode without any arg.
Rename the file was well to have cloud_run instead of c6n.
| @@ -0,0 +1,102 @@ | |||
| # Copyright 2024 gRPC authors. | |||
| @@ -651,6 +736,8 @@ def _execute( # pylint: disable=arguments-differ | |||
| ) | |||
| request.headers[DEBUG_HEADER_KEY] = self.gfe_debug_header | |||
| request.add_response_callback(self._log_debug_header) | |||
| logger.info("Executing request: %s", request) | |||
| logger.info(request.to_json()) | |||
There was a problem hiding this comment.
Remove this logging, it seems not really related to our changes.
| logger.info("backend service protocol") | ||
| logger.info(self.backend_service_protocol) |
| MESH_NAME = "grpc-mesh" | ||
| SERVER_TLS_POLICY_NAME = "server-tls-policy" | ||
| CLIENT_TLS_POLICY_NAME = "client-tls-policy" | ||
| AUTHZ_POLICY_NAME = "authz-policy" | ||
| ENDPOINT_POLICY = "endpoint-policy" | ||
| CERTIFICATE_PROVIDER_INSTANCE = "google_cloud_private_spiffe" |
| # Managed resources | ||
| self.server_tls_policy: Optional[ServerTlsPolicy] = None | ||
| self.client_tls_policy: Optional[ClientTlsPolicy] = None | ||
| self.authz_policy: Optional[AuthorizationPolicy] = None | ||
| self.endpoint_policy: Optional[EndpointPolicy] = None |
There was a problem hiding this comment.
We won't need these policies for the cloud run CSM gRPC testcase, but only for the SPIFFE test.
| new_backends.append(new_backend) | ||
|
|
||
| # self.backends.update(new_backends) | ||
| backend_service = self.backend_service |
There was a problem hiding this comment.
Directly use self.backend_service
| raise ValueError("service_name cannot be empty or None") | ||
| if not image_name: | ||
| raise ValueError("image_name cannot be empty or None") | ||
| service_name = service_name[:49] |
There was a problem hiding this comment.
Why 49? Please consider add a comment for this magic number.
There was a problem hiding this comment.
It was something done by Sourabh. Let me confirm with him.
There was a problem hiding this comment.
Can you figure this out by printing what is that first 48 characters being extracted? If there is some delimiter at index 49 we can use that to split.
framework/test_app/runners/cloud_run/cloud_run_xds_server_runner.py
Outdated
Show resolved
Hide resolved
| self.compute.backend_service_patch_backends( | ||
| self.backend_service, | ||
| backends, | ||
| is_cloudrun=True, |
There was a problem hiding this comment.
Can we ensure that 'Cloud Run' is treated as two separate words consistently throughout?
bin/lib/common.py
Outdated
| @@ -59,6 +62,11 @@ def gcp_api_manager(): | |||
| return gcp.api.GcpApiManager() | |||
|
|
|||
|
|
|||
| @functools.cache | |||
| def c6n_api_manager(): | |||
There was a problem hiding this comment.
Can we ensure that we are not using c6n anywhere ?
| @@ -0,0 +1,141 @@ | |||
| # Copyright 2022 gRPC authors. | |||
| ) | ||
| return neg | ||
|
|
||
| def assertXdsConfigExists(self, test_client: XdsTestClient): |
There was a problem hiding this comment.
Since the only difference w.r.t. the method of the same name in XdsKubernetesBaseTestCase is the assertion for the endpoint_config, can we add a boolean parameter to indicate cloud run and have the different assertion based on that?
There was a problem hiding this comment.
What would be the benefit of doing that ? As far as I could see we overwrite the functions we can to change from the base class.
There was a problem hiding this comment.
To avoid repetition of logic. If the assertion for Xds config for GKE case and cloud run case was vastly different, it would make more sense to override it completely but here the only difference is that in the GKE case we assert for endpoint_config but not in the case of cloud run Xds config.
There may be an argument that the base class is polluted with subclass logic, and it is valid. For that I have this new suggestion: let the base class assertXdsConfig only validate LDS, RDS and CDS, and then call another method mayBeAssertEdsConfig(seen) . Base class method will do the EDS config assertions but the cloud run subclass override of mayBeAssertEdsConfig will just be a no-op (pass statement).
framework/xds_k8s_testcase.py
Outdated
| @@ -1395,3 +1400,177 @@ def debug_cert(cert): | |||
| return "missing" | |||
| sha1 = hashlib.sha1(cert) | |||
| return f"sha1={sha1.hexdigest()}, len={len(cert)}" | |||
|
|
|||
|
|
|||
| class CloudRunXdsKubernetesTestCase(RegularXdsKubernetesTestCase): | |||
There was a problem hiding this comment.
Can we extend from SecurityXdsKubernetesTestCase and create the client using the existing method StartSecureTestClient? If we do that we can also maintain a single client deployment yaml with the new changes for cloud run test added in the same yaml.
There was a problem hiding this comment.
changed to make it extend SecurityXdsKubernetesTestCase , but I dont understand the part about using the same YAML , we are still using the same yaml as client.deployement.yaml
There was a problem hiding this comment.
Yes, you can ignore the part of my comment where I wrongly thought there was an additional yaml created for cloud run.
| ) | ||
| # TODO(sergiitk): dataclass | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
This try catch only creates redundant log as the caller site already has it.
| @@ -41,6 +41,9 @@ spec: | |||
| - "--qps=${qps}" | |||
| - "--rpc=${rpc}" | |||
| - "--metadata=${metadata}" | |||
| % if secure_mode: | |||
| - "--secure_mode=${secure_mode}" | |||
There was a problem hiding this comment.
We don't need this change. --secure_mode=false/true is always passed here
Tests the case where the client is in GKE and the server is running in cloudrun. Verifies that 100 RPCs reach the server correctly and return with the correct response.