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

chore: Add backend service reference and refactor reference code #3316

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gemmahou
Copy link
Collaborator

@gemmahou gemmahou commented Dec 5, 2024

Change description

b/379722657

Use ComputeBackendService as an example, explore how we normalize and resolve referenced resources.

My idea in this PR is to add a ReferenceContext struct, developer need to provide info when they try to resolve a referenced resource. Depends on different ReferenceContext, we can handle new referenced resources and ensure backward compatibility at the same time. I plan to add some questions regarding resource reference in the questionnaire.

For example, ComputeTargetTCPProxy has reference to ComputeBackendService(BackendServiceRef):
First, developer need to figure out whether ComputeBackendService is a pure direct resource(No); Then they need to know the required value(format) of the field, for ComputeTargetTCPProxy, the value(format) of BackendServiceRef is selfLink(https://www.googleapis.com/compute/v1/projects/${projectId}/global/backendServices/backendService). So their ReferenceContext looks like ReferenceContext{IsDirectOnly: false, TargetField: "status.selfLink"}.

When they resolve the referenced BackendService in the direct controller, they need to provide the ReferenceContext, based on their own API requirements. And resolve/convert the value to API required value.

In the future, we can add more functions inside ReferenceContext.

I refactored BackendService ref in this PR, and there was no log change.

Tests you have done

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

@gemmahou gemmahou force-pushed the backendserviceref branch 3 times, most recently from 30c60b8 to 4903616 Compare December 5, 2024 22:42
@gemmahou gemmahou changed the title WIP: chore: Add backend service reference and refactor reference code chore: Add backend service reference and refactor reference code Dec 5, 2024
@gemmahou gemmahou marked this pull request as ready for review December 5, 2024 22:42
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 justinsb. 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

external = strings.TrimPrefix(external, "/")
tokens := strings.Split(external, "/")
if len(tokens) == 5 && tokens[0] == "projects" && tokens[3] == "backendServices" {
return &BackendServiceIdentity{
Copy link
Collaborator Author

@gemmahou gemmahou Dec 12, 2024

Choose a reason for hiding this comment

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

One thought: maybe we could rename ResourceIdentity to ResourceExternal? I feel "Identity" is just the same thing as "ExternalRef", so that we don't introduce many new terms to people.

@gemmahou gemmahou force-pushed the backendserviceref branch 2 times, most recently from 23913f8 to 1d5cc2b Compare December 12, 2024 21:17
Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

This PR contains several things where we do not have full test coverage yet. Suggest spliting it into smaller PRs and focus on one change a time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants