Add terminationMessagePolicy and required-scc annotation for OCP 4.21 conformance#377
Add terminationMessagePolicy and required-scc annotation for OCP 4.21 conformance#377dustman9000 wants to merge 3 commits into
Conversation
WalkthroughAdds OpenShift SCC annotation and container termination policy to pod templates; bumps CI/build images; enables codecov status targets; adds comprehensive pre-commit hooks; removes three OWNERS_ALIASES entries; and refactors/vet-style updates across AWS/VPC endpoint controller code (including a signature change, error-wrapping, nolint directives, and corresponding test update). ChangesDeployments & Images (CI / Runtime)
Pre-commit & Repo Hooks
Coverage & Ownership Metadata
VpcEndpoint Controller Refactor & Tests
Minor formatting / lint-only edits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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 |
openshift-ci
Bot
added
the
approved
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (19.04%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
==========================================
+ Coverage 40.69% 40.73% +0.03%
==========================================
Files 32 32
Lines 2150 2148 -2
==========================================
Hits 875 875
+ Misses 1171 1170 -1
+ Partials 104 103 -1
🚀 New features to boost your workflow:
|
dustman9000
force-pushed
the
termination-message-policy
branch
from
2900386 to
f4f4edd
Compare
|
New changes are detected. LGTM label has been removed. |
|
@dustman9000: The following test 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/vpcendpoint/helpers.go (1)
920-934:⚠️ Potential issue | 🔴 CriticalCritical: Caller dereferences nil record without nil check.
This function now returns
(nil, nil)in success paths—when the endpoint is missing or has no DNS entries during deletion. The only caller incontrollers/vpcendpoint/validation.go:377skips the error check whenerr == niland immediately dereferences the record at line 385:resourceRecord, err := r.generateRoute53Record(ctx, resource) if err != nil { r.log.V(0).Info("Skipping Route53 Record", "error", err.Error()) return nil } input := &route53Types.ResourceRecordSet{ ... ResourceRecords: []route53Types.ResourceRecord{*resourceRecord}, // nil panic here ... }Either guard against nil before dereferencing or have
generateRoute53Recordreturn an error instead of(nil, nil).
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
57-60: 🏗️ Heavy liftValidate the PKO manifest path too.
This hook only checks raw YAML under
deploy/, so edits todeploy_pko/Deployment-aws-vpce-operator.yaml.gotmpl—one of the conformance manifests touched in this PR—still have no local syntax guard. Add a rendered-template validation step fordeploy_pko/, or narrow the surrounding documentation so contributors do not assume both deployment paths are covered.🤖 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 @.pre-commit-config.yaml around lines 57 - 60, The YAML syntax hook (id: check-yaml) only targets deploy/ and misses rendered templates under deploy_pko (e.g. deploy_pko/Deployment-aws-vpce-operator.yaml.gotmpl); update .pre-commit-config.yaml to either add a second hook that validates rendered templates in deploy_pko (or extend the existing check-yaml files regex to include deploy_pko/.*\.(ya?ml|yaml\.gotmpl|gotmpl)$) so changes to deploy_pko are checked locally, and ensure the new pattern covers .gotmpl files or add a separate rendered-template validation hook referencing deploy_pko.
129-133: ⚡ Quick winExtend the RBAC wildcard check to
deploy_pko/.Right now this hook skips
deploy_pko/*.yaml.gotmpl, so wildcard RBAC regressions in the PKO manifest will not be caught before commit.🤖 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 @.pre-commit-config.yaml around lines 129 - 133, The RBAC hook with id "rbac-wildcard-check" currently only matches ^deploy/.*\.ya?ml$ and thus skips deploy_pko/*.yaml.gotmpl; update its files regex to include both deploy and deploy_pko directories and the .yaml.gotmpl variant (for example change the files pattern on the rbac-wildcard-check entry to a regex like ^(?:deploy|deploy_pko)/.*\.ya?ml(?:\.gotmpl)?$ so it catches .yaml, .yml and .yaml.gotmpl/.yml.gotmpl files).
🤖 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 @.pre-commit-config.yaml:
- Around line 57-60: The YAML syntax hook (id: check-yaml) only targets deploy/
and misses rendered templates under deploy_pko (e.g.
deploy_pko/Deployment-aws-vpce-operator.yaml.gotmpl); update
.pre-commit-config.yaml to either add a second hook that validates rendered
templates in deploy_pko (or extend the existing check-yaml files regex to
include deploy_pko/.*\.(ya?ml|yaml\.gotmpl|gotmpl)$) so changes to deploy_pko
are checked locally, and ensure the new pattern covers .gotmpl files or add a
separate rendered-template validation hook referencing deploy_pko.
- Around line 129-133: The RBAC hook with id "rbac-wildcard-check" currently
only matches ^deploy/.*\.ya?ml$ and thus skips deploy_pko/*.yaml.gotmpl; update
its files regex to include both deploy and deploy_pko directories and the
.yaml.gotmpl variant (for example change the files pattern on the
rbac-wildcard-check entry to a regex like
^(?:deploy|deploy_pko)/.*\.ya?ml(?:\.gotmpl)?$ so it catches .yaml, .yml and
.yaml.gotmpl/.yml.gotmpl files).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57fc4bf1-0a83-4d9a-976c-f6452ccd988f
📒 Files selected for processing (16)
.pre-commit-config.yamlapi/v1alpha1/groupversion_info.gocontrollers/vpcendpoint/cleanup.gocontrollers/vpcendpoint/helpers.gocontrollers/vpcendpoint/helpers_test.gocontrollers/vpcendpoint/validation.gocontrollers/vpcendpointtemplate/vpcendpointtemplate_controller.gofips.gomain.gopkg/aws_client/route53_hosted_zone.gopkg/aws_client/security_group.gopkg/aws_client/subnet.gopkg/aws_client/tags.gopkg/aws_client/vpc_endpoint.gopkg/dnses/dnses_test.gopkg/secrets/secrets.go
✅ Files skipped from review due to trivial changes (2)
- pkg/aws_client/subnet.go
- pkg/aws_client/tags.go
3 participants
Summary
terminationMessagePolicy: FallbackToLogsOnErrorto the operator container spec in both OLM (deploy/) and PKO (deploy_pko/) deployment manifestsopenshift.io/required-scc: restricted-v2annotation to pod template metadata in both deployment manifestsThese are required for OCP 4.21 conformance. The
terminationMessagePolicyensures container termination messages capture log output on error, and therequired-sccannotation explicitly declares the security context constraint the pod needs.Test plan
terminationMessagePolicyappears in pod spec viaoc get pod -o yamlrequired-sccannotation appears in pod metadataSummary by CodeRabbit
Chores
Refactor
Tests