CIDRGroup reference metric will not count nonexistent CIDRGroups

[akstron]

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Are you a user of Cilium? Please add yourself to the Users doc
• Thanks for contributing!

Fixes: #25022

Added a check on CIDRGroupRef to make sure that the reference is valid.

CIDRGroup reference metric will not count nonexistent CIDRGroups

Signed-off-by: Alok Kumar Singh alokaks601@gmail.com

[christarazi]

/test

[pippolo84]

Thanks for your PR. 🙏

Could you please explain in more detail in which case you ended up with rule.CIDR and rule.CIDRGroupRef both empty?

Given the kubebuilder validation that should not be possible:

cilium/pkg/policy/api/cidr.go

Lines 39 to 49 in fb48741

	// CIDR is a CIDR prefix / IP Block.
	//
	// +kubebuilder:validation:OneOf
	Cidr CIDR `json:"cidr,omitempty"`
	// CIDRGroupRef is a reference to a CiliumCIDRGroup object.
	// A CiliumCIDRGroup contains a list of CIDRs that the endpoint, subject to
	// the rule, can (Ingress) or cannot (IngressDeny) receive connections from.
	//
	// +kubebuilder:validation:OneOf
	CIDRGroupRef CIDRGroupRef `json:"cidrGroupRef,omitempty"`

In fact, trying to apply this:

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: "cidr-group-ref-cnp"
spec:
  endpointSelector:
    matchLabels:
      run: app
  ingress:
  - fromCIDRSet:
    - cidrGroupRef: ""

is failing as expected:

The CiliumNetworkPolicy "cidr-group-ref-cnp" is invalid: spec.ingress[0].fromCIDRSet[0].cidrGroupRef: Invalid value: "": spec.ingress[0].fromCIDRSet[0].cidrGroupRef in body should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'

And a null value:

spec:
  endpointSelector:
    matchLabels:
      run: app
  ingress:
  - fromCIDRSet:
    - cidrGroupRef: null

is leading to a validation error, too.

[christarazi]

@pippolo84 I think the case is when the user applies a policy with a ref that doesn't exist, like

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: "cidr-group-ref-cnp"
spec:
  endpointSelector:
    matchLabels:
      run: app
  ingress:
  - fromCIDRSet:
    - cidrGroupRef: "doesn't-exist"

the metric is still incremented.

[pippolo84]

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
name: "cidr-group-ref-cnp"
spec:
endpointSelector:
matchLabels:
run: app
ingress:

• fromCIDRSet:
  • cidrGroupRef: "doesn't-exist"

If I'm not mistaken the proposed change is just checking the length of rule.CIDRGroupRef, that is, the length of the string "doesn't-exist" in your example policy.
If we want to avoid incrementing the metric for a non-existing reference I think we need to check cidrGroupCache to see if a CiliumCIDRGroup with that name actually exists.

[akstron]

Hi @pippolo84, probably I misunderstood the issue. What I understand from the conversation above is that we can add a particular groupRef, even though the object itself does not exist and we need to check this existence to resolve the issue.

What I suggest is we can use cidrGroupRefsToCIDRsSets, to translate the cidrGroupRefs we receive during metric update to get valid CIDRSets and check its length instead of the length of cidrGroupRefs.

[pippolo84]

I think the current approach is the correct one, thanks! I've left a suggestion to improve the comment.

Could you please squash this last commit into the previous one?
Also, you need to use your real name for the Signed-off-by line. I suggest to read the Cilium Contribution Guidelines and the Developer's Certificate of Origin section.

[ti-mo]

@akstron Looks like your second commit overwrites the first, please squash.

[joestringer]

It's likely that the test failures could be resolved by rebasing against the tip of the main branch.

[pippolo84]

Thanks! 🚀

[pippolo84]

/test