NE-2387: IngressController AWS NLB Security Group Selection#2914
NE-2387: IngressController AWS NLB Security Group Selection#2914aswinsuryan wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@aswinsuryan: This pull request references NE-2387 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 story 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. |
|
Hello @aswinsuryan! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds the 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/v1/types_ingresscontroller.go (1)
902-966: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNo validation test coverage included for the new field/type.
This PR introduces a new gated field with non-trivial CEL validation logic (duplicate check, format regex, length bounds), but no
.testsuite.yamlfiles are included in this diff to exercise the new validation rules (including the edge case above).As per coding guidelines ("Add validation tests in
<group>/<version>/tests/<crd-name>/... Runmake update-codegen-crdsto generate CRDs"), please add integration tests covering valid/invalidsecurityGroupsvalues, especially inputs lacking-. Want me to draft a.testsuite.yamlfor this?🤖 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 `@operator/v1/types_ingresscontroller.go` around lines 902 - 966, The new SecurityGroups field and SecurityGroupID type add CEL validation but have no test coverage. Add a .testsuite.yaml under the ingresscontroller CRD tests path to exercise the generated validation for IngressController/IngressControllerSpec.SecurityGroups, covering valid values, duplicate entries, max item limits, and invalid formats such as missing the sg- prefix or bad hex length. Use the new SecurityGroups and SecurityGroupID symbols to ensure the tests target the gated field and its validation rules, then regenerate CRDs with make update-codegen-crds.Source: Coding guidelines
🧹 Nitpick comments (2)
operator/v1/types_ingresscontroller.go (2)
902-926: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOmitted-field behavior only partially documented.
The comment explains behavior when the field is omitted only for the case where
NLBSecurityGroupModeisManagedin the CCM cloud-config. It's unclear what happens when the field is omitted and that mode is notManaged(e.g.Disabledor unset). Per the API review documentation requirements, every+optionalfield should fully explain omitted behavior.As per coding guidelines, "Ensure every
+optionalfield explains omitted behavior" — please clarify the omitted-field behavior for all CCM configurations, not just theManagedcase.🤖 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 `@operator/v1/types_ingresscontroller.go` around lines 902 - 926, The SecurityGroups field comment only documents the omitted case for NLBSecurityGroupMode=Managed, so update the doc on SecurityGroups to describe what happens when it is omitted for all CCM cloud-config modes, including Disabled or unset. Keep the existing behavior for the Managed case and add the missing omitted-field behavior so the +optional field is fully specified.Source: Coding guidelines
958-964: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLength range is broader than real AWS security group ID formats.
AWS generates security group IDs as either 8 hex characters (legacy) or 17 hex characters (current standard), e.g.
sg-903004f8orsg-0a1b2c3d4e5f6a7b8, not any length in between. The{8,17}range andMinLength=11/MaxLength=20would incorrectly accept e.g. a 12-hex-char suffix that AWS would never issue.Consider tightening to
^[0-9a-fA-F]{8}$|^[0-9a-fA-F]{17}$(and adjusting length bounds) if strict AWS format matching is desired.🤖 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 `@operator/v1/types_ingresscontroller.go` around lines 958 - 964, The SecurityGroupID validation in the ingress controller types is too permissive because it allows any hex suffix length between 8 and 17 instead of only the AWS-issued 8- or 17-character formats. Update the kubebuilder validation on SecurityGroupID to match only the two valid AWS shapes and align the MinLength/MaxLength constraints accordingly, using the existing SecurityGroupID annotation block in types_ingresscontroller.go.
🤖 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 `@operator/v1/types_ingresscontroller.go`:
- Around line 958-966: The SecurityGroupID validation currently splits on “-” in
a separate XValidation rule, which can still fail at runtime for values missing
the prefix. Update the validation on SecurityGroupID so the prefix and suffix
checks are combined safely, either by folding the sg- check into the same CEL
rule or by using a safe substring-based match instead of self.split("-", 2)[1].
Keep the existing intent of enforcing sg- followed by 8 to 17 hex characters
while avoiding indexed-expression errors.
---
Outside diff comments:
In `@operator/v1/types_ingresscontroller.go`:
- Around line 902-966: The new SecurityGroups field and SecurityGroupID type add
CEL validation but have no test coverage. Add a .testsuite.yaml under the
ingresscontroller CRD tests path to exercise the generated validation for
IngressController/IngressControllerSpec.SecurityGroups, covering valid values,
duplicate entries, max item limits, and invalid formats such as missing the sg-
prefix or bad hex length. Use the new SecurityGroups and SecurityGroupID symbols
to ensure the tests target the gated field and its validation rules, then
regenerate CRDs with make update-codegen-crds.
---
Nitpick comments:
In `@operator/v1/types_ingresscontroller.go`:
- Around line 902-926: The SecurityGroups field comment only documents the
omitted case for NLBSecurityGroupMode=Managed, so update the doc on
SecurityGroups to describe what happens when it is omitted for all CCM
cloud-config modes, including Disabled or unset. Keep the existing behavior for
the Managed case and add the missing omitted-field behavior so the +optional
field is fully specified.
- Around line 958-964: The SecurityGroupID validation in the ingress controller
types is too permissive because it allows any hex suffix length between 8 and 17
instead of only the AWS-issued 8- or 17-character formats. Update the
kubebuilder validation on SecurityGroupID to match only the two valid AWS shapes
and align the MinLength/MaxLength constraints accordingly, using the existing
SecurityGroupID annotation block in types_ingresscontroller.go.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8077e609-8fb4-4105-997a-13173f1f2637
⛔ Files ignored due to path filters (6)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerLBSecurityGroupsAWS.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (11)
features.mdfeatures/features.gooperator/v1/types_ingresscontroller.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
aswinsuryan
force-pushed
the
NE-2387/ingress-controller-lb-security-groups-aws
branch
from
0c8edfa to
0c5d225
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
needs-rebase
|
/assign @gcs278 |
|
/assign @bentito |
| // When this field is omitted, behavior depends on the | ||
| // NLBSecurityGroupMode setting in the CCM cloud-config: if set to | ||
| // Managed, the Cloud Controller Manager automatically creates and | ||
| // manages a security group for the NLB; if set to Disabled or | ||
| // unset, no security group is attached to the NLB. |
There was a problem hiding this comment.
Is this true that you can turn off SecurityGroup management in Openshift?
I thought the CCMO disables it when the feature gate is GA (which is is).
Maybe @mtulio could chime in too. Is this really exposed as a supported knob in OpenShift?
There was a problem hiding this comment.
It's not optional, Managed mode must be enforced in config.
afaict the only possibility to a service NLB does not have SGs will be on existing services (created prior this feature).
|
@aswinsuryan you will need to rebase - and take a look at verify & lint failures (you'll need to add an integration test). |
aswinsuryan
force-pushed
the
NE-2387/ingress-controller-lb-security-groups-aws
branch
from
0c5d225 to
6d44ef8
Compare
openshift-ci
Bot
removed
the
needs-rebase
Allows administrators to specify custom security groups for IngressControllers using AWS Network Load Balancers. Introduce under the `IngressControllerLBSecurityGroupsAWS` FeatureGate. Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
aswinsuryan
force-pushed
the
NE-2387/ingress-controller-lb-security-groups-aws
branch
from
6d44ef8 to
3056220
Compare
|
@aswinsuryan: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
cc @mfbonfigli |
|
@aswinsuryan Generally LGTM - you'll just need integration tests to clear CI. |
Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSecurityGroupsAWS.yaml (1)
1-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComprehensive test coverage for security group validation.
The test suite thoroughly covers the upstream contract from
types_ingresscontroller.go: valid 8-char and 17-char hex IDs, empty values, wrong prefix, non-hex characters, wrong hex length, MaxItems exceeded, duplicates, MaxLength exceeded, and mutability (add, change, remove). TheexpectedErrorstrings align with the kubebuilder validation markers (MinLength,MaxLength,MaxItems,XValidationmessages).Two minor edge-case gaps worth considering:
MaxItems upper boundary (exactly 5 valid groups) — the suite tests 6 groups (invalid) but doesn't confirm that exactly 5 is accepted. This validates the boundary itself.
Empty list
[](MinItems=1) — the suite tests omission (valid via+optional) and a single item, but not an explicit empty list which should triggerMinItems=1.➕ Suggested additional test cases
- name: Should not allow more than 5 security groups. initial: | apiVersion: operator.openshift.io/v1 kind: IngressController metadata: name: sgtestnlb namespace: openshift-ingress-operator spec: endpointPublishingStrategy: loadBalancer: scope: External providerParameters: type: AWS aws: type: NLB networkLoadBalancer: securityGroups: - sg-1234567890abcdef0 - sg-1234567890abcdef1 - sg-1234567890abcdef2 - sg-1234567890abcdef3 - sg-1234567890abcdef4 - sg-1234567890abcdef5 type: LoadBalancerService expectedError: "spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.networkLoadBalancer.securityGroups: Too many: 6: must have at most 5 items" + - name: Should allow exactly 5 security groups (MaxItems boundary). + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: sgtestnlb + namespace: openshift-ingress-operator + spec: + endpointPublishingStrategy: + loadBalancer: + scope: External + providerParameters: + type: AWS + aws: + type: NLB + networkLoadBalancer: + securityGroups: + - sg-1234567890abcdef0 + - sg-1234567890abcdef1 + - sg-1234567890abcdef2 + - sg-1234567890abcdef3 + - sg-1234567890abcdef4 + type: LoadBalancerService + expected: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: sgtestnlb + namespace: openshift-ingress-operator + spec: + httpEmptyRequestsPolicy: Respond + idleConnectionTerminationPolicy: Immediate + closedClientConnectionPolicy: Continue + endpointPublishingStrategy: + loadBalancer: + dnsManagementPolicy: Managed + scope: External + providerParameters: + type: AWS + aws: + type: NLB + networkLoadBalancer: + securityGroups: + - sg-1234567890abcdef0 + - sg-1234567890abcdef1 + - sg-1234567890abcdef2 + - sg-1234567890abcdef3 + - sg-1234567890abcdef4 + type: LoadBalancerService + - name: Should not allow an empty security groups list. + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: sgtestnlb + namespace: openshift-ingress-operator + spec: + endpointPublishingStrategy: + loadBalancer: + scope: External + providerParameters: + type: AWS + aws: + type: NLB + networkLoadBalancer: + securityGroups: [] + type: LoadBalancerService + expectedError: "spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.networkLoadBalancer.securityGroups: Too few: 0: must have at least 1 items"🤖 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 `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSecurityGroupsAWS.yaml` around lines 1 - 435, Add tests under onCreate for the securityGroups field in the IngressController cases: one with exactly five valid security group IDs that succeeds, and one with an explicitly empty securityGroups list that expects the MinItems=1 validation error. Keep the fixtures consistent with the existing NLB examples and assert the corresponding validation message.
🤖 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.
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSecurityGroupsAWS.yaml`:
- Around line 1-435: Add tests under onCreate for the securityGroups field in
the IngressController cases: one with exactly five valid security group IDs that
succeeds, and one with an explicitly empty securityGroups list that expects the
MinItems=1 validation error. Keep the fixtures consistent with the existing NLB
examples and assert the corresponding validation message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2e487cd6-07ce-49f2-babc-1f4b79ce396b
📒 Files selected for processing (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSecurityGroupsAWS.yaml
5 participants
Allows administrators to specify custom security groups for
IngressControllers using AWS Network Load Balancers. Introduce
under the
IngressControllerLBSecurityGroupsAWSFeatureGate.Adds a
securityGroupsfield toAWSNetworkLoadBalancerParameterswith a new
SecurityGroupIDtype validated against the AWSsg-*format. When specified, these security groups replace the managed
security group that the Cloud Controller Manager would otherwise
create automatically.
Enhancement: openshift/enhancements#2037
Epic: https://issues.redhat.com/browse/NE-2387