openshift
DNM: Bpfman operator APIs review
Apis review for bpfman-operator in preparation for GA.
This is a follow-up to https://github.com/openshift/api/pull/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)
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[bot] avatar](https://avatars.githubusercontent.com/in/91280?v=4 "Published by a pub.dev verified publisher"){kind=small}
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
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
@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!
@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
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
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:
- I thought we should avoid pointers, but the /api-review insists that I use pointers in the Status resources.
- 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
- 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
- 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
- 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
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
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.
@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 | 7ebee29776de80e874f95ba6241bb24ce0516b23 | link | true | /test verify |
| ci/prow/lint | 58dc1410b3312eb61fe53e48ec4aeb11876310a2 | 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.
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
[!IMPORTANT]
Review skipped
Auto reviews are limited based on label configuration.
:no_entry_sign: 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.yamlfile in this repository. To trigger a single review, invoke the@coderabbitai reviewcommand.You can disable this status message by setting the
reviews.review_statustofalsein 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.
![coderabbitai[bot] avatar](https://avatars.githubusercontent.com/in/347564?v=4 "Published by a pub.dev verified publisher"){kind=small}