Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Incorrect value of clientTLSPolicy from ComputeBackendService #3007

Merged
merged 3 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,8 @@ spec:
- external
properties:
external:
description: 'Allowed value: The `name` field of a `NetworkSecurityClientTLSPolicy`
description: 'Allowed value: string of the format `//networksecurity.googleapis.com/projects/{{project}}/locations/{{location}}/clientTlsPolicies/{{value}}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to make this sort of description more of a standard. Seems like a huge improvement. Worth noting that for this resource it seems like location always has to be "global".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientTlsPolicy does support other locations(API doc).

If a user incorrectly configures a regional policy, the request should be sent to the API as is and let the API to handle the error. We may not want to convert the regional policy to a global policy on our side(?). In rare cases, there might be a global resource with the same name, and this could lead to confusion.

where {{value}} is the `name` field of a `NetworkSecurityClientTLSPolicy`
resource.'
type: string
name:
Expand Down
1 change: 1 addition & 0 deletions config/servicemappings/compute.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ spec:
kind: NetworkSecurityClientTLSPolicy
version: v1beta1
group: networksecurity.cnrm.cloud.google.com
valueTemplate: "//networksecurity.googleapis.com/projects/{{project}}/locations/{{location}}/clientTlsPolicies/{{value}}"
dclBasedResource: true
- tfField: iap.oauth2_client_id
description: OAuth2 Client ID for IAP.
Expand Down
11 changes: 11 additions & 0 deletions pkg/krmtotf/templating.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,17 @@ func expandFieldTemplate(template string, r *Resource, c client.Client, smLoader
if val, exists, _ := unstructured.NestedString(r.GetStatusOrObservedState(), strings.Split(path, ".")...); exists {
return val
}

// Special handling to resolve project from DCL-based resource
// Only applied to a referenced NetworkSecurityClientTLSPolicy resource
// and tested by fixtures/globalcomputebackendservicesecuritysettings test for now
if path == "project" && r.Kind == "NetworkSecurityClientTLSPolicy" {
dclPath := "projectRef.external"
if val, exists, _ := unstructured.NestedString(r.Spec, strings.Split(dclPath, ".")...); exists {
return val
}
}

if isRequired {
resolutionError = fmt.Errorf("unable to resolve missing value: %v", field)
}
Expand Down
9 changes: 9 additions & 0 deletions pkg/test/resourcefixture/contexts/compute_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,15 @@ func init() {
SkipUpdate: true,
}

resourceContextMap["globalcomputebackendservicesecuritysettings"] = ResourceContext{
ResourceKind: "ComputeBackendService",
// Underlying API changes dependency resource's project id to number after successful creation.
// For now TF servicemapping does not have a way to resolve dependency DCL resources' project number.
// Skip checking no change after creation(testNoChangeAfterCreate) to bypass this temporarily.
// See https://buganizer.corp.google.com/issues/374166656#comment11 for details.
SkipNoChange: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this field unreadable so that it'll always use the value specified in the CR as the live state for comparison?

Not a blocker for this PR though.

}

resourceContextMap["computeexternalvpngateway"] = ResourceContext{
ResourceKind: "ComputeExternalVPNGateway",
SkipUpdate: true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright 2024 Google LLC
#
# Licensed 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.

apiVersion: compute.cnrm.cloud.google.com/v1beta1
kind: ComputeBackendService
metadata:
name: computebackendservice-${uniqueId}
spec:
loadBalancingScheme: INTERNAL_SELF_MANAGED
# The security settings that apply to this backend service. This field is applicable to
# global backend service with the load_balancing_scheme set to INTERNAL_SELF_MANAGED
securitySettings:
clientTLSPolicyRef:
name: networksecurityclienttlspolicy-${uniqueId}
subjectAltNames:
- spiffe://junk.svc.id.goog/ns/test/sa/frontend
timeoutSec: 10
location: global
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2024 Google LLC
#
# Licensed 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.

apiVersion: networksecurity.cnrm.cloud.google.com/v1beta1
kind: NetworkSecurityClientTLSPolicy
metadata:
name: networksecurityclienttlspolicy-${uniqueId}
spec:
clientCertificate:
certificateProviderInstance:
pluginInstance: google_cloud_private_spiffe
location: global
serverValidationCa:
- certificateProviderInstance:
pluginInstance: google_cloud_private_spiffe
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright 2024 Google LLC
#
# Licensed 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.

apiVersion: compute.cnrm.cloud.google.com/v1beta1
kind: ComputeBackendService
metadata:
name: computebackendservice-${uniqueId}
spec:
loadBalancingScheme: INTERNAL_SELF_MANAGED
# The security settings that apply to this backend service. This field is applicable to
# global backend service with the load_balancing_scheme set to INTERNAL_SELF_MANAGED
securitySettings:
clientTLSPolicyRef:
name: networksecurityclienttlspolicy-${uniqueId}
subjectAltNames:
- spiffe://junk.svc.id.goog/ns/test/sa/frontend
timeoutSec: 5
location: global
Original file line number Diff line number Diff line change
Expand Up @@ -2150,7 +2150,7 @@ service resource.{% endverbatim %}</p>
</td>
<td>
<p><code class="apitype">string</code></p>
<p>{% verbatim %}Allowed value: The `name` field of a `NetworkSecurityClientTLSPolicy` resource.{% endverbatim %}</p>
<p>{% verbatim %}Allowed value: string of the format `//networksecurity.googleapis.com/projects/{{project}}/locations/{{location}}/clientTlsPolicies/{{value}}`, where {{value}} is the `name` field of a `NetworkSecurityClientTLSPolicy` resource.{% endverbatim %}</p>
</td>
</tr>
<tr>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4305,11 +4305,7 @@ func expandComputeBackendServiceSecuritySettings(v interface{}, d tpgresource.Te
}

func expandComputeBackendServiceSecuritySettingsClientTlsPolicy(v interface{}, d tpgresource.TerraformResourceData, config *transport_tpg.Config) (interface{}, error) {
f, err := tpgresource.ParseGlobalFieldValue("regions", v.(string), "project", d, config, true)
if err != nil {
return nil, fmt.Errorf("Invalid value for client_tls_policy: %s", err)
}
return f.RelativeLink(), nil
return v, nil
}

func expandComputeBackendServiceSecuritySettingsSubjectAltNames(v interface{}, d tpgresource.TerraformResourceData, config *transport_tpg.Config) (interface{}, error) {
Expand Down
Loading