DNM: Bpfman operator APIs review

[andreaskaris]

Apis review for bpfman-operator in preparation for GA.

This is a follow-up to #2221
The second commit adds a new Config CRD (the plan is to squash all commits together eventually but right now for the review for clarity to keep the "old" stuff and the new additions separate)

[openshift-ci]

Hello @andreaskaris! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JoelSpeed]

Testing out the Claude API review I'm working on

  🔍 OpenShift API Review Report

  Executive Summary

  ✅ Linting Status: All linting checks passed
  successfully❌ Review Status: FAILED - Critical
  documentation issues found in new bpfman-operator
  APIs📊 Files Reviewed: 20 new bpfman-operator
  v1alpha1 API files

  Critical Issues Found: 4 documentation violations
  requiring immediate attention

  ---
  🚨 Critical Issues Found

  All issues are in newly added bpfman-operator
  v1alpha1 API files:

  Issue 1: InterfaceSelector structural validation 
  not documented

  +56: Missing structural validation
  documentationCurrent (problematic) code:
  // InterfaceSelector describes the set of 
  interfaces to attach a program to.
  // +kubebuilder:validation:MaxProperties=1
  // +kubebuilder:validation:MinProperties=1
  type InterfaceSelector struct {

  Suggested change:
  - // InterfaceSelector describes the set of 
  interfaces to attach a program to.
  + // InterfaceSelector describes the set of 
  interfaces to attach a program to.
  + // Exactly one of interfacesDiscoveryConfig, 
  interfaces, or primaryNodeInterface must be 
  specified.

  Explanation: The MaxProperties=1 and
  MinProperties=1 validation markers enforce that
  exactly one field must be set, but this critical
  requirement is not documented for users.

  ---
  Issue 2: ByteCodeSelector structural validation not
   documented

  +232: Missing structural validation
  documentationCurrent (problematic) code:
  // ByteCodeSelector defines the various ways to 
  reference BPF bytecode objects.
  // +kubebuilder:validation:MaxProperties=1
  // +kubebuilder:validation:MinProperties=1
  type ByteCodeSelector struct {

  Suggested change:
  - // ByteCodeSelector defines the various ways to 
  reference BPF bytecode objects.
  + // ByteCodeSelector defines the various ways to 
  reference BPF bytecode objects.
  + // Exactly one of image or path must be 
  specified.

  Explanation: Similar structural constraint not
  documented - users won't understand that exactly
  one configuration method must be chosen.

  ---
  Issue 3: Pattern validation not explained in human 
  terms

  +243: Pattern constraint lacks human-readable
  explanationCurrent (problematic) code:
  // path is an optional field and used to specify a 
  bytecode object file via
  // filepath on a Kubernetes node.
  // +optional
  // 
  +kubebuilder:validation:Pattern=`^(/[^/\0]+)+/?$`
  Path *string `json:"path,omitempty"`

  Suggested change:
  // path is an optional field and used to specify a
  bytecode object file via
  - // filepath on a Kubernetes node.
  + // filepath on a Kubernetes node. The path must 
  be an absolute path starting with '/' 
  + // and cannot contain null characters.

  Explanation: The pattern ^(/[^/\0]+)+/?$ enforces
  absolute paths without null characters, but this
  requirement is not documented in human-readable
  terms.

  ---
  Issue 4: Pattern validation mismatch with 
  documentation

  +33: Documentation doesn't match validation
  patternCurrent (problematic) code:
  // name is a required field and is the name of the 
  function that is the entry
  // point for the eBPF program. name must not be an 
  empty string, must not
  // exceed 64 characters in length, must start with 
  alpha characters and must
  // only contain alphanumeric characters.
  // +required
  // +kubebuilder:validation:Pattern="^[a-zA-Z][a-zA-
  Z0-9_]+."

  Suggested change:
  // name is a required field and is the name of the
  function that is the entry
  // point for the eBPF program. name must not be an
  empty string, must not
  // exceed 64 characters in length, must start with
  alpha characters and must
  - // only contain alphanumeric characters.
  + // only contain alphanumeric characters and 
  underscores.

  Explanation: The pattern ^[a-zA-Z][a-zA-Z0-9_]+.
  allows underscores but the comment says "only
  alphanumeric characters." This mismatch will
  confuse users.

  ---
  ✅ What's Working Well

  1. Enum Documentation: The PullPolicy enum properly
   documents valid values (Always, Never,
  IfNotPresent)
  2. Optional Field Behavior: Most optional fields
  explain behavior when omitted (e.g., "defaults to
  IfNotPresent")
  3. XValidation Rules: Complex validation rules
  include clear error messages
  4. Required Fields: All required fields are
  properly marked and documented
  5. Field Comments: Most fields have comprehensive
  documentation

  ---
  🎯 Resolution Required

  This API review FAILS due to violations of
  OpenShift API documentation standards:

  1. Structural validation constraints
  (MaxProperties/MinProperties) must be documented
  2. Pattern validations must be explained in
  human-readable terms
  3. Documentation must match validation patterns
  exactly

  ---
  📊 Next Steps

  1. Fix all 4 critical issues identified above
  2. Re-run API review to verify compliance
  3. Ensure all kubebuilder validation markers are
  documented in field comments

  The bpfman-operator APIs are well-structured and
  mostly well-documented, but these critical gaps
  must be addressed before merge.

  ✅ Local API review complete.

Looks like its picked up a few helpful things in there about markers that are not documented in the godocs

[andreaskaris]

@JoelSpeed cool I'll address those and I'll ping you once I need that human review (I so far fixed some issues with the Config resource, first), thanks!

[andreaskaris]

@JoelSpeed This is ready for another AI review and/or human review.

Changes:

• addressed the issues that Claude flagged with some of the comments.
• made changes to the Config type
• removed unused method GetClientObject from bpfman-operator/apis/v1alpha1/bpf_application_state_types.go and bpfman-operator/apis/v1alpha1/cluster_bpf_application_state_types.go

Question: according to https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#no-functions:

Do not add functions to the openshift/api. Functions seem innocuous, but they have significant side effects over time.

    Dependency chain. We want our dependency chain on openshift/api to be as short as possible to avoid conflicts when they are vendored into other projects.
    Building interfaces on APIs. Building interfaces on top of our structs is an anti-goal. Even the interfaces we have today, runtime.Object and meta.Accessor, cause pain when mismatched levels result in structs dropping in and out of type compliance

The simplest line is "no functions". Functions can be added in a separate repo, possibly library-go if there are sufficient consumers. Helpers for accessing labels and annotations are not recommended.

So should we remove the methods in the aforementioned files, or are those o.k.?

Thanks

[JoelSpeed]

So should we remove the methods in the aforementioned files, or are those o.k.?

It appears (and I haven't checked the whole diff) that most if not all of your functions are "getters", since getters don't introduce new dependencies, they are fairly innocuous, but they do also form part of your API surface once you have introduced them. Personally, I'd lean on removing them and just fetching the fields directly, but they don't look like they'll necessarily cause issues

[andreaskaris]

I'm still going through the review progress; meanwhile, some notes:

Here's my current prompt for the api-review to Claude, just to make some progress.

For the following /api-review: ignore uint to int conversion; ignore pointer conversions in statuses; ignore FEntry and FExit omitzero hint; focus only on bpfman issues; ignore bool/*bool conversion to string fields

My doubts / questions:

1. I thought we should avoid pointers, but the /api-review insists that I use pointers in the Status resources.
2. The bpfman-operator communicates with bpfman via protobuf, and the fields are acutally all uint32. So I'm a bit afraid of using int as that might cause loss when reading data from bpfman
3. I don't understand the omitzero hint stuff. When I set it, it asks me to set it on more fields, and when I do so, it asks me to convert fields to pointers IIRC
4. The /api-review flags that +required fields should have omitempty (which makes no sense to me). Then, when I add omitempty, on the next round, it flags that +required fields should not have omitempty :-D
5. I ignored conversion from bool to string so far - mainly because we have a dependency from one repo to another for the bpfman-operator integration tests; I'm also wondering if it's necessary, or just a suggestion

[andreaskaris]

O.k., I gave up on the Claude /api-review. Instead, I'm running:

[akaris@workstation bpfman-operator (bpfman-operator-apis-review)]$ /home/akaris/development/api/tools/_output/bin/linux/amd64/golangci-lint run --new-from-rev=master

I fixed all the trivial stuff so far, with the following remaining:

bpfman-operator/apis/v1alpha1/bpf_application_state_types.go:165:2: optionalfields: field Status has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer. (kubeapilinter)
        Status BpfApplicationStateStatus `json:"status,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/bpf_application_types.go:200:2: optionalfields: field Status has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer. (kubeapilinter)
        Status BpfAppStatus `json:"status,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_bpf_application_state_types.go:216:2: optionalfields: field Status has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer. (kubeapilinter)
        Status ClBpfApplicationStateStatus `json:"status,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_kprobe_program_types.go:55:2: integers: field Offset should not use unsigned integers, use only int32 or int64 and apply validation to ensure the value is positive (kubeapilinter)
        Offset uint64 `json:"offset"`
        ^
bpfman-operator/apis/v1alpha1/cluster_kprobe_program_types.go:86:2: integers: field Offset should not use unsigned integers, use only int32 or int64 and apply validation to ensure the value is positive (kubeapilinter)
        Offset uint64 `json:"offset"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tc_program_types.go:55:2: optionalfields: field NetworkNamespaces does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        NetworkNamespaces *ClNetworkNamespaceSelector `json:"networkNamespaces,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tc_program_types.go:80:2: optionalfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tc_program_types.go:140:2: requiredfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tcx_program_types.go:53:2: optionalfields: field NetworkNamespaces does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        NetworkNamespaces *ClNetworkNamespaceSelector `json:"networkNamespaces,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tcx_program_types.go:78:2: optionalfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_tcx_program_types.go:123:2: requiredfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority"`
        ^
bpfman-operator/apis/v1alpha1/cluster_uprobe_program_types.go:56:2: integers: field Offset should not use unsigned integers, use only int32 or int64 and apply validation to ensure the value is positive (kubeapilinter)
        Offset uint64 `json:"offset,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_uprobe_program_types.go:75:2: optionalfields: field Containers does not allow the zero value. The field does not need to be a pointer. (kubeapilinter)
        Containers *ClContainerSelector `json:"containers,omitzero"`
        ^
bpfman-operator/apis/v1alpha1/cluster_xdp_program_types.go:55:2: optionalfields: field NetworkNamespaces does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        NetworkNamespaces *ClNetworkNamespaceSelector `json:"networkNamespaces,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_xdp_program_types.go:67:2: optionalfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/cluster_xdp_program_types.go:121:2: requiredfields: field Priority has a valid zero value (0) and should be a pointer. (kubeapilinter)
        Priority int32 `json:"priority"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:32:2: nobools: field InterfaceAutoDiscovery pointer should not use a bool. Use a string type with meaningful constant values as an enum. (kubeapilinter)
        InterfaceAutoDiscovery *bool `json:"interfaceAutoDiscovery,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:92:2: nobools: field PrimaryNodeInterface pointer should not use a bool. Use a string type with meaningful constant values as an enum. (kubeapilinter)
        PrimaryNodeInterface *bool `json:"primaryNodeInterface,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:183:2: nomaps: GlobalData should not use a map type, use a list type with a unique name/identifier instead (kubeapilinter)
        GlobalData map[string][]byte `json:"globalData,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:220:2: nobools: field ShouldAttach should not use a bool. Use a string type with meaningful constant values as an enum. (kubeapilinter)
        ShouldAttach bool `json:"shouldAttach"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:231:2: integers: field LinkId pointer should not use unsigned integers, use only int32 or int64 and apply validation to ensure the value is positive (kubeapilinter)
        LinkId *uint32 `json:"linkId,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:254:2: integers: field ProgramId pointer should not use unsigned integers, use only int32 or int64 and apply validation to ensure the value is positive (kubeapilinter)
        ProgramId *uint32 `json:"programId,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:278:2: optionalfields: field Image does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        Image *ByteCodeImage `json:"image,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/shared_types.go:319:2: optionalfields: field ImagePullSecret does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        ImagePullSecret *ImagePullSecretSelector `json:"imagePullSecret,omitempty"`
        ^
bpfman-operator/apis/v1alpha1/uprobe_program_types.go:77:2: optionalfields: field Containers does not allow the zero value. It must have the omitzero tag. (kubeapilinter)
        Containers ContainerSelector `json:"containers,omitempty"`
        ^
25 issues:
* kubeapilinter: 25

[everettraven]

Just wanted to give an update that this is on my list of things to take a look at. I'm catching up on things post shift week, so if I don't get around to leaving an initial review by EOD today, it will be top of my list for tomorrow.

[everettraven]

I've made it ~halfway through on this pass.

I'll have to circle back around to this one to continue where I've left off.

Most of the comments I've left here seem generally applicable throughout the APIs in this PR - please apply any changes wherever my comments apply.

[everettraven]

Nit: Be cautious of formatting like this. It may not render as you expect when users use oc explain or the OpenShift Console to see schema details of the type.

For OpenShift APIs, we generally try to stick to a sentence with the structure:

Allowed values are A, B, and C.

[everettraven]

You could have up to 1023 unique conditions? That's an awful lot of conditions.

[everettraven]

What other constraints exist for a Node name beyond just the length? is there a particular pattern they must conform to?

[everettraven]

A quick google search seems to point to them needing to be RFC 1123 DNS subdomain names: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names

[everettraven]

This reads as a Phase-like field that we generally try to avoid.

Any particular reason why this can't be represented as a condition or set of conditions?

[everettraven]

How do I find out the parent BpfApplication?

[everettraven]

Explain the constraint here in the GoDoc please.

[everettraven]

We strongly discourage the use of unsigned integers in APIs because their support is inconsistent across languages and libraries.

Instead, we prefer an explicit int32 or int64 with a minimum validation in place that ensures the value provided will never be negative. int64 field values should be capped at 2^53

[everettraven]

Do you need unique entries here?

[everettraven]

Any naming patterns that can be enforced here?

[everettraven]

Any patterns that can be enforced here to ensure a valid path string is provided?

[everettraven]

/test lint

[openshift-ci]

@andreaskaris: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify	7ebee29	link	true	/test verify
ci/prow/lint	58dc141	link	true	/test lint

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)

• do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}