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.

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

[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	7ebee29	link	true	/test lint

Full PR test history. Your PR dashboard.

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.