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.

Sign up for GitHub

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 3)Fix diffs in compute forwarding rule: remove networkTier field from global forwarding rule #2724

Closed
wants to merge 1 commit into from
Closed

(Part 3)Fix diffs in compute forwarding rule: remove networkTier field from global forwarding rule #2724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions pkg/controller/direct/compute/forwardingrule_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, createOp *directbase
var err error
op := &gcp.Operation{}
if a.id.location == "global" {
// todo(yuhou): TF does not support networkTier field for global forwarding rule
// It will always use GCP's default value, which is "PREMIUM." Any value set by the user will be ignored and not sent to GCP.
// To align with the TF controller, I remove this field.
// Ideally, direct controller should support this field and validate that the value.
forwardingRule.NetworkTier = nil
Copy link
Collaborator

@yuwenma yuwenma Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't quite get this (but I like this comment). Could you describe what the Compute GCP service does for this field? I guess I'm confused because from your description it looks like TF sets the default "PREMIUM", in which case this code shall use "PREMIUM" as well to keep the behavior, but it sets 'nil'? Or, is the "PREMIUM" set by the GCP service if nil is passed in? If it is the latter, does the GCP return any errors/warnings?

Regarding the previous TF behavior, I guess it could relate to the spec "auto-correct" in state-into-spec:merge mode.

Copy link
Collaborator Author

@gemmahou gemmahou Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The latter one.
TF for global forwarding rule does not have "networkTier" field(see their documentation), that means whatever value provided by user, will be ignored in the request. If the field is unspecified, GCP will default it to "PREMIUM", no errors/warnings. I hope below example can explain that clearly:

Configuration yaml:

metadata:
 name: globalforwardingrule
spec:
 networkTier: "anything"
 (other fields)

request body with TF controller:

POST ...globalforwardingrule...
{
(other fields)
}

200 ok
{
operationType: insert,
progress: 0,
status: RUNNING,
}

response body with TF controller:

GET ...gloablforwardingrule...
200 ok
{
networkTier: "PREMIUM"
(other fields)
}

Quote the field description: For regional ForwardingRule, the valid values are 'PREMIUM' and 'STANDARD'. For GlobalForwardingRule, the valid value is 'PREMIUM'. If this field is not specified, it is assumed to be 'PREMIUM'. I think that explains why TF does not support this field for global forwarding rule and uses GCP default value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation. I think

  1. we want to include the networkTier field in status.observedState.
  2. I think we don't need this line forwardingRule.NetworkTier = nil, with the previous TF-based state-into-spec: merge default, this guarantees backward compatibility for existing forwarding rules.

req := &computepb.InsertGlobalForwardingRuleRequest{
ForwardingRuleResource: forwardingRule,
Project: a.id.project,
Expand Down
1 change: 0 additions & 1 deletion ...ata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrulefull/_http.log
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,6 @@ x-goog-request-params: project=${projectId}
],
"name": "computeglobalforwardingrule-${uniqueId}",
"network": "projects/${projectId}/global/networks/${networkID}",
"networkTier": "PREMIUM",
"portRange": "80",
"target": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/targetHttpProxies/computetargethttpproxy-${uniqueId}"
}
Expand Down
Loading