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

(Part 5 Do not merge)Fix diffs in compute forwarding rule: collect realGCP logs to verify #2727

Closed
wants to merge 6 commits into from

Conversation

gemmahou
Copy link
Collaborator

@gemmahou gemmahou commented Sep 18, 2024

Change description

Based on Fixes PR part 1-4

This is the final PR for diff fixes, and is used to verify changes in part 1-4. We do not merge this PR.

Note: we only focus on logs related to ComputeForwardingRule in this PR. For diffs in other dependencies, we should fix them later during their migration to direct controller.

realGCP with direct controller: No diffs in ComputeForwardingRule logs

realGCP with TF controller: No diffs in ComputeForwardingRule logs, except request URLs&headers, and compute endpoint versions(TF uses beta, but direct controller uses v1 instead)

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from gemmahou. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@@ -568,6 +477,7 @@ x-goog-request-params: project=${projectId}&region=us-central1
},
"loadBalancingScheme": "EXTERNAL",
"name": "computeregionalforwardingrule-${uniqueId}",
"region": "projects/${projectId}/global/regions/us-central1",
Copy link
Collaborator Author

@gemmahou gemmahou Sep 18, 2024

Choose a reason for hiding this comment

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

This field appears in TF-controller log, but not in direct controller log.

We have noticed this diff before, and I think this is a "bug" in TF-controller. region is output only, it does nothing here in the request body. Also the format global/regions looks strange, I don't find the source code of this value.


---

POST https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/${forwardingRuleID}/setLabels
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's strange that there's no "setLabels" request with TF-controller(update label-one: value-one to label-one: value-two). I tried to update labels with the TF-controller and checked gcloud output, the label value did get updated. But there's no update labels request in the log.

While, I think the direct-controller log is correct, we expect this "setLabels" request to update the label value.

@gemmahou gemmahou changed the title (Part 5)Fix diffs in compute forwarding rule: collect realGCP logs to verify (Part 5 Do not merge)Fix diffs in compute forwarding rule: collect realGCP logs to verify Sep 18, 2024
@gemmahou gemmahou closed this Oct 2, 2024
@gemmahou gemmahou deleted the compare-real branch October 2, 2024 18:31
@yuwenma yuwenma added this to the 1.124 milestone Oct 25, 2024
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.

2 participants