OSAC-1442: Add ExternalIP CRD types, controllers, and feedback controllers - #318
Conversation
|
@ori-amizur: This pull request references OSAC-1442 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds three new ChangesExternalIP CRD API, Conditions, and Controller Wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/externalipattachment_types.go`:
- Around line 28-31: Finish the PublicIP to ExternalIP rename by updating the
ExternalIP field’s JSON serialization tag in the ExternalIPAttachment spec to
use the new serialized name, and update any user-facing path strings that still
reference the old publicIP path. Check the ExternalIPAttachment type definition
and the related formatting/logging code that prints the field path so both the
CRD surface and displayed paths consistently use spec.externalIP instead of
spec.publicIP.
- Around line 33-37: The ExternalIPAttachment target is currently optional even
though the type comment says exactly one target field must be set, so add
admission validation to enforce that invariant. Update the ExternalIPAttachment
spec around ComputeInstance and the other target fields to require exactly one
non-empty target at creation/update, using Kubebuilder validation markers or a
custom validation webhook if needed, and ensure the validation matches the
existing reconcile nil-handling in ExternalIPAttachment.
In `@api/v1alpha1/externalippool_types.go`:
- Around line 38-44: The ImplementationStrategy field in ExternalIPPool is
documented as defaulting to metallb-l2, but the type definition does not declare
a kubebuilder default, so empty values can persist on create. Update the
ExternalIPPool spec field annotation to include the default for
ImplementationStrategy alongside the existing validation and immutability
markers, so generated CRDs and admission set metallb-l2 when the field is
omitted.
In `@config/crd/bases/osac.openshift.io_externalipattachments.yaml`:
- Around line 20-21: The ExternalIPAttachment API contract is still using the
old publicIP field and is missing enforcement for the required target, so update
the source type that defines the spec to rename the field to externalIP and add
validation that exactly one target is set (at minimum require computeInstance,
or add a spec-level CEL rule if multiple targets are planned). Then regenerate
the CRD manifests instead of editing the generated config/crd output directly,
using the ExternalIPAttachment type and its spec/validation definitions as the
source of truth.
In `@config/crd/bases/osac.openshift.io_externalippools.yaml`:
- Around line 70-80: The ExternalIPPool implementationStrategy field is
described as defaulting to metallb-l2 but the API type does not declare that
default, so update the source type for implementationStrategy with the
kubebuilder default marker in the ExternalIPPool/OpenShift API definition and
regenerate the CRD manifests. Make the change in the struct that owns
implementationStrategy, then run the normal generation workflow so the default
appears in the generated CRD instead of hand-editing the YAML.
- Around line 60-69: The ExternalIPPool CRD currently only enforces cidrs
immutability and still allows arbitrary strings, so add source-level validation
in the ExternalIPPool API type (the spec/cidrs field in externalippool_types.go)
to require valid CIDR syntax and ensure each CIDR matches spec.ipFamily using
kubebuilder/CEL or equivalent admission validation. Keep the validation on the
API type, not the generated CRD, then regenerate the manifests so the changes
flow into the config/crd output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 515cc59d-0880-4916-8be7-0667e0676fc0
⛔ Files ignored due to path filters (30)
internal/api/osac/private/v1/baremetal_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (34)
api/v1alpha1/externalip_conditions.goapi/v1alpha1/externalip_types.goapi/v1alpha1/externalip_types_test.goapi/v1alpha1/externalipattachment_conditions.goapi/v1alpha1/externalipattachment_types.goapi/v1alpha1/externalipattachment_types_test.goapi/v1alpha1/externalippool_conditions.goapi/v1alpha1/externalippool_types.goapi/v1alpha1/externalippool_types_test.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/scheme_test.goapi/v1alpha1/zz_generated.deepcopy.gobuf.gen.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_externalipattachments.yamlconfig/crd/bases/osac.openshift.io_externalippools.yamlconfig/crd/bases/osac.openshift.io_externalips.yamlconfig/rbac/role.yamlinternal/controller/constants_common.gointernal/controller/externalip_controller.gointernal/controller/externalip_controller_test.gointernal/controller/externalip_feedback_controller.gointernal/controller/externalip_feedback_controller_test.gointernal/controller/externalip_names.gointernal/controller/externalipattachment_controller.gointernal/controller/externalipattachment_controller_test.gointernal/controller/externalipattachment_feedback_controller.gointernal/controller/externalipattachment_feedback_controller_test.gointernal/controller/externalipattachment_names.gointernal/controller/externalippool_controller.gointernal/controller/externalippool_controller_test.gointernal/controller/externalippool_feedback_controller.gointernal/controller/externalippool_feedback_controller_test.gointernal/controller/externalippool_names.go
87d47af to
036b336
Compare
|
🔴 CI Triage: Root cause: The operator is crash-looping because it lacks RBAC permissions to list the newly added ExternalIP, ExternalIPPool, and ExternalIPAttachment CRDs, causing E2E tests to time out waiting for resources to reconcile. Explanation: The PR adds new CRDs ( Evidence:
Suggestion: Update the operator's ClusterRole in the Helm chart (likely in Prow job | Build For deeper investigation, use the |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/externalip_types.go`:
- Around line 80-82: The new status.Conditions field is missing kubebuilder
list-map markers, so it will not merge by condition type as intended. Add the
listType=map and listMapKey=type markers to this Conditions definition in
ExternalIPStatus, and apply the same update to every other newly added
[]metav1.Condition field in this change, then regenerate the CRDs so the schema
reflects the merge semantics.
In `@api/v1alpha1/externalippool_types.go`:
- Around line 84-95: The status counters in ExternalIPPoolStatus use int64,
which cannot safely represent common IPv6 pool sizes like a /64. Update the
ExternalIPPoolStatus fields Total, Allocated, and Available to avoid overflow by
either adding validation that restricts supported IPv6 prefix sizes in the
CRD/spec or switching these fields to a wider representation, and make sure any
related status calculations and serializers in ExternalIPPoolStatus are updated
consistently.
- Around line 25-36: The ExternalIPPool schema in ExternalIPPoolSpec currently
enforces only non-empty CIDRs and an IPFamily enum, so invalid CIDR strings or
CIDRs that do not match the declared family can still be admitted. Add
admission-time validation on the CIDRs field, using the ExternalIPPoolSpec/CIDRs
and IPFamily symbols, so each CIDR is parsed and checked against the selected
family and rejected if it does not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cabddead-796c-499c-83de-6829776442b1
⛔ Files ignored due to path filters (30)
internal/api/osac/private/v1/baremetal_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
api/v1alpha1/externalip_conditions.goapi/v1alpha1/externalip_types.goapi/v1alpha1/externalip_types_test.goapi/v1alpha1/externalipattachment_conditions.goapi/v1alpha1/externalipattachment_types.goapi/v1alpha1/externalipattachment_types_test.goapi/v1alpha1/externalippool_conditions.goapi/v1alpha1/externalippool_types.goapi/v1alpha1/externalippool_types_test.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/scheme_test.goapi/v1alpha1/zz_generated.deepcopy.gobuf.gen.yamlcharts/operator-crds/templates/osac.openshift.io_externalipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_externalippools.yamlcharts/operator-crds/templates/osac.openshift.io_externalips.yamlcharts/operator/templates/clusterrole.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_externalipattachments.yamlconfig/crd/bases/osac.openshift.io_externalippools.yamlconfig/crd/bases/osac.openshift.io_externalips.yamlconfig/rbac/role.yamlinternal/controller/constants_common.gointernal/controller/externalip_controller.gointernal/controller/externalip_controller_test.gointernal/controller/externalip_feedback_controller.gointernal/controller/externalip_feedback_controller_test.gointernal/controller/externalip_names.gointernal/controller/externalipattachment_controller.gointernal/controller/externalipattachment_controller_test.gointernal/controller/externalipattachment_feedback_controller.gointernal/controller/externalipattachment_feedback_controller_test.gointernal/controller/externalipattachment_names.gointernal/controller/externalippool_controller.gointernal/controller/externalippool_controller_test.gointernal/controller/externalippool_feedback_controller.gointernal/controller/externalippool_feedback_controller_test.gointernal/controller/externalippool_names.go
| // Conditions holds an array of metav1.Condition that describe the state of the ExternalIP | ||
| // +kubebuilder:validation:Optional | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that all new condition lists have list-map markers in source
# and generate the expected CRD schema markers downstream.
echo "=== Source condition fields ==="
rg -n -C3 'Conditions \[\]metav1\.Condition|listType=map|listMapKey=type' api/v1alpha1
echo
echo "=== Generated CRD condition schemas ==="
rg -n -C2 'conditions:|x-kubernetes-list-type|x-kubernetes-list-map-keys' config/crd/bases charts/operator-crds/templatesRepository: osac-project/osac-operator
Length of output: 30868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== condition field annotations in source ==="
sed -n '74,88p' api/v1alpha1/externalip_types.go
echo
sed -n '116,128p' api/v1alpha1/tenant_types.go
echo
sed -n '262,272p' api/v1alpha1/computeinstance_types.go
echo
echo "=== any existing kubebuilder patch markers in comments ==="
rg -n '// \+patchStrategy=merge|// \+patchMergeKey=' api/v1alpha1
echo
echo "=== any listType/listMapKey on metav1.Condition fields ==="
rg -n -C2 'Conditions \[\]metav1\.Condition' api/v1alpha1Repository: osac-project/osac-operator
Length of output: 2328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== condition field annotations in source ==="
sed -n '74,88p' api/v1alpha1/externalip_types.go
echo
sed -n '116,128p' api/v1alpha1/tenant_types.go
echo
sed -n '262,272p' api/v1alpha1/computeinstance_types.go
echo
echo "=== any existing kubebuilder patch markers in comments ==="
rg -n '// \+patchStrategy=merge|// \+patchMergeKey=' api/v1alpha1
echo
echo "=== any listType/listMapKey on metav1.Condition fields ==="
rg -n -C2 'Conditions \[\]metav1\.Condition' api/v1alpha1Repository: osac-project/osac-operator
Length of output: 2328
Add +listType=map / +listMapKey=type to every new status.conditions field.
patchStrategy/patchMergeKey alone don’t give CRD merge-by-type semantics; these arrays are still treated atomically unless the kubebuilder list-map markers are present. Apply the same fix to the other new []metav1.Condition fields in this change, then regenerate the CRDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/externalip_types.go` around lines 80 - 82, The new
status.Conditions field is missing kubebuilder list-map markers, so it will not
merge by condition type as intended. Add the listType=map and listMapKey=type
markers to this Conditions definition in ExternalIPStatus, and apply the same
update to every other newly added []metav1.Condition field in this change, then
regenerate the CRDs so the schema reflects the merge semantics.
Source: Coding guidelines
| // CIDRs is the list of CIDR blocks for this pool. All CIDRs must match the declared IPFamily. | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="cidrs is immutable" | ||
| CIDRs []string `json:"cidrs"` | ||
|
|
||
| // IPFamily indicates the IP address family for this pool (IPv4 or IPv6) | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Type=string | ||
| // +kubebuilder:validation:Enum=IPv4;IPv6 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ipFamily is immutable" | ||
| IPFamily string `json:"ipFamily"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate cidrs against ipFamily at admission.
CIDRs is documented as “All CIDRs must match the declared IPFamily”, but this schema only enforces a non-empty slice plus the IPFamily enum. Inputs like ["not-a-cidr"] or IPv4 CIDRs with ipFamily: IPv6 will be persisted and fail later in reconciliation/provisioning.
As per path instructions, "Validate at trust boundaries with allow-lists, not deny-lists".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/externalippool_types.go` around lines 25 - 36, The
ExternalIPPool schema in ExternalIPPoolSpec currently enforces only non-empty
CIDRs and an IPFamily enum, so invalid CIDR strings or CIDRs that do not match
the declared family can still be admitted. Add admission-time validation on the
CIDRs field, using the ExternalIPPoolSpec/CIDRs and IPFamily symbols, so each
CIDR is parsed and checked against the selected family and rejected if it does
not match.
Source: Path instructions
| // Total is the total number of usable IP addresses across all CIDRs in this pool. | ||
| // Uses int64 to accommodate large IPv6 CIDR ranges. | ||
| // +kubebuilder:validation:Optional | ||
| Total int64 `json:"total,omitempty"` | ||
|
|
||
| // Allocated is the number of IPs currently allocated from the pool. | ||
| // +kubebuilder:validation:Optional | ||
| Allocated int64 `json:"allocated,omitempty"` | ||
|
|
||
| // Available is the number of IPs available for allocation. | ||
| // +kubebuilder:validation:Optional | ||
| Available int64 `json:"available,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
int64 cannot represent common IPv6 pool sizes.
A single IPv6 /64 already has 2^64 addresses, which exceeds math.MaxInt64. Since the spec accepts IPv6 pools, status.total, allocated, and available will overflow or require truncation for valid inputs. Either cap supported IPv6 prefix sizes via validation or switch these counters to a wider representation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/externalippool_types.go` around lines 84 - 95, The status
counters in ExternalIPPoolStatus use int64, which cannot safely represent common
IPv6 pool sizes like a /64. Update the ExternalIPPoolStatus fields Total,
Allocated, and Available to avoid overflow by either adding validation that
restricts supported IPv6 prefix sizes in the CRD/spec or switching these fields
to a wider representation, and make sure any related status calculations and
serializers in ExternalIPPoolStatus are updated consistently.
|
💀 CI Triage: Root cause: The merge of osac-installer PR 328 broke the main branch by enabling secret-based AAP token configuration, which triggers a Kubernetes strategic merge patch conflict during Helm upgrade on snapshot-booted clusters. Explanation: The failure is caused by a strategic merge patch conflict when upgrading the 'osac-operator' deployment during the cluster refresh phase. Recently merged PR 316 in 'osac-operator' introduced support for loading the AAP token from a secret via 'valueFrom.secretKeyRef'. Subsequently, PR 328 in 'osac-installer' was merged, which updated the CI Helm values ('values/vmaas-ci/values.yaml') to use this new 'tokenSecret' configuration. However, the pre-built cluster snapshot contains an existing 'osac-operator' deployment where 'OSAC_AAP_TOKEN' is set as a literal 'value'. When 'helm upgrade' runs during the boot step, Kubernetes attempts to merge the new deployment spec into the existing one. Because the strategic merge patch merges list items by name, the resulting environment variable spec contains both 'value' and 'valueFrom', which is invalid in Kubernetes and causes the upgrade to fail with a validation error. This is a broken_main issue affecting all PRs and periodic runs across all repositories. Evidence: Suggestion: To resolve this, apply one of the following fixes:
{{- if and .Values.aap.tokenSecret .Values.aap.tokenSecret.name }}
- name: OSAC_AAP_TOKEN
value: null
valueFrom:
secretKeyRef:
name: {{ .Values.aap.tokenSecret.name }}
key: {{ .Values.aap.tokenSecret.key | default "token" }}
optional: true
Prow job | Build For deeper investigation, use the |
…llers Add ExternalIPPool, ExternalIP, and ExternalIPAttachment as new operator resources (OSAC-1480, OSAC-1481, OSAC-1482). Each mirrors its PublicIP counterpart with the PublicIP → ExternalIP rename. Includes CRD type definitions, provisioning controllers, feedback controllers, tests, generated deepcopy methods, CRD manifests, and controller registration in main.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/scheme_test.go (1)
86-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest claims "exactly 24" but never asserts the count. The loop only verifies each expected kind is present; it doesn't check that no other kinds were registered. An accidental extra registration would pass undetected. Assert the actual count to match the description.
Suggested addition
for _, kind := range expectedKinds { gvk := v1alpha1.GroupVersion.WithKind(kind) knownTypes := scheme.KnownTypes(v1alpha1.GroupVersion) _, found := knownTypes[gvk.Kind] Expect(found).To(BeTrue(), "expected kind %s to be registered", kind) } + Expect(expectedKinds).To(HaveLen(24))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/scheme_test.go` around lines 86 - 108, The test in scheme_test.go only checks that each expected kind is present in scheme.KnownTypes(v1alpha1.GroupVersion), but it never verifies the total number of registered types. Update the It block around expectedKinds and the knownTypes lookup to also assert the count matches the “exactly 24 registered types plus internal types” expectation, so any accidental extra registration fails the test.
♻️ Duplicate comments (1)
config/crd/bases/osac.openshift.io_externalipattachments.yaml (1)
58-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a target before making the attachment spec immutable.
computeInstancesays exactly one target must be set, but onlyexternalIPis required. A user can createspec.externalIPwithout any target, and Line 72 then makes that invalid attachment unfixable in-place.Expected generated shape after updating the API type
required: - externalIP + - computeInstance type: object x-kubernetes-validations: + - message: exactly one target field must be set + rule: has(self.computeInstance) - message: spec is immutable after creation rule: self == oldSelfIf more target fields are planned, replace the simple required field with an exact-one CEL rule in the source type.
As per coding guidelines, update the API type source and regenerate manifests instead of editing generated
config/crd/files directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/crd/bases/osac.openshift.io_externalipattachments.yaml` around lines 58 - 74, The ExternalIPAttachment spec currently allows creating an attachment without any target because only externalIP is required, and the immutability rule then makes that invalid state impossible to correct. Update the API type source for the ExternalIPAttachment spec to require a target field as well—either add the missing required target in the struct/schema or, if multiple targets are planned, use an exact-one CEL validation on the source type. Regenerate the CRD manifests from the updated source instead of editing the generated config/crd artifact directly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@buf.gen.yaml`:
- Line 27: The Buf module bump to v0.0.67 needs to be paired with regenerated
API artifacts and a checked-in buf.lock for reproducible generation. Update the
Buf configuration via the existing buf.gen.yaml / internal/api generation flow
by running buf generate against the new module version, then commit the
resulting buf.lock alongside any regenerated files so the generated code and
dependency resolution stay in sync.
---
Outside diff comments:
In `@api/v1alpha1/scheme_test.go`:
- Around line 86-108: The test in scheme_test.go only checks that each expected
kind is present in scheme.KnownTypes(v1alpha1.GroupVersion), but it never
verifies the total number of registered types. Update the It block around
expectedKinds and the knownTypes lookup to also assert the count matches the
“exactly 24 registered types plus internal types” expectation, so any accidental
extra registration fails the test.
---
Duplicate comments:
In `@config/crd/bases/osac.openshift.io_externalipattachments.yaml`:
- Around line 58-74: The ExternalIPAttachment spec currently allows creating an
attachment without any target because only externalIP is required, and the
immutability rule then makes that invalid state impossible to correct. Update
the API type source for the ExternalIPAttachment spec to require a target field
as well—either add the missing required target in the struct/schema or, if
multiple targets are planned, use an exact-one CEL validation on the source
type. Regenerate the CRD manifests from the updated source instead of editing
the generated config/crd artifact directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0eb5ba41-46b4-46f8-936e-55bdc5ee577e
⛔ Files ignored due to path filters (30)
internal/api/osac/private/v1/baremetal_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachment_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_attachments_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_pools_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/external_ips_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/ip_family_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_group_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
api/v1alpha1/externalip_conditions.goapi/v1alpha1/externalip_types.goapi/v1alpha1/externalip_types_test.goapi/v1alpha1/externalipattachment_conditions.goapi/v1alpha1/externalipattachment_types.goapi/v1alpha1/externalipattachment_types_test.goapi/v1alpha1/externalippool_conditions.goapi/v1alpha1/externalippool_types.goapi/v1alpha1/externalippool_types_test.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/scheme_test.goapi/v1alpha1/zz_generated.deepcopy.gobuf.gen.yamlcharts/operator-crds/templates/osac.openshift.io_externalipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_externalippools.yamlcharts/operator-crds/templates/osac.openshift.io_externalips.yamlcharts/operator/templates/clusterrole.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_externalipattachments.yamlconfig/crd/bases/osac.openshift.io_externalippools.yamlconfig/crd/bases/osac.openshift.io_externalips.yamlconfig/rbac/role.yamlinternal/controller/constants_common.gointernal/controller/externalip_controller.gointernal/controller/externalip_controller_test.gointernal/controller/externalip_feedback_controller.gointernal/controller/externalip_feedback_controller_test.gointernal/controller/externalip_names.gointernal/controller/externalipattachment_controller.gointernal/controller/externalipattachment_controller_test.gointernal/controller/externalipattachment_feedback_controller.gointernal/controller/externalipattachment_feedback_controller_test.gointernal/controller/externalipattachment_names.gointernal/controller/externalippool_controller.gointernal/controller/externalippool_controller_test.gointernal/controller/externalippool_feedback_controller.gointernal/controller/externalippool_feedback_controller_test.gointernal/controller/externalippool_names.go
| inputs: | ||
|
|
||
| - module: buf.build/osac-project/private-api:v0.0.65 | ||
| - module: buf.build/osac-project/private-api:v0.0.67 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the pinned version and that generated client compiles/matches expectations.
rg -n 'private-api:v0\.0\.6[0-9]' buf.gen.yaml buf.lock 2>/dev/null
fd -t f . internal/api | head -50Repository: osac-project/osac-operator
Length of output: 3660
🏁 Script executed:
#!/bin/bash
# Check for buf.lock to verify the exact commit hash pinned for v0.0.67
if [ -f "buf.lock" ]; then
echo "=== buf.lock contents ==="
rg -A 2 "private-api" buf.lock
else
echo "buf.lock not found in repository root."
fd -f "buf.lock" . 2>/dev/null
fi
# Also check the very first line of one generated file for version comments if available
head -n 5 internal/api/osac/private/v1/access_key_type.pb.goRepository: osac-project/osac-operator
Length of output: 420
🌐 Web query:
buf build version pinning buf.lock usage
💡 Result:
In the Buf ecosystem, the buf.lock file is a machine-generated file that records the exact state of external dependencies for your workspace, ensuring that builds are reproducible, protected against tampering, and consistent across environments [1][2][3]. It should never be edited by hand and should always be checked into version control [2][3]. Core Functions of buf.lock: 1. Dependency Version Pinning: While your buf.yaml file declares the dependencies you want to use (often using flexible references), the buf.lock file records the specific commit and cryptographic manifest digest for every direct and transitive dependency [1][2]. This locks your project to a known, stable version [2]. 2. Tamper-Proofing: Every BSR commit is content-addressed by a cryptographic manifest digest [3]. The Buf CLI records this digest in buf.lock [1][3]. During every buf build, the CLI verifies the downloaded dependency's content against this digest; if they do not match, the build fails, protecting you from man-in-the-middle attacks or unauthorized upstream changes [1][3]. 3. Reproducibility: By checking buf.lock into source control, you ensure that every developer and build system uses the exact same version of dependencies, preventing "it works on my machine" issues [2][3]. Workflow for Managing buf.lock: - Initial Setup: When you add dependencies to the deps field in buf.yaml, you must generate or update the buf.lock file [1][2]. - Update Dependencies: Run the command buf dep update at the root of your workspace [2][4]. This fetches the latest commits for your dependencies, updates the commit and digest information in buf.lock, and includes any new transitive dependencies [1][4]. - Clean Up: If you remove dependencies from buf.yaml, you can run buf dep prune to remove unused entries from buf.lock [1]. - Code Review: Treat changes to buf.lock as security-relevant during code reviews, as they represent shifts in the underlying code your project depends on [3].
Citations:
- 1: https://buf.build/docs/bsr/module/dependency-management/
- 2: https://buf.build/docs/configuration/v2/buf-lock/
- 3: https://buf.build/docs/bsr/module/tamper-proofing/
- 4: https://buf.build/docs/reference/cli/buf/dep/update/
🏁 Script executed:
#!/bin/bash
# Attempt to resolve the exact commit for v0.0.67 from BSR to verify if local files match the latest hash
# Since buf.lock is missing, this is a high risk.
# Let's check the git history of the generated files to see when they were updated.
git log --oneline -n 10 internal/api/ | head -10
# Check if the comment in generated files contains a version tag or timestamp
grep -r "v0.0.67" internal/api/ || echo "No visible v0.0.67 tag in file headers"
# Check the git log of buf.gen.yaml to see when the version was changed
git log --oneline -n 10 buf.gen.yamlRepository: osac-project/osac-operator
Length of output: 369
Ensure generated code matches the v0.0.67 bump and add buf.lock.
The buf.gen.yaml and internal/api/ were updated in the same commit (d56c9a3), indicating generation likely ran correctly. However, buf.lock is missing.
Per AGENTS.md and BSR best practices, run buf generate (which updates dependencies) and commit the resulting buf.lock to guarantee reproducible builds and prevent silent drift between the module version and the actual generated code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@buf.gen.yaml` at line 27, The Buf module bump to v0.0.67 needs to be paired
with regenerated API artifacts and a checked-in buf.lock for reproducible
generation. Update the Buf configuration via the existing buf.gen.yaml /
internal/api generation flow by running buf generate against the new module
version, then commit the resulting buf.lock alongside any regenerated files so
the generated code and dependency resolution stay in sync.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, ori-amizur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Add ExternalIPPool, ExternalIP, and ExternalIPAttachment as new operator resources ( OSAC-1480 , OSAC-1481 , OSAC-1482 ). Each mirrors its PublicIP counterpart with the PublicIP → ExternalIP rename.
Includes CRD type definitions, provisioning controllers, feedback controllers, tests, generated deepcopy methods, CRD manifests, and controller registration in main.go.
Summary by CodeRabbit