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

Closed

Conversation

gemmahou
Copy link
Collaborator

Change description

Based on #2723. To review the change, check the last commit in this PR.

TF does not support networkTier field for global forwarding rule, It will always use GCP's default value "PREMIUM". Any value set by the user will be ignored and not sent to GCP.

Not sure if that's an ideal behavior, but let's try to match TF controller at this point.

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

// 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

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

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.

@gemmahou
Copy link
Collaborator Author

gemmahou commented Sep 19, 2024

As discussed, we do not want to drop "networkTier" field in direct controller. The diff is acceptable.

@gemmahou gemmahou closed this Sep 19, 2024
@gemmahou gemmahou deleted the fix-networktier branch September 19, 2024 21:17
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