feat: support listener name in SecurityPolicy target

[kinolaev]

What type of PR is this?

feat: support listener name in SecurityPolicy target

What this PR does / why we need it:
It allows to target only a specific listener in a Security Policy. One usage example is attaching mtls authorization as described in #5909

Release Notes: No

[kinolaev]

Test is fixed, make go.test.cel runs without any error now

[arkodg]

• similar to L567, can we add a var for h.Name[strings.LastIndex(h.Name, "/")+1:]
• can you add some tests in internal/gatewayapi/testdata
• the current implementation is incomplete, it only ensures a listener policy targets/applies the config to the right IR, the remaining piece is to skip any top level Gateway policy that is targeting this listener level policy, similar to whats done for Gateway and Route

[kinolaev]

Thanks for pointing these issues out! I've addressed all of them in 3f7c7bb6454040b667b4fe4d370500bed1c865e3

[codecov]

Codecov Report

Attention: Patch coverage is 91.97080% with 11 lines in your changes missing coverage. Please review.

Project coverage is 70.68%. Comparing base (9925189) to head (11818cb).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/gatewayapi/securitypolicy.go	91.97%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5916      +/-   ##
==========================================
+ Coverage   70.64%   70.68%   +0.03%     
==========================================
  Files         220      220              
  Lines       36868    36937      +69     
==========================================
+ Hits        26046    26108      +62     
- Misses       9291     9295       +4     
- Partials     1531     1534       +3     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zhaohuabing]

Hi @kinolaev Could you clarify in the description how the overriding/merging strategy works between Gateway/Listener/Route?

[zhaohuabing]

Supporting Listener still makes sense, but do we need this for #5909? Cert validation will be done in ClientTrafficPolicy at #5868, and fine-grained control using SAN as a type of Principal can be done at Gateway/Route level.

[kinolaev]

Sorry for long reply(

Could you clarify in the description how the overriding/merging strategy works

A Listener attached security policy applies only to Routes that doesn't have a security policy attached to them directly. Could you please be more specific about where exactly you want me to add this explanation?

do we need this for #5909

It depends on your decision) In the issue I've described two possibilities of mTLS authorization - using ClientTrafficPolicy (during handshake) or using SecurityPolicy (after handshake). ClientTrafficPolicy is a good place to add SAN allowlist into envoy's CertificateValidationContext and we don't need this PR for that. But if you only want to authorize SANs using envoy's RBAC then SecurityPolicy might be a better place and consequently we need this PR as a prerequisite.

So, yeah, I made steps in both directions because I didn't want to guess what you would decide) But as you already mentioned, this PR could be useful even without relation to the mTLS authorization issue.

[arkodg]

requesting @zirain to review, who implemented something similar for BTP but per route rule

[zirain]

Is this wording correct? Do we only need to tell the end users on which routes the current policy takes effect?
cc @arkodg

[arkodg]

this is fine, this is consistent with overrides today

gateway/internal/gatewayapi/testdata/backendtrafficpolicy-status-conditions.out.yaml

Line 129 in 28424b0

message: 'This policy is being overridden by other backendTrafficPolicies

[zhaohuabing]

A Listener attached security policy applies only to Routes that doesn't have a security policy attached to them directly. Could you please be more specific about where exactly you want me to add this explanation?

Yeah, this makes sense as we need to apply SAN based authz for the entire listener.

[arkodg]

Suggested change

	// +kubebuilder:validation:XValidation:rule="has(self.targetRef) ? self.targetRef.kind == 'Gateway' || !has(self.targetRef.sectionName) : true",message="this policy supports the sectionName field only for gateways"
	// +kubebuilder:validation:XValidation:rule="has(self.targetRef) ? self.targetRef.kind == 'Gateway' || !has(self.targetRef.sectionName) : true",message="this policy supports the sectionName field only for kind Gateway"

[kinolaev]

Updated in f1a1fd5262b6609b7617e39edee602514d3c4595

[arkodg]

Suggested change

	// +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.kind == 'Gateway' || !has(ref.sectionName)) : true",message="this policy supports the sectionName field only for gateways"
	// +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.kind == 'Gateway' || !has(ref.sectionName)) : true",message="this policy supports the sectionName field only for kind Gateway"

[kinolaev]

Updated in f1a1fd5262b6609b7617e39edee602514d3c4595

[arkodg]

can we have one with Gateway so we also tst posiitive case

[kinolaev]

Added in f1a1fd5262b6609b7617e39edee602514d3c4595

[arkodg]

can you add a comment for this ? can't understand this code

[kinolaev]

"A HTTPListener name has the format namespace/gatewayName/listenerName" and "If specified the sectionName must match the listenerName part of the HTTPListener name" added in f1a1fd5262b6609b7617e39edee602514d3c4595

[arkodg]

LGTM thanks !

[arkodg]

the test is failing due to a difference in string

--- FAIL: TestSecurityPolicyTarget (0.15s)
    --- FAIL: TestSecurityPolicyTarget/sectionName_disabled_until_supported_for_kind_xRoute_-_targetRef (0.00s)
        securitypolicy_test.go:1358: Unexpected response while creating SecurityPolicy; got err=
            SecurityPolicy.gateway.envoyproxy.io "sp-1749864680953051291" is invalid: spec: Invalid value: "object": this policy supports the sectionName field only for kind Gateway
            ;missing strings within error=["spec: Invalid value: \"object\": this policy supports the sectionName field only kind Gateway"]

you should be able to repro this locally using make kube-test

[kinolaev]

Oh, sorry, forgot "for" in "only kind Gateway"( fixed