Skip to content

Fix IsDefaultNSXProject always returning false after SDK field rename#1389

Merged
timdengyun merged 1 commit intovmware-tanzu:mainfrom
zhengxiexie:topic/zhengxie/main/fix_3666180
Mar 16, 2026
Merged

Fix IsDefaultNSXProject always returning false after SDK field rename#1389
timdengyun merged 1 commit intovmware-tanzu:mainfrom
zhengxiexie:topic/zhengxie/main/fix_3666180

Conversation

@zhengxiexie
Copy link
Copy Markdown
Contributor

@zhengxiexie zhengxiexie commented Mar 11, 2026

✨ What's Changed

IsDefaultNSXProject

  • Replace SDK struct deserialization with direct REST API call via Cluster.HttpGet
  • Read the "default" field from NSX Policy API JSON response (/policy/api/v1/orgs/{orgID}/projects/{projectID})
  • Bypass the unexported model.Project._Default field that the VAPI type converter cannot populate
  • Remove old getProjectDefault() helper and reflect/unsafe imports

Test Updates

  • 4 test cases using gomonkey to mock Cluster.HttpGet:
    • Project is default ("default": true) → returns true
    • Project is not default ("default": false) → returns false
    • Response missing default field → returns false
    • HTTP error → returns error

🎯 Motivation

SDK commit 7bd53190 renamed model.Project.Default (exported) to model.Project._Default (unexported). The VAPI type converter uses reflect to populate struct fields, but Go reflect cannot write unexported fields (CanSet() returns false) — so _Default is never deserialized and always nil.

Issue: IsDefaultNSXProject() always returns false for all projects including the NSX default project. This causes NetworkPolicy with namespaceSelector under the default project to build incorrect share group paths (/orgs/default/projects/default/infra/... instead of /infra/...), resulting in NSX error 524297: "Invalid infra operation on default project."

Solution: Use Cluster.HttpGet to query the NSX Policy API directly and read the "default" boolean from the raw JSON response, bypassing the broken SDK struct deserialization entirely.

✅ Testing

Unit Tests

  • All 4 test cases pass
  • TestIsDefaultNSXProject covers: default project, non-default project, missing field, HTTP error

Integration Test

✅ Test Case 1: Non-Default Project (project-quality)

Purpose: Validate fix using the exact NetworkPolicy spec from the bug report under a non-default project. Shares should go to project-level path.

Configuration (same spec as bug report, namespace adapted for testbed):

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: np-1
  namespace: default
spec:
  ingress:
  - from:
    - ipBlock:
        cidr: 40.100.100.200/16
    - namespaceSelector:
        matchLabels:
          app: tcp
    ports:
    - port: 6000
      protocol: TCP
  podSelector:
    matchLabels:
      nptcp1: tcp1
  policyTypes:
  - Ingress

Before fix (from bug report, rule_hash 1ba78f27):

ERROR firewall.go:1038 Failed to create or update NSX SecurityPolicy in VPC
  error="nsx error code: 524297, message: Invalid infra operation on default project."
  SourceGroups:["/orgs/default/projects/default/infra/domains/default/groups/np-1-allow-1ba78f27-src_e2tx2"]

After fix (same spec, same rule_hash 1ba78f27): ✅ SUCCESS

INFO firewall.go:638 Successfully created or updated NSX SecurityPolicy resources in VPC
  DisplayName: "np-1", Id: "np-1-allow_87vzt"
  SourceGroups: ["/orgs/default/projects/project-quality/infra/domains/default/groups/np-1-allow-1ba78f27-src_87vzt"]
  nsx_share_created_for: "project"

✅ Test Case 2: Default Project (project ID "default")

Purpose: Validate that shares go to /infra/ level when VPC is under the NSX default project. This is the exact scenario from the bug report.

Setup: Created VPC under the default project (/orgs/default/projects/default/vpcs/test-default-project_sfh7c) with a new VPCNetworkConfiguration pointing to nsxProject: /orgs/default/projects/default.

Configuration:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: np-1
  namespace: test-default-project
  labels:
    nptcp1: tcp1
spec:
  podSelector: {}
  policyTypes:
  - Ingress
  ingress:
  - from:
    - ipBlock:
        cidr: 10.0.0.0/8
    - namespaceSelector:
        matchLabels:
          app: peer-ns
    ports:
    - protocol: TCP
      port: 6000

Result: ✅ SUCCESS — Both ALLOW and ISOLATION SecurityPolicies created. Shares correctly placed at /infra/ level:

Share Group: /infra/domains/default/groups/np-1-allow-45b4acf7-src_hw6m9
  nsx_share_created_for: "infra"

Share: /infra/shares/default_group_np-1-allow-45b4acf7-src_hw6m9_share
  sharedWith: ["/orgs/default/projects/default"]
  nsx_share_created_for: "infra"

Explanation: IsDefaultNSXProject("default", "default") correctly returns true, routing shares to the global /infra/ hierarchy instead of the invalid project-level path.


NSX Project Verification

Confirmed via NSX API that the default project has "default": true and "_system_owned": true:

Project "default":         { "id": "default",         "default": true,  "_system_owned": true  }
Project "project-quality": { "id": "project-quality", "default": false, "_system_owned": false }
SDK Root Cause Verification

Added diagnostic logging to DataValueToNativeConverter.setStructType() and confirmed:

CanSet=false, IsValid=true, Kind=ptr   → _Default field skipped by reflect
_Default value is nil (SDK did not populate this field)

This proves the VAPI type converter silently skips the unexported _Default field.

🔄 Backward Compatibility

This change is fully backward compatible:

  • For non-default projects: IsDefaultNSXProject() still returns false (same behavior as before, verified with project-quality)
  • For the default project: returns true (corrected from broken false, verified with shares at /infra/ level)
  • No API signature changes; the function still takes (orgID, projectID) and returns (bool, error)
  • Uses Cluster.HttpGet which leverages existing NSX client connection pooling and authentication

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.75%. Comparing base (4ba0355) to head (fe8bb50).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/nsx/services/vpc/vpc.go 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1389   +/-   ##
=======================================
  Coverage   76.74%   76.75%           
=======================================
  Files         151      151           
  Lines       21307    21308    +1     
=======================================
+ Hits        16353    16354    +1     
  Misses       3784     3784           
  Partials     1170     1170           
Flag Coverage Δ
unit-tests 76.75% <84.61%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
pkg/nsx/services/vpc/vpc.go 72.99% <84.61%> (-0.04%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zhengxiexie
Copy link
Copy Markdown
Contributor Author

/e2e

@zhengxiexie zhengxiexie force-pushed the topic/zhengxie/main/fix_3666180 branch from 0bd4558 to 328ade0 Compare March 11, 2026 09:19
@zhengxiexie
Copy link
Copy Markdown
Contributor Author

/e2e

@zhengxiexie zhengxiexie force-pushed the topic/zhengxie/main/fix_3666180 branch from 328ade0 to c7f4b82 Compare March 11, 2026 10:12
timdengyun
timdengyun previously approved these changes Mar 13, 2026
Copy link
Copy Markdown
Contributor

@timdengyun timdengyun left a comment

Choose a reason for hiding this comment

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

just a nit

Use Cluster.HttpGet to read the "default" field directly from the NSX
REST API JSON response, bypassing the SDK struct deserialization which
cannot populate the unexported model.Project._Default field.

Co-Authored-By: Oz <oz-agent@warp.dev>
Change-Id: I79cfd117c95abe17ef7032525e69cd1695e8822e
@timdengyun
Copy link
Copy Markdown
Contributor

/e2e

@timdengyun timdengyun merged commit c3a71ec into vmware-tanzu:main Mar 16, 2026
2 checks passed
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.

4 participants