Skip to content

Conversation

gemmahou
Copy link
Collaborator

@gemmahou gemmahou commented Sep 18, 2025

Change description

Move refs from apis/refs/v1beta1 to apis/compute/v1beta1 and use NormalizedExternal to resolve its value

Special notes for your reviewer:

Does this PR add something which needs to be 'release noted'?


  • Reviewer reviewed release note.

Additional documentation e.g., references, usage docs, etc.:


Intended Milestone

Please indicate the intended milestone.

  • Reviewer tagged PR with the actual milestone.

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 refactor-computenetwork-ref branch 2 times, most recently from 761214d to 8b05f90 Compare September 18, 2025 22:22
@gemmahou
Copy link
Collaborator Author

There's no untested ComputeNetwork references in direct resources(source). There's no golden logs changed post-refactoring confirm that direct resources won't be affected by this PR.

@gemmahou gemmahou force-pushed the refactor-computenetwork-ref branch from 8b05f90 to 8e75a0c Compare September 18, 2025 22:31
@gemmahou gemmahou changed the title WIP: chore: refactor ComputeNetworkRef chore: refactor ComputeNetworkRef Sep 18, 2025
@gemmahou gemmahou force-pushed the refactor-computenetwork-ref branch from 8e75a0c to eebc340 Compare September 18, 2025 23:33
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.

/assign @cheftako

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

@gemmahou gemmahou force-pushed the refactor-computenetwork-ref branch from 72a6b08 to 01d7464 Compare October 7, 2025 03:22
@gemmahou gemmahou force-pushed the refactor-computenetwork-ref branch from 01d7464 to 68bd1d5 Compare October 8, 2025 23:16
@gemmahou
Copy link
Collaborator Author

gemmahou commented Oct 9, 2025

Generated code review:

**Summary of Changes**

This pull request is a significant refactoring of how ComputeNetworkRef, a reference to a ComputeNetwork resource, is handled within the Config Connector codebase. The core change
involves moving the ComputeNetworkRef type definition from the generic apis/refs/v1beta1 package to the more specific apis/compute/v1beta1 package.

The purpose of this refactoring is to improve code organization and maintainability by placing the reference type alongside the resource it refers to. As part of this change, the
logic for normalizing the external reference format has been improved and centralized in the new network_reference.go file. This new implementation, NormalizedExternal, handles
resolving the reference from either an external field or a name and namespace pair, and it also includes logic to handle different URL formats for backward compatibility.

The PR touches a large number of files because it updates all the resources that use ComputeNetworkRef to point to the new location. This includes updating the CRD definitions, the Go
type definitions for various resources, and the direct controller logic that resolves these references.

**Implementation Analysis**

The implementation appears to be a sound and well-executed refactoring. The new ComputeNetworkRef implementation in apis/compute/v1beta1/network_reference.go is more robust than the
previous one. It correctly handles resolving the reference from either the external field or the name/namespace fields, and it also includes logic to handle different URL formats from
GCP, which improves backward compatibility.

The introduction of FixStaleExternalFormat in apis/common/utils.go is a good addition to handle various URL prefixes that might be present in the external field. The new
NetworkIdentity struct provides a clean way to parse and represent the components of a ComputeNetwork's external identifier.

The changes are spread across many files, but they are all related to updating the import path of ComputeNetworkRef. The logic changes are concentrated in the new
apis/compute/v1beta1/network_reference.go and apis/compute/v1beta1/network_identity.go files, and in the direct controllers that now use the new NormalizedExternal method. The changes
in the direct controllers are minimal and consistent, simply replacing the old normalization logic with a call to the new method.

**Testing Gaps**

The author states in a comment that there are no untested ComputeNetwork references in direct resources and that the golden logs have not changed after the refactoring. This suggests
that the existing test suite provides good coverage for this change. However, given the scale of the refactoring, it would be beneficial to have explicit unit tests for the new
NormalizedExternal function to cover various scenarios, such as:

 * Resolving from external with a full URL.
 * Resolving from external with a partial URL.
 * Resolving from name and namespace.
 * Handling cases where the referenced ComputeNetwork does not exist.
 * Handling various stale external formats in FixStaleExternalFormat.

While the existing E2E tests seem to cover the functionality, adding these unit tests would provide more targeted and faster feedback for any future changes to this logic.

**Backward Compatibility**

This PR is a breaking change for any external users who might be importing the refs.ComputeNetworkRef type directly. However, since this is an internal refactoring and all internal
call sites have been updated, it is an acceptable breaking change within the pkg/ directory.

For users of the CRDs, this change is backward compatible. The CRD definitions have been updated to reflect the new structure, but the user-facing API remains the same. The logic in
NormalizedExternal is designed to be backward compatible with the old URL formats, so existing resources should continue to work without any issues.

**Performance & Scalability**

The changes are in the critical path of the direct controllers' reconciliation logic. However, the new implementation for resolving references is not expected to have any negative
performance impact. The logic is efficient and involves simple string manipulations and a single Kubernetes API call to fetch the referenced ComputeNetwork resource when needed. There
are no new scalability bottlenecks introduced.

**Security Considerations**

This refactoring does not introduce any new security vulnerabilities. The changes are focused on code organization and improving the reference resolution logic. There are no changes
to how credentials or sensitive information are handled.

**Maintainability & Code Organization**

This PR significantly improves the maintainability and code organization of the project. Moving the ComputeNetworkRef to the apis/compute/v1beta1 package makes the codebase more
modular and easier to understand. Centralizing the reference resolution logic in the NormalizedExternal function reduces code duplication and makes it easier to maintain and update in
the future. The introduction of NetworkIdentity and FixStaleExternalFormat also contributes to cleaner and more robust code.

@gemmahou
Copy link
Collaborator Author

gemmahou commented Oct 9, 2025

To address Gemini code review suggestion, added unit test for the NormalizedExternal function in ff83de2

This ensured that both formats(full and partial url) are accepted and normalized to the standard external format.

@gemmahou gemmahou force-pushed the refactor-computenetwork-ref branch from e9fd7e7 to 94b8128 Compare October 9, 2025 04:44
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