refactor: group xds security features for security policy

[shawnh2]

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

related: #2954, separated from #2926

[codecov]

Codecov Report

Attention: Patch coverage is 51.09489% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 64.41%. Comparing base (29946b0) to head (6c98fd2).
Report is 28 commits behind head on main.

❗ Current head 6c98fd2 differs from pull request most recent head e56efaf. Consider uploading reports for the commit e56efaf to get more accurate results

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	38 Missing and 1 partial ⚠️
internal/ir/xds.go	21.87%	22 Missing and 3 partials ⚠️
internal/xds/translator/oidc.go	70.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3019      +/-   ##
==========================================
- Coverage   66.51%   64.41%   -2.10%     
==========================================
  Files         161      121      -40     
  Lines       22673    21428    -1245     
==========================================
- Hits        15080    13802    -1278     
- Misses       6720     6759      +39     
+ Partials      873      867       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shawnh2]

/retest

[shawnh2]

/retest

[zhaohuabing]

Thanks for the refactoring. This looks good, just a few nits.

[shawnh2]

/retest

[zhaohuabing]

LGTM. Thanks!

[zirain]

/retest

[zhaohuabing]

@shawnh2 Apologies for the delay on this PR. Can you fix the conflicts so we can move on?

[shawnh2]

@shawnh2 Apologies for the delay on this PR. Can you fix the conflicts so we can move on?

Resolved, thanks for reviewing this.

[shawnh2]

/retest

[shawnh2]

TestE2E/GatewayInfraResourceTest/create_gateway seems a liitle unstable.

[shawnh2]

/retest

[zhaohuabing]

LGTM, thanks!

[arkodg]

will we ever hit this case ? where s != nil, but all sub fields are nil ?

[shawnh2]

we may very unlikely to hit the case that all fields are nil, but we may hit the case that only some (if any) of the fields are set, indicates s is not Empty, return false. This is exactly what we used for this method, as you can see in translateSecurityPolicyForGateway method.

I took the @zhaohuabing's suggestion, make this method called Empty, instead of Any (or other name), so it will be very comprehensible for anyone who read it.

[arkodg]

non blocking comment - I personally feel this is not needed, and s != nil is enough w/o the need for a Empty func

[shawnh2]

I will try this out, if it's working w/o Empty method, I will refactor this in the follow-up PR along with the refactor of BTP.

[arkodg]

this looks good @shawnh2 ! sorry for the delay in reviewing it
have one minor/curious comment reg Empty() , else LGTM !