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

feat(kuma-cp): allow sectionName and labels in targetRef #11819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Neyaz
Copy link

@Neyaz Neyaz commented Oct 21, 2024

Motivation

According to #11572 there's no real reason to have such validation.
However, I didn't get if this part from the issue If one of the matching meshService doesn't have such a sectionName it should simply not match just like it's already the case for labels. already works in this way or it needs to be implemented.

Implementation information

The change is pretty simple.

Supporting documentation

None

@Neyaz Neyaz requested a review from a team as a code owner October 21, 2024 18:38
@Neyaz Neyaz requested review from michaelbeaumont and Automaat and removed request for a team October 21, 2024 18:38
Signed-off-by: Nikolai M <ne9zzz@gmail.com>
@Neyaz Neyaz force-pushed the relax-validation-section-name branch from 70c3970 to 4cd048d Compare October 21, 2024 18:40
@michaelbeaumont michaelbeaumont changed the title fix(k8s): relax validation on sectionName with ServiceMesh feat(kuma-cp): allow sectionName and labels in targetRef Oct 21, 2024
@jakubdyszkiewicz
Copy link
Contributor

Hey, thanks for the contribution.

However, I didn't get if this part from the issue If one of the matching meshService doesn't have such a sectionName it should simply not match just like it's already the case for labels. already works in this way or it needs to be implemented.

I believe it already works that way. @lobkovilya to confirm

Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

Unfortunately, even though it looks like it's supported, method resolveTargetRef doesn't take SectionName into account.

I added the following input file to pkg/plugins/policies/core/rules/testdata/resourcerules:

# MeshTimeout with targetRef to the real MeshService resource
type: MeshTimeout
name: matched-for-rules-mt-1
mesh: mesh-1
spec:
  targetRef:
    kind: Mesh
  to:
    - targetRef:
        kind: MeshService
        labels:
          env: dev
        sectionName: httpport
      default:
        idleTimeout: 10s
        connectionTimeout: 11s
        http:
          requestTimeout: 12s
---
type: MeshService
name: backend-1
mesh: mesh-1
labels:
  env: dev
spec:
  selector:
    dataplaneTags:
      app: backend
  ports:
    - port: 80
      name: httpport
      targetPort: 80
      appProtocol: http
---
type: MeshService
name: backend-2
mesh: mesh-1
labels:
  env: dev
spec:
  selector:
    dataplaneTags:
      app: backend
  ports:
    - port: 80
      targetPort: 80
      appProtocol: http
---
type: MeshService
name: backend-3
mesh: mesh-1
labels:
  env: prod
spec:
  selector:
    dataplaneTags:
      app: backend
  ports:
    - port: 80
      targetPort: 80
      appProtocol: http

and the golden file is not correct.

@Neyaz
Copy link
Author

Neyaz commented Oct 22, 2024

@jakubdyszkiewicz @lobkovilya Thank you for a review!

Unfortunately, even though it looks like it's supported, method resolveTargetRef doesn't take SectionName into account.

@lobkovilya I believe it should be handled by sectionName method. As I understand it, it should return refSectionName for every resolvedPolicyItem if sectionName is set.

@lobkovilya
Copy link
Contributor

TBH I think the problem is in resolveTargetRef, it never calls sectionName() method and operates only with implicitSectionName (should also take into account the explicit one)

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