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

Conversation

gemmahou
Copy link
Collaborator

@gemmahou gemmahou commented Oct 25, 2024

Change description

Fixes b/374166656

Tests you have done

Resource created successfully:

kubectl describe ComputeBackendService computebackendservice-yuhou1025
Spec:
  Load Balancing Scheme:  INTERNAL_SELF_MANAGED
  Location:               global
  Resource ID:            computebackendservice-yuhou1025
  Security Settings:
    Client TLS Policy Ref:
      Name:  clienttlspolicy-yuhou1025
    Subject Alt Names:
      spiffe://junk.svc.id.goog/ns/test/sa/frontend
  Timeout Sec:  10
Status:
  Conditions:
    Last Transition Time:  2024-10-25T19:18:09Z
    Message:               The resource is up to date
    Reason:                UpToDate
    Status:                True
    Type:                  Ready
  Creation Timestamp:      2024-10-25T12:03:28.186-07:00
  Fingerprint:             KkS0AHjQtyc=
  Generated Id:            2810711594725955000
  Observed Generation:     2
  Self Link:               https://www.googleapis.com/compute/v1/projects/cnrm-yuhou/global/backendServices/computebackendservice-yuhou1025
Events:
  Type     Reason              Age                  From                              Message
  ----     ------              ----                 ----                              -------
  Warning  DependencyNotReady  17m                  computebackendservice-controller  reference NetworkSecurityClientTLSPolicy default/clienttlspolicy-yuhou1025 is not ready
  Normal   Updating            2m40s (x4 over 17m)  computebackendservice-controller  Update in progress
  Normal   UpToDate            2m19s (x4 over 16m)  computebackendservice-controller  The resource is up to date

Dynamic test passed(after skipping testNoChangeAfterCreate):

--- PASS: TestCreateNoChangeUpdateDelete (0.17s)
    --- PASS: TestCreateNoChangeUpdateDelete/compute (0.00s)
        --- PASS: TestCreateNoChangeUpdateDelete/compute/basic-globalcomputebackendservicesecuritysettings (154.81s)
PASS
  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@gemmahou gemmahou force-pushed the valuetemplate branch 3 times, most recently from fd98442 to b3d51e8 Compare October 28, 2024 18:07
@gemmahou gemmahou changed the title fix: Incorrect value of clientTLSPolicy from ComputeBackendService WIP: fix: Incorrect value of clientTLSPolicy from ComputeBackendService Oct 28, 2024
@gemmahou gemmahou changed the title WIP: fix: Incorrect value of clientTLSPolicy from ComputeBackendService fix: Incorrect value of clientTLSPolicy from ComputeBackendService Oct 31, 2024
Copy link
Collaborator

@maqiuyujoyce maqiuyujoyce left a comment

Choose a reason for hiding this comment

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

Thank you for the fix, @gemmahou ! Mostly LGTM with a couple of comments.

pkg/krmtotf/templating.go Outdated Show resolved Hide resolved
pkg/krmtotf/templating.go Outdated Show resolved Hide resolved
pkg/krmtotf/templating.go Outdated Show resolved Hide resolved
// 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.

@@ -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.

Copy link
Collaborator

@maqiuyujoyce maqiuyujoyce left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Nov 1, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 68d9f31 into GoogleCloudPlatform:master Nov 1, 2024
18 checks passed
@gemmahou gemmahou deleted the valuetemplate branch November 4, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants