OCPBUGS-57437: Always validate EgressIPs - #309
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references Jira Issue OCPBUGS-57437, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
|
[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 |
openshift-ci-robot
added
jira/valid-bug
|
@mdbooth: This pull request references Jira Issue OCPBUGS-57437, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
WalkthroughThis PR registers the OVN EgressIP CRD, adds egress-aware CSR authorization and serving-certificate validation, updates retry handling for validation errors, and refreshes tests and indirect dependencies. ChangesOVN EgressIP CSR authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: sunzhaohua2. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
@mdbooth: This pull request references Jira Issue OCPBUGS-57437, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: sunzhaohua2. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@go.mod`:
- Line 48: The dependency on go.opentelemetry.io/otel/sdk is still pinned to
v1.39.0, which needs to be raised to a safe version. Update the OpenTelemetry
module set in go.mod to v1.40.0 or later, and make sure any related go.sum
entries are refreshed so the dependency graph stays consistent.
In `@pkg/controller/csr_check_test.go`:
- Line 82: The init path in csr_check_test.go is ignoring the error from
egressipv1.AddToScheme(scheme.Scheme), which is inconsistent with the existing
panic-on-failure pattern in init(). Update the init() setup around AddToScheme
to handle its returned error explicitly, using a panic or equivalent hard-fail
consistent with generateCertKeyPair, so golangci-lint errcheck is satisfied. Use
the AddToScheme call and init() as the key symbols to locate the change.
In `@pkg/controller/csr_check.go`:
- Around line 94-164: getNodeEgressIPs and getKubeletCA are still making API
requests with context.Background(), so they can hang without a deadline. Replace
the internal background context usage in getNodeEgressIPs (for the network,
EgressIP list, and HostSubnet reads) and in getKubeletCA with a bounded context
created locally with a timeout, and ensure it is canceled after each call. Keep
the fix localized to these two functions and preserve their existing error
handling.
- Around line 381-400: In authorizeServingRenewal, stop wrapping every
verifyCertificateCommonName failure as CSRValidationError so that cert-chain and
expiry verification failures from currentCert.Verify(options) can propagate back
to reconcileCSR as retryable errors. Keep the SAN/CN comparison checks in
authorizeServingRenewal as validation errors, but only convert the specific
mismatch cases to newCSRValidationError and let non-validation errors from
verifyCertificateCommonName pass through unchanged.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be9227c1-af7b-4382-934e-d0a7667dc34b
⛔ Files ignored due to path filters (249)
go.sumis excluded by!**/*.sumvendor/cel.dev/expr/BUILD.bazelis excluded by!**/vendor/**,!vendor/**vendor/cel.dev/expr/MODULE.bazelis excluded by!**/vendor/**,!vendor/**vendor/cel.dev/expr/checked.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/cel.dev/expr/eval.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/cel.dev/expr/explain.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/cel.dev/expr/syntax.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/cel.dev/expr/value.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/pointer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yaml.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/bytestostr.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/lexer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jwriter/writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/spf13/afero/.editorconfigis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/copyOnWriteFs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/iofs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/lstater.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/mem/file.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/unionFile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/util.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/id.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/number.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/status.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/traces.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/value.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/auto/sdk/tracer.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/.clomonitor.ymlis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/.codespellignoreis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/.lycheeignoreis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CODEOWNERSis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/Makefileis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/README.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/RELEASING.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/SECURITY-INSIGHTS.ymlis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/VERSIONING.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/encoder.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/hash.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/xxhash/xxhash.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/iterator.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/key.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/kv.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/set.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/type_string.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/baggage/baggage.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/codes/codes.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/dependencies.Dockerfileis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/errorhandler/errorhandler.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/handler.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/instruments.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/internal_logging.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/meter.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/state.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/internal/global/trace.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/asyncfloat64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/asyncint64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/config.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/meter.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/noop/noop.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/syncfloat64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/syncint64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/propagation/baggage.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/propagation/propagation.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/propagation/trace_context.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/internal/x/features.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/internal/x/x.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/builtin.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/container.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/env.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_bsd.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_linux.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_windows.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_release_unix.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_unix.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/process.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/resource.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/batch_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/id_generator.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/env/env.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/batch_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/simple_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/tracer.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/provider.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/sampling.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/simple_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/snapshot.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/span_limits.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/tracer.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/version.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/version.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/MIGRATION.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/README.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/error_type.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/exception.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/otelconv/metric.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/schema.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/MIGRATION.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/README.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/attribute_group.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/error_type.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/exception.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/schema.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/config.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/hex.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/attr.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/id.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/noop/noop.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/trace.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/tracestate.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/version.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/versions.yamlis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/deviceauth.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/oauth2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/pkce.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/token.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/transport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/time/rate/rate.gois excluded by!**/vendor/**,!vendor/**vendor/gomodules.xyz/jsonpatch/v2/jsonpatch.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/checked.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/eval.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/explain.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/syntax.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/value.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/api/httpbody/httpbody.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/MAINTAINERS.mdis excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/README.mdis excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/balancer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/endpointsharding/endpointsharding.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/pickfirst/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/pickfirst/pickfirst.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/pickfirst/pickfirstleaf/pickfirstleaf.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/roundrobin/roundrobin.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer/subconn.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/balancer_wrapper.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/binarylog/grpc_binarylog_v1/binarylog.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/clientconn.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/credentials/credentials.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/credentials/insecure/insecure.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/credentials/tls.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/dialoptions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/encoding/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/encoding/gzip/gzip.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/encoding/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/encoding/proto/proto.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/experimental/stats/metricregistry.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/experimental/stats/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/health/grpc_health_v1/health.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/health/grpc_health_v1/health_grpc.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/interceptor.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/balancer/gracefulswitch/gracefulswitch.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/balancer/weight/weight.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/buffer/unbounded.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/channelz/trace.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/credentials/credentials.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/envconfig/envconfig.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/envconfig/xds.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/experimental.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/grpcsync/callback_serializer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/grpcsync/event.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/idle/idle.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/resolver/delegatingresolver/delegatingresolver.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/resolver/dns/dns_resolver.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/stats/metrics_recorder_list.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/stats/stats.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/status/status.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/client_stream.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/controlbuf.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/flowcontrol.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/handler_server.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/http2_client.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/http2_server.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/http_util.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/server_stream.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/internal/transport/transport.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/mem/buffer_pool.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/mem/buffer_slice.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/mem/buffers.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/picker_wrapper.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/preloader.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/resolver/resolver.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/resolver_wrapper.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/rpc_util.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/server.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/stats/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/stats/stats.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/stream.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/grpc/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/editionssupport/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/proto.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/ref.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modmain.gopkg/controller/controller.gopkg/controller/csr_check.gopkg/controller/csr_check_test.go
| // getNodeEgressIPs returns the egress IPs assigned to the given node. | ||
| // It determines the network type and queries the appropriate CRD: | ||
| // - OVNKubernetes: queries EgressIP objects from k8s.ovn.org/v1 | ||
| // - OpenShiftSDN: queries HostSubnet objects |
There was a problem hiding this comment.
SDN hasn't been supported since ~4.17 IIRC. We should verify this and remove any SDN references now
There was a problem hiding this comment.
If you go back to the Jira that was my original plan. But @sdodson had asked for a 4.16 backport where it is required, and the code was already there so I left it in. I'm inclined to follow up with another patch removing SDN support back to 4.17.
There was a problem hiding this comment.
That comment is a year old, do we still intend to backport this all the way to 4.16? I would expect us to only backport this as far as 4.18 which is the oldest, currently in maintenance release.
Sustaining may wish to backport to 4.16 to support the EUS term 2 customers that are using 4.16 there, perhaps we should get their impression on whether this woud be something they want to backport?
| // No error, the renewal is authorized. | ||
| return true, nil | ||
| // Use machine API authorization if the node has a machine. | ||
| machine, machineErr := machinehandlerpkg.FindMatchingMachineFromNodeRef(machines, nodeAsking) |
There was a problem hiding this comment.
Why are we reversing this order of machine vs renewing against existing SANs? Seems like an unrelated change we don't necessarily need to include here?
There was a problem hiding this comment.
We're not reversing it. We're eliminating it. There is no fallback any more. Either we have a Machine, in which case we do the Machine validation flow, or we don't, in which case we do the existing SAN validation flow.
This is relevant to EgressIPs because the IPI flow can reject a CSR with an EgressIP which was formerly present but is no longer present, but the existing SAN validation can't. If we allow a fallback back we remove that ability.
It's also interesting to note that if you look carefully at the tests, we haven't altered any existing test cases with this change, except by removing unrelated information from their error messages.
There was a problem hiding this comment.
We discussed this. @JoelSpeed additionally raised the question of whether this could impact platforms which use keepalived to manage a VIP, e.g. metal, vSphere, OpenStack.
To verify this, I checked the state of all e2e jobs running against the installer. I noted that, as expected, all UPI jobs implement an approve_csrs method to 'manually' approve CSRs. In contrast, only 1 IPI job currently does this: openshift-e2e-vsphere-hybrid-env. This job adds 2 BM nodes to a vSphere cluster. As the additional nodes will not be auto-approved, it approves the additional nodes.
We know that SAN-reapproval is not relevant to joining nodes, so this means that all IPI platforms under test are approved by CMA using the Machine-backed flow.
As for keepalived specifically, I checked what actually happens here. I created a vsphere cluster and found the control plane node which currently owned the external api VIP. I confirmed that the API VIP was added to br-ex as an additional IP address. I deleted kubelet's serving cert and restarted it, which caused it to request a new one. The new CSR:
- Did not include the api server VIP
- Was automatically approved
The reason was that the CCM had not added the VIP to the Node. The reason for this is that the CCM is only aware of IPs which have been added via infrastructure API calls. Keepalived is moving the VIP through L2 network shenanigans, and therefore isn't visible to the CCM.
By contrast, EgressIPs are added via infrastructure API calls, and are visible to the CCM. Therefore they must either be picked up by (ideally) the {Machine,Cluster}API provider, or CMA must be explicitly aware of them.
| github.com/openshift/client-go v0.0.0-20260317180604-743f664b82d1 | ||
| github.com/openshift/controller-runtime-common v0.0.0-20260428152732-64ee174f5e2e | ||
| github.com/openshift/library-go v0.0.0-20260318142011-72bf34f474bc | ||
| github.com/ovn-kubernetes/ovn-kubernetes/go-controller v0.0.0-20260703130629-a6c3ed490571 |
There was a problem hiding this comment.
This is an unfortunate dependency, is it necessary? Is it worth looking at unstructured instead?
There was a problem hiding this comment.
Hah, that's exactly what the bot said. I said no. I think the ovn-kubernetes api package was fairly well isolated, no? IIRC this doesn't pull in anything other than apimachinery, which we've already got.
There was a problem hiding this comment.
I think Unstructured was viable here, I just prefer the relative safety of structured types whenever possible. I can look at this again, but IIRC it was just apimachinery, which I felt was safe enough.
There was a problem hiding this comment.
We discussed this. The resolution: on my recognizance 😅 I'm going to double check my assertion that the api-only import from ovn-kubernetes is as clean as I'm claiming, but if so then I'm ok with it.
There was a problem hiding this comment.
Yep, this API is extremely clean: https://github.com/ovn-kubernetes/ovn-kubernetes/tree/master/go-controller/pkg/crd/egressip/v1
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/controller/csr_check.go (1)
435-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale requeue comment.
This now returns
CSRValidationError, which downstream reconciliation handles without retry, so the comment says the opposite of the current behavior.🧹 Proposed comment fix
- // return error so we requeue, in case machine network is out of date - // for some reason + // Return a validation error so invalid CSRs are not retried automatically.Based on learnings, keep documentation aligned with actual behavior.
🤖 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 `@pkg/controller/csr_check.go` around lines 435 - 439, The requeue comment in the CSR validation path is stale and contradicts the current behavior in the CSR check logic. Update the comment near the code that logs the DNS name mismatch and returns newCSRValidationError so it accurately states that this path returns CSRValidationError and is handled by downstream reconciliation without retry, rather than implying it will requeue; keep the wording aligned with the behavior of csr_check.go and the validation flow.Source: Learnings
🤖 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 `@pkg/controller/csr_check.go`:
- Line 603: The deferred conn.Close() in the CSR check flow is ignoring a
returned error, which violates the never-ignore-errors rule and the errcheck
hint. Update the cleanup in the function that opens conn so the close error is
handled explicitly, either by logging it or by propagating it through the
surrounding error path, while keeping the connection lifecycle in this same
function.
- Line 596: The log in csr_check.go currently exposes the kubelet endpoint via
the klog.Infof call in the serving-cert retrieval path. Update the logging
around the retrieval flow (near the existing nodeName/kubelet message) to omit
the kubelet value and keep only non-sensitive context such as the node name.
Make sure any related log statements in this code path follow the same rule and
do not print internal hostnames, addresses, or ports.
- Around line 310-315: Propagate the serving-cert lookup failure from the CSR
check path instead of swallowing it: in the getServingCert call site inside
csr_check logic, and any related reconcileCSR handling, return the actual error
when Node GET or internal-IP lookup fails so the reconciliation loop retries.
Keep only explicit validation/approval failures as non-retryable, and preserve
the existing logging around the getServingCert error path.
---
Nitpick comments:
In `@pkg/controller/csr_check.go`:
- Around line 435-439: The requeue comment in the CSR validation path is stale
and contradicts the current behavior in the CSR check logic. Update the comment
near the code that logs the DNS name mismatch and returns newCSRValidationError
so it accurately states that this path returns CSRValidationError and is handled
by downstream reconciliation without retry, rather than implying it will
requeue; keep the wording aligned with the behavior of csr_check.go and the
validation flow.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed680079-9f81-4e5a-9061-3a0a3a1e5554
⛔ Files ignored due to path filters (154)
go.sumis excluded by!**/*.sumvendor/go.opentelemetry.io/otel/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/AGENTS.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CLAUDE.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/Makefileis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/README.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/RELEASING.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/encoder.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/hash.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/key.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/kv.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/set.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/type_string.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/baggage/baggage.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/dependencies.Dockerfileis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/asyncfloat64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/asyncint64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/config.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/instrument.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/syncfloat64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/metric/syncint64.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/propagation/baggage.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/propagation/trace_context.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/requirements.txtis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/internal/x/features.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/builtin.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/config.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/container.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/env.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_exec.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id_readfile.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_unix.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_windows.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/process.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/resource.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/batch_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/batch_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/simple_span_processor.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/tracer.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/provider.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/sampling.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/span_limits.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/sdk/version.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.26.0/attribute_group.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/MIGRATION.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/README.mdis excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/attribute_group.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/error_type.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/exception.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/otelconv/metric.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.41.0/schema.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/config.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/trace.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/trace/tracestate.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/version.gois excluded by!**/vendor/**,!vendor/**vendor/go.opentelemetry.io/otel/versions.yamlis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/plan9/syscall_plan9.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ioctl_signed.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ioctl_unsigned.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/mkall.shis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/readv_unix.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_darwin.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux_arm.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux_arm64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux_loong64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux_riscv64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_openbsd.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_unix.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_386.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_amd64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_loong64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mipsle.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_riscv64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_s390x.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_sparc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_386.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_386.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_amd64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_amd64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_arm.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_arm.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_arm64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_arm64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_mips64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_mips64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_ppc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_ppc64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_riscv64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsyscall_openbsd_riscv64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_386.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_amd64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_loong64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mipsle.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_riscv64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_s390x.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_sparc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_386.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_amd64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_loong64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mipsle.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_riscv64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_s390x.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_sparc64.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/aliases.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/dll_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/security_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modmain.gopkg/controller/controller.gopkg/controller/csr_check.gopkg/controller/csr_check_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- main.go
- pkg/controller/controller.go
- go.mod
- pkg/controller/csr_check_test.go
|
/retest-required |
|
@mdbooth: 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. |
| egressIPList := &egressipv1.EgressIPList{} | ||
| if err := c.List(ctx, egressIPList); err != nil { | ||
| return result, fmt.Errorf("could not list EgressIP objects: %v", err) | ||
| } |
There was a problem hiding this comment.
Do you have RBAC for egressIP?
| klog.Errorf("%v: IP address '%s' not in machine addresses: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | ||
| return fmt.Errorf("IP address '%s' not in machine addresses: %s", san, strings.Join(attemptedAddresses, " ")) | ||
| klog.Errorf("%v: IP address '%s' not in machine addresses or egress IPs: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | ||
| return newCSRValidationError("IP address '%s' not in machine addresses or egress IPs: %s", san, strings.Join(attemptedAddresses, " ")) |
There was a problem hiding this comment.
Are the IPs and DNS already available when the CSR is first reconciled? CSRValidationError is being handled as permanent failure. Previously this returned error and requeued.
There was a problem hiding this comment.
This is ok because:
- For the initial serving cert there are no egress IPs and we know Machine addresses are up to date
- For rotations, kubelet will retry after 15 minutes by creating a new CSR
EgressIPs manifest as additional IP addresses on a Node. They are dynamic, so can be added and removed. Even though they cannot be used to serve incoming traffic, CCMs cannot currently distinguish EgressIPs and they will typically be added to a Node's Addresses. Consequently kubelet will typically request a CSR containing EgressIPs when they are present on a Node. We had some existing code to make the approver aware of OpenShiftSDN egress IPs, but no equivalent for OVN Kubernetes. This change fixes 2 aspects of validating a Node with EgressIPs: * Extend support for EgressIP matching to OVN Kubernetes * Ensure that a Node cannot continue to request an EgressIP which it no longer has For the first, we also fetch OVNKubernetes EgressIP based on the infrastructure's networkType. For the second, we can only do this for IPI clusters as we don't know the complete set of permitted address in a UPI cluster. To enable this for IPI clusters, we deterministically split the authorisation path chosen based on the presence of a Machine object for a Node. We no longer permit fall-through authentication. A side benefit of this is that the error messages reported on authentication failures become shorter. However, note from the test changes that this doesn't result in any additional failure modes.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@mdbooth: An error was encountered querying GitHub for users with public email (zhsun@redhat.com) for bug OCPBUGS-57437 on the Jira server at https://redhat.atlassian.net. No known errors were detected, please see the full error message for details. Full error message.
Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: connect: connection refused
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/controller/csr_check.go (1)
656-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
makeIPSetinsubsetIPAddresses.
subsetIPAddressesbuilds the same string set thatmakeIPSetbuilds. Both are added in this change. Call the helper to remove the duplicate loop.♻️ Proposed refactor
func subsetIPAddresses(super, sub []net.IP) bool { - superSet := make(map[string]struct{}) - for _, ipAddr := range super { - superSet[ipAddr.String()] = struct{}{} - } - + superSet := makeIPSet(super) for _, ipAddr := range sub { if !ipInSet(superSet, ipAddr) { return false } } return true }🤖 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 `@pkg/controller/csr_check.go` around lines 656 - 677, Update subsetIPAddresses to initialize superSet by calling makeIPSet(super) instead of duplicating the loop that converts IP addresses to strings, while preserving the existing sub-address validation through ipInSet.pkg/controller/csr_check_test.go (2)
1652-1726: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a UPI case for a removed egress IP.
The IPI cases cover removal at lines 1598-1621. The UPI cases cover only "assigned to node" and "assigned elsewhere". The PR states that the UPI path cannot enforce removal, because the current certificate IPs remain in the allowed set. A test that asserts this documented asymmetry prevents a later refactor from silently changing it.
Add a case with
node: withName("test", defaultNode()),csr: goodCSR,networkType: "OVNKubernetes", noegressIPObjs, andca: []*x509.Certificate{parseCert(t, rootCertGood)}. The CSR IPs are still present in the current serving certificate, soauthorizeistrue.🤖 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 `@pkg/controller/csr_check_test.go` around lines 1652 - 1726, Add a UPI test case alongside the existing OVNKubernetes cases using node withName("test", defaultNode()), goodCSR, no egressIPObjs, and the rootCertGood CA. Assert authorization succeeds (authorize: true) with no error, documenting that removed egress IPs remain allowed through the current serving certificate.
2433-2472: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for the IPv4-mapped IPv6 form.
subsetIPAddresses,makeIPSet, andipInSetall key onnet.IP.String(). That normalizes a 16-byte::ffff:10.0.0.1and a 4-byte10.0.0.1to the same key. The feature depends on this, becausenet.ParseIPon an EgressIP string can return a 16-byte value whilex509stores IPv4 SANs as 4 bytes. No case covers this.💚 Proposed test case
{ name: "sub is a subset with duplicates", super: []net.IP{tenDotOne, tenDotTwo}, sub: []net.IP{tenDotOne, tenDotOne}, expected: true, }, + { + name: "IPv4-mapped IPv6 matches dotted-quad", + super: []net.IP{net.ParseIP("10.0.0.1")}, + sub: []net.IP{net.IPv4(10, 0, 0, 1).To4()}, + expected: true, + },🤖 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 `@pkg/controller/csr_check_test.go` around lines 2433 - 2472, Add a table-driven test case to TestSubsetIPAddresses covering an IPv4-mapped IPv6 address in one input and the equivalent 4-byte IPv4 address in the other, asserting subsetIPAddresses returns true. Use the existing tenDotOne value and an explicitly constructed ::ffff:10.0.0.1 representation so the test verifies normalization through makeIPSet and ipInSet.
🤖 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 `@pkg/controller/csr_check_test.go`:
- Line 1952: Update the error assertions in the getServingCert test cases so an
empty tt.wantErr explicitly requires err to be nil before any substring check;
retain strings.Contains for non-empty expected-error prefixes and keep the
existing error-message validation for those cases.
In `@pkg/controller/csr_check.go`:
- Around line 285-290: Update the getServingCert error handling in reconcileCSR
so kubelet TLS verification failures return a CSRValidationError, making
first-boot UPI serving CSRs non-retryable, while preserving plain errors for
infrastructure failures such as Node lookup or missing internal IP. Reuse the
existing CSRValidationError classification and queue behavior symbols rather
than changing unrelated retry logic.
- Around line 436-444: Update the denial log and newCSRValidationError message
in the CSR validation path to include the node’s egress IPs alongside
attemptedAddresses, while preserving the existing machine-address details and
wording. Use the egressIPs value already checked by ipInSet, and update affected
expected strings in the related CSR validation tests.
---
Nitpick comments:
In `@pkg/controller/csr_check_test.go`:
- Around line 1652-1726: Add a UPI test case alongside the existing
OVNKubernetes cases using node withName("test", defaultNode()), goodCSR, no
egressIPObjs, and the rootCertGood CA. Assert authorization succeeds (authorize:
true) with no error, documenting that removed egress IPs remain allowed through
the current serving certificate.
- Around line 2433-2472: Add a table-driven test case to TestSubsetIPAddresses
covering an IPv4-mapped IPv6 address in one input and the equivalent 4-byte IPv4
address in the other, asserting subsetIPAddresses returns true. Use the existing
tenDotOne value and an explicitly constructed ::ffff:10.0.0.1 representation so
the test verifies normalization through makeIPSet and ipInSet.
In `@pkg/controller/csr_check.go`:
- Around line 656-677: Update subsetIPAddresses to initialize superSet by
calling makeIPSet(super) instead of duplicating the loop that converts IP
addresses to strings, while preserving the existing sub-address validation
through ipInSet.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: abcb9044-baed-43cc-8906-a1974352630a
⛔ Files ignored due to path filters (28)
go.sumis excluded by!**/*.sumvendor/github.com/go-openapi/jsonpointer/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/pointer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/crd/egressip/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/spf13/afero/.editorconfigis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/copyOnWriteFs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/iofs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/lstater.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/mem/file.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/unionFile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/afero/util.gois excluded by!**/vendor/**,!vendor/**vendor/gomodules.xyz/jsonpatch/v2/jsonpatch.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/rpc/errdetails/error_details.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/fieldpath/element.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/fieldpath/pathelementmap.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/fieldpath/set.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/value/allocator.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/value/jsontagutil.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
go.modmain.gomanifests/01-rbac.yamlpkg/controller/controller.gopkg/controller/csr_check.gopkg/controller/csr_check_test.gostatus_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/controller.go
| node: wrongAddr, | ||
| rootCerts: []*x509.Certificate{parseCert(t, rootCertGood)}, | ||
| wantErr: "dial tcp 127.0.0.1:25544: connect: connection refused", | ||
| wantErr: "dial tcp 127.0.0.1:25544:", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
strings.Contains makes the success cases unable to fail.
tt.wantErr is the empty string for the "all good" case. strings.Contains(anything, "") returns true, so line 1990 passes for any error. Line 1993 then guards on err == nil, so it also skips. A regression that makes getServingCert return an error for a valid node would pass this test.
Keep strings.Contains for the prefix cases, but assert err == nil explicitly when tt.wantErr is empty.
💚 Proposed fix
go respond(server)
serverCert, err := getServingCert(t.Context(), cl, tt.nodeName, certPool)
- if !strings.Contains(errString(err), tt.wantErr) {
+ if tt.wantErr == "" {
+ if err != nil {
+ t.Fatalf("got: %v, want: no error", err)
+ }
+ } else if !strings.Contains(errString(err), tt.wantErr) {
t.Fatalf("got: %v, want: %s", err, tt.wantErr)
}
if err == nil && !serverCert.Equal(parseCert(t, serverCertGood)) {
t.Fatal("Expected server certificate match on success")
}Also applies to: 1989-1995
🤖 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 `@pkg/controller/csr_check_test.go` at line 1952, Update the error assertions
in the getServingCert test cases so an empty tt.wantErr explicitly requires err
to be nil before any substring check; retain strings.Contains for non-empty
expected-error prefixes and keep the existing error-message validation for those
cases.
| servingCert, err := getServingCert(ctx, c, nodeAsking, ca) | ||
| if err != nil { | ||
| klog.Infof("Could not determine if egress enabled: %v", err) | ||
| return false, fmt.Errorf("could not determine if egress enabled: %v", err) | ||
| } | ||
|
|
||
| if servingCert != nil && egressEnabled { | ||
| klog.Infof("Falling back to serving cert renewal with Egress IP checks") | ||
| if err := authorizeServingRenewalWithEgressIPs(c, nodeAsking, csr, servingCert, x509VerificationOpts); err != nil { | ||
| approvalErrors = append(approvalErrors, err) | ||
| klog.Infof("Could not use current serving cert and egress IPs for renewal: %v", err) | ||
| } else { | ||
| // No error means the machine was able to authorize the cert | ||
| return true, nil | ||
| } | ||
| // Could not reach kubelet or verify cert. Not auto-approvable on | ||
| // this attempt; kubelet will resubmit the CSR. | ||
| return false, fmt.Errorf("failed to retrieve current serving cert: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getServingCert failures are now retryable for every UPI serving CSR.
getServingCert returns an error for two very different conditions:
- Infrastructure failures, for example the Node GET fails or the node has no internal IP.
- Kubelet TLS failures, for example the kubelet presents a self-signed or otherwise untrusted certificate.
Both are returned as plain errors here, so reconcileCSR classifies both as retryable and requeues forever with backoff. Condition 2 is expected on a UPI node that has no signed serving certificate yet. Those CSRs are never approvable through this path, so the item stays in the workqueue permanently.
Confirm that this is the intended queue behavior for UPI first-boot serving CSRs. If it is not, return a CSRValidationError for TLS verification failures and keep the plain error for the Node lookup failures.
🤖 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 `@pkg/controller/csr_check.go` around lines 285 - 290, Update the
getServingCert error handling in reconcileCSR so kubelet TLS verification
failures return a CSRValidationError, making first-boot UPI serving CSRs
non-retryable, while preserving plain errors for infrastructure failures such as
Node lookup or missing internal IP. Reuse the existing CSRValidationError
classification and queue behavior symbols rather than changing unrelated retry
logic.
| // The CSR requested an IP not in machine addresses. | ||
| // Check if it's an egress IP assigned to this node. | ||
| if ipInSet(makeIPSet(egressIPs), san) { | ||
| continue | ||
| } | ||
|
|
||
| //TODO: set annotation/emit event here. | ||
| // return error so we requeue, in case machine network is out of date | ||
| // for some reason | ||
| klog.Errorf("%v: IP address '%s' not in machine addresses: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | ||
| return fmt.Errorf("IP address '%s' not in machine addresses: %s", san, strings.Join(attemptedAddresses, " ")) | ||
| klog.Errorf("%v: IP address '%s' not in machine addresses or egress IPs: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | ||
| return newCSRValidationError("IP address '%s' not in machine addresses or egress IPs: %s", san, strings.Join(attemptedAddresses, " ")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The denial message lists only machine addresses but claims to list egress IPs.
attemptedAddresses collects machine addresses only. The message text is "not in machine addresses or egress IPs: ". A reader who debugs an EgressIP denial cannot tell whether the node had zero assigned egress IPs or a different set. Include the egress IPs in the message.
🛠️ Proposed fix
//TODO: set annotation/emit event here.
- klog.Errorf("%v: IP address '%s' not in machine addresses or egress IPs: %s", req.Name, san, strings.Join(attemptedAddresses, " "))
- return newCSRValidationError("IP address '%s' not in machine addresses or egress IPs: %s", san, strings.Join(attemptedAddresses, " "))
+ egressStrings := make([]string, 0, len(egressIPs))
+ for _, ip := range egressIPs {
+ egressStrings = append(egressStrings, ip.String())
+ }
+ klog.Errorf("%v: IP address '%s' not in machine addresses [%s] or egress IPs [%s]", req.Name, san, strings.Join(attemptedAddresses, " "), strings.Join(egressStrings, " "))
+ return newCSRValidationError("IP address '%s' not in machine addresses [%s] or egress IPs [%s]", san, strings.Join(attemptedAddresses, " "), strings.Join(egressStrings, " "))
}This changes the expected strings in pkg/controller/csr_check_test.go at lines 744, 774, 1595, 1619, and 1704.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The CSR requested an IP not in machine addresses. | |
| // Check if it's an egress IP assigned to this node. | |
| if ipInSet(makeIPSet(egressIPs), san) { | |
| continue | |
| } | |
| //TODO: set annotation/emit event here. | |
| // return error so we requeue, in case machine network is out of date | |
| // for some reason | |
| klog.Errorf("%v: IP address '%s' not in machine addresses: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | |
| return fmt.Errorf("IP address '%s' not in machine addresses: %s", san, strings.Join(attemptedAddresses, " ")) | |
| klog.Errorf("%v: IP address '%s' not in machine addresses or egress IPs: %s", req.Name, san, strings.Join(attemptedAddresses, " ")) | |
| return newCSRValidationError("IP address '%s' not in machine addresses or egress IPs: %s", san, strings.Join(attemptedAddresses, " ")) | |
| // The CSR requested an IP not in machine addresses. | |
| // Check if it's an egress IP assigned to this node. | |
| if ipInSet(makeIPSet(egressIPs), san) { | |
| continue | |
| } | |
| //TODO: set annotation/emit event here. | |
| egressStrings := make([]string, 0, len(egressIPs)) | |
| for _, ip := range egressIPs { | |
| egressStrings = append(egressStrings, ip.String()) | |
| } | |
| klog.Errorf("%v: IP address '%s' not in machine addresses [%s] or egress IPs [%s]", req.Name, san, strings.Join(attemptedAddresses, " "), strings.Join(egressStrings, " ")) | |
| return newCSRValidationError("IP address '%s' not in machine addresses [%s] or egress IPs [%s]", san, strings.Join(attemptedAddresses, " "), strings.Join(egressStrings, " ")) |
🤖 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 `@pkg/controller/csr_check.go` around lines 436 - 444, Update the denial log
and newCSRValidationError message in the CSR validation path to include the
node’s egress IPs alongside attemptedAddresses, while preserving the existing
machine-address details and wording. Use the egressIPs value already checked by
ipInSet, and update affected expected strings in the related CSR validation
tests.
4 participants
EgressIPs manifest as additional IP addresses on a Node. They are dynamic, so can be added and removed. Even though they cannot be used to serve incoming traffic, CCMs cannot currently distinguish EgressIPs and they will typically be added to a Node's Addresses. Consequently kubelet will typically request a CSR containing EgressIPs when they are present on a Node.
We had some existing code to make the approver aware of OpenShiftSDN egress IPs, but no equivalent for OVN Kubernetes.
This change fixes 2 aspects of validating a Node with EgressIPs:
For the first, we also fetch OVNKubernetes EgressIP based on the infrastructure's networkType.
For the second, we can only do this for IPI clusters as we don't know the complete set of permitted address in a UPI cluster. To enable this for IPI clusters, we deterministically split the authorisation path chosen based on the presence of a Machine object for a Node. We no longer permit fall-through authentication. A side benefit of this is that the error messages reported on authentication failures become shorter. However, note from the test changes that this doesn't result in any additional failure modes.
Summary by CodeRabbit