OCPCLOUD-3346: tls: use centralized TLS#1456
Open
RadekManak wants to merge 13 commits intoopenshift:mainfrom
Open
OCPCLOUD-3346: tls: use centralized TLS#1456RadekManak wants to merge 13 commits intoopenshift:mainfrom
RadekManak wants to merge 13 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@RadekManak: This pull request references OCPCLOUD-3346 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 "4.22.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. |
Contributor
Author
|
/assign @damdo |
damdo
reviewed
Jan 26, 2026
Member
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks! Looks good, a couple of minor points.
pkg/operator/operator.go
Outdated
| var tlsProfile *osconfigv1.TLSProfileSpec | ||
| apiServer, err := optr.osClient.ConfigV1().APIServers().Get(context.Background(), "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| klog.Warningf("Failed to fetch APIServer, using default TLS profile: %v", err) |
Member
There was a problem hiding this comment.
Shouldn't we return here?
What would happen otherwise?
Contributor
|
[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 |
RadekManak
force-pushed
the
feature-centralized-tsl-endpoints
branch
from
4e665c0 to
24eed11
Compare
Contributor
|
@RadekManak: This pull request references OCPCLOUD-3346 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 "4.22.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. |
RadekManak
force-pushed
the
feature-centralized-tsl-endpoints
branch
from
24eed11 to
6b35a05
Compare
damdo
reviewed
Feb 23, 2026
Member
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks for this Radek!
I left a bunch of comments but it mostly looks good! TY
Member
|
/retest |
Remove the kube-rbac-proxy sidecar, mount the serving cert, and restart the operator on APIServer TLS profile changes.
Capture the APIServer TLS profile in operator config and use it to configure kube-rbac-proxy TLS args, with unit coverage.
Add unit tests to verify TLS configuration handling in newKubeProxyContainer, including tests for TLS 1.2 with cipher suites and TLS 1.3 without cipher suites.
Move TLS configuration computation from per-container to once per batch in newKubeProxyContainers. This avoids redundant processing when creating multiple kube-rbac-proxy containers with the same TLS profile.
Introduce command-line flags for TLS cipher suites and minimum version in the machineset controller. Update the container creation logic to utilize these TLS settings
This ensures Metal3 remediation webhooks use the configured TLS profile while keeping other controllers unchanged.
RadekManak
force-pushed
the
feature-centralized-tsl-endpoints
branch
from
9b911a2 to
fe1ce87
Compare
WalkthroughReplaces in-process HTTP metrics with a TLS-secured metrics server, adds APIServer TLSProfile retrieval and watcher to trigger reloads, removes kube-rbac-proxy from the deployment, wires TLS options through operator/webhook/container args, updates RBAC for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Contributor
|
@RadekManak: This pull request references OCPCLOUD-3346 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 "4.22.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. |
Contributor
Author
|
/hold This will crash baremetal until the PR merges. |
openshift-ci
bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/machine-api-operator/start.go`:
- Around line 247-251: The current goroutine fatals on any metricsServer.Start
error; change it to only treat context.Canceled as a non-fatal shutdown and exit
gracefully, while still fataling for other unexpected errors. Locate the
anonymous goroutine that calls metricsServer.Start(metricsServerCtx) and update
its error handling to check for errors.Is(err, context.Canceled) (allowing a
clean shutdown/log and not calling klog.Fatalf) and call klog.Fatalf only for
other error values; do not special-case http.ErrServerClosed because
metricsServer.Start already filters that.
In `@cmd/machineset/main.go`:
- Line 177: The call to utiltls.NewTLSConfigFromProfile is ignoring its error
(tlsOpts, _ := utiltls.NewTLSConfigFromProfile(tlsProfile)); change this to
capture and handle the error (e.g., tlsOpts, err :=
utiltls.NewTLSConfigFromProfile(tlsProfile)) and if err != nil log the error via
the existing logger or fmt and exit/return a non-zero status so the webhook
server does not start with invalid TLS settings; update the initialization path
in main (where tlsProfile and tlsOpts are used) to propagate or handle the error
appropriately.
In `@pkg/operator/sync.go`:
- Around line 865-875: In getTLSArgs, capture the second return value from
utiltls.NewTLSConfigFromProfile (the unsupportedCiphers slice) instead of
discarding it, and emit a log containing those unsupported cipher names so
profile-to-flag translation mismatches are visible; specifically, change the
call to utiltls.NewTLSConfigFromProfile to receive (tlsConfigFn,
unsupportedCiphers), and then log unsupportedCiphers (mirroring the pattern used
in cmd/machine-api-operator/start.go) before continuing to build the tlsArgs.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (287)
go.sumis excluded by!**/*.sumvendor/github.com/BurntSushi/toml/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/decode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/encode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/lex.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/meta.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/BurntSushi/toml/parse.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/dbus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/methods.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/subscription.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/subscription_set.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/journal/journal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-systemd/v22/journal/journal_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/COPYING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/LICENSE.BSDis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/LICENSE.MPL-2.0is excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/VERSIONis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/codecov.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/deprecated_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/gocompat_generics_go121.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/gocompat_generics_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/internal/consts/consts.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/openat2_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/openat_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/assert/assert.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/errors_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/at_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/fd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/fd_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/mount_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/openat2_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_go119.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_errors_go120.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_errors_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_generics_go121.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_generics_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/kernelversion/kernel_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/mount_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/openat2_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/procfs/procfs_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/procfs/procfs_lookup_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/lookup_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/mkdir_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/open_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/openat2_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/procfs/procfs_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/procfs_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cyphar/filepath-securejoin/vfs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/pprof/profile/merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/pprof/profile/profile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/pprof/profile/proto.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/pprof/profile/prune.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/gmeasure/experiment.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/match_error_strictly_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/runtime-spec/specs-go/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/runtime-spec/specs-go/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/label/label.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/label/label_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/label/label_stub.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/selinux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/selinux_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/selinux/go-selinux/selinux_stub.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clusterimagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gathererconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicyfulciocawithrekorrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicypkirootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicypublickeyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagesigstoreverificationpolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagatherspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeclaimreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/policyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/storage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyfulciocawithrekorrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypkirootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypublickeyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagesigstoreverificationpolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/interface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/expansion_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ssh/keys.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ssh/messages.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ssh/ssh_gss.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ssh/streamlocal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ssh/tcpip.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/modfile/print.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/modfile/read.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/modfile/rule.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/module/module.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/semver/semver.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/context/context.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/trace/events.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/websocket/hybi.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/cpu/cpu_arm64.sis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/cpu/cpu_x86.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_linux.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/ztypes_linux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ztypes_netbsd_arm.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/golang.org/x/term/terminal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/japanese/eucjp.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/japanese/iso2022jp.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/japanese/shiftjis.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/korean/euckr.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/gbk.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/hzgb2312.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/traditionalchinese/big5.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/text/encoding/unicode/unicode.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/diagnostic.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/appends/appends.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/asmdecl/asmdecl.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/assign/assign.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/atomic/atomic.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/atomicalign/atomicalign.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/bools/bools.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildssa/buildssa.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildtag/buildtag.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/cgocall/cgocall.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/copylock/copylock.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/ctrlflow/ctrlflow.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/deepequalerrors/deepequalerrors.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/defers/defers.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/directive/directive.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/errorsas/errorsas.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/fieldalignment/fieldalignment.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/framepointer/framepointer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/httpresponse/httpresponse.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/ifaceassert/ifaceassert.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/inspect/inspect.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/analysisutil/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclosure.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/lostcancel/lostcancel.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilfunc/nilfunc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilness/nilness.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/pkgfact/pkgfact.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/doc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/printf.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/types.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/shadow/shadow.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/shift/shift.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/sigchanyzer/sigchanyzer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/slog/slog.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/sortslice/analyzer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/stdmethods/stdmethods.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/stdversion/stdversion.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/stringintconv/string.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/structtag/structtag.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/tests/tests.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/timeformat/timeformat.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unmarshal/unmarshal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unreachable/unreachable.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unsafeptr/unsafeptr.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedresult/unusedresult.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedwrite/unusedwrite.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/waitgroup/waitgroup.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ast/astutil/imports.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/buildutil/allpackages.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/buildutil/tags.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/cfg/builder.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/cfg/cfg.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/golist.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/packages.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/visit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/builder.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/create.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/emit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/func.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/instantiate.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/ssa.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/ssautil/visit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/subst.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/typeutil/callee.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/typeutil/map.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/imports/forward.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/doc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/extractdoc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/readfile.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/version.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/typeindex/typeindex.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysisinternal/analysis.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/comment.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/equal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/stringlit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/event/core/event.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/event/core/export.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/event/label/label.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/bimport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/iexport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/iimport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/imports/fix.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/imports/sortimports.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/modindex/index.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/modindex/lookup.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/modindex/symbols.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/moreiters/iters.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/packagepath/packagepath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/delete.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/edit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/imports.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/refactor.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/deps.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/import.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/stdlib.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typeparams/normalize.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/classify_call.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/element.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/fx.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/isnamed.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/qualifier.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/typeindex/typeindex.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/types.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
cmd/machine-api-operator/start.gocmd/machineset/main.gogo.modhack/go-build.shinstall/0000_30_machine-api-operator_09_rbac.yamlinstall/0000_30_machine-api-operator_11_deployment.yamlpkg/controller/machine/machine_controller_test.gopkg/operator/config.gopkg/operator/operator.gopkg/operator/operator_test.gopkg/operator/sync.gopkg/operator/sync_test.gopkg/webhooks/machine_webhook.go
Comment on lines
+865
to
+875
| func getTLSArgs(tlsProfile configv1.TLSProfileSpec) []string { | ||
| // Compute TLS arguments once from the profile | ||
| tlsConfigFn, _ := utiltls.NewTLSConfigFromProfile(tlsProfile) | ||
| tlsConf := &tls.Config{} | ||
| tlsConfigFn(tlsConf) | ||
|
|
||
| tlsArgs := []string{} | ||
| // Only set CipherSuites if they are specified. | ||
| if len(tlsConf.CipherSuites) > 0 { | ||
| ianaCiphers := libgocrypto.CipherSuitesToNamesOrDie(tlsConf.CipherSuites) | ||
| tlsArgs = append(tlsArgs, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(ianaCiphers, ","))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find NewTLSConfigFromProfile usages
echo "=== NewTLSConfigFromProfile usages ==="
rg -n --type=go 'NewTLSConfigFromProfile\('
echo -e "\n=== unsupportedCiphers handling ==="
rg -n --type=go 'unsupportedCiphers'
echo -e "\n=== getTLSArgs function context ==="
rg -n --type=go -A 15 'func getTLSArgs'Repository: openshift/machine-api-operator
Length of output: 2879
🏁 Script executed:
#!/bin/bash
# Find the utiltls package to check the function signature
find . -name "*.go" -type f | xargs grep -l "func NewTLSConfigFromProfile" | head -5Repository: openshift/machine-api-operator
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Check klog imports and usage in sync.go
grep -n "import\|klog" pkg/operator/sync.go | head -20Repository: openshift/machine-api-operator
Length of output: 1591
Log unsupported ciphers when deriving TLS args for controllers/proxies.
The unsupportedCiphers return value from NewTLSConfigFromProfile is discarded (line 867), preventing visibility into any profile-to-flag translation mismatches. This makes debugging TLS configuration issues harder. The codebase already implements this pattern correctly in cmd/machine-api-operator/start.go (lines 270–272), which should be mirrored here.
♻️ Suggested adjustment
func getTLSArgs(tlsProfile configv1.TLSProfileSpec) []string {
// Compute TLS arguments once from the profile
- tlsConfigFn, _ := utiltls.NewTLSConfigFromProfile(tlsProfile)
+ tlsConfigFn, unsupportedCiphers := utiltls.NewTLSConfigFromProfile(tlsProfile)
+ if len(unsupportedCiphers) > 0 {
+ klog.Infof("TLS profile contains unsupported ciphers that will be ignored for controller/proxy args: %v", unsupportedCiphers)
+ }
tlsConf := &tls.Config{}
tlsConfigFn(tlsConf)📝 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.
Suggested change
| func getTLSArgs(tlsProfile configv1.TLSProfileSpec) []string { | |
| // Compute TLS arguments once from the profile | |
| tlsConfigFn, _ := utiltls.NewTLSConfigFromProfile(tlsProfile) | |
| tlsConf := &tls.Config{} | |
| tlsConfigFn(tlsConf) | |
| tlsArgs := []string{} | |
| // Only set CipherSuites if they are specified. | |
| if len(tlsConf.CipherSuites) > 0 { | |
| ianaCiphers := libgocrypto.CipherSuitesToNamesOrDie(tlsConf.CipherSuites) | |
| tlsArgs = append(tlsArgs, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(ianaCiphers, ","))) | |
| func getTLSArgs(tlsProfile configv1.TLSProfileSpec) []string { | |
| // Compute TLS arguments once from the profile | |
| tlsConfigFn, unsupportedCiphers := utiltls.NewTLSConfigFromProfile(tlsProfile) | |
| if len(unsupportedCiphers) > 0 { | |
| klog.Infof("TLS profile contains unsupported ciphers that will be ignored for controller/proxy args: %v", unsupportedCiphers) | |
| } | |
| tlsConf := &tls.Config{} | |
| tlsConfigFn(tlsConf) | |
| tlsArgs := []string{} | |
| // Only set CipherSuites if they are specified. | |
| if len(tlsConf.CipherSuites) > 0 { | |
| ianaCiphers := libgocrypto.CipherSuitesToNamesOrDie(tlsConf.CipherSuites) | |
| tlsArgs = append(tlsArgs, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(ianaCiphers, ","))) |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 867-867: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/sync.go` around lines 865 - 875, In getTLSArgs, capture the
second return value from utiltls.NewTLSConfigFromProfile (the unsupportedCiphers
slice) instead of discarding it, and emit a log containing those unsupported
cipher names so profile-to-flag translation mismatches are visible;
specifically, change the call to utiltls.NewTLSConfigFromProfile to receive
(tlsConfigFn, unsupportedCiphers), and then log unsupportedCiphers (mirroring
the pattern used in cmd/machine-api-operator/start.go) before continuing to
build the tlsArgs.
Contributor
Author
There was a problem hiding this comment.
This is intentional to avoid log spam.
There was a problem hiding this comment.
@RadekManak, understood — avoiding log spam during repeated reconciliations makes sense. Since getTLSArgs can be called multiple times when generating pod templates, keeping the logging only at startup (in start.go) is the right call.
Mirror machine-api-operator logging so ignored ciphers are visible during startup.
Contributor
|
@RadekManak: This pull request references OCPCLOUD-3346 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/machine-api-operator/start.go`:
- Around line 299-316: The code must avoid converting a shutdown-triggered cache
sync failure into a hard crash: in the initMachineAPIInformers() path where
cache.WaitForCacheSync(...) is currently followed by klog.Fatal on false, change
the logic to detect a shutdown caused by
setupTLSProfileWatcher/handleTLSProfileEvent calling shutdown() (i.e., the stop
channel being closed or context cancelled) and return an error or nil to allow
graceful restart instead of calling klog.Fatal; update initMachineAPIInformers()
to check the stop channel or context before deciding to fatal, and ensure
setupTLSProfileWatcher/handleTLSProfileEvent continue to call shutdown() as
before.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
cmd/machine-api-operator/start.gocmd/machineset/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/machineset/main.go
Comment on lines
+299
to
+316
| func setupTLSProfileWatcher(ctx *ControllerContext, shutdown func()) error { | ||
| configClient := ctx.ClientBuilder.OpenshiftClientOrDie("tls-profile-watcher") | ||
| initialProfile, err := fetchAPIServerTLSProfileSpec(context.Background(), configClient) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| klog.Fatal(server.ListenAndServe()) | ||
|
|
||
| apiServerInformer := ctx.ConfigInformerFactory.Config().V1().APIServers().Informer() | ||
| _, err = apiServerInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
| AddFunc: func(obj interface{}) { | ||
| handleTLSProfileEvent(obj, &initialProfile, shutdown) | ||
| }, | ||
| UpdateFunc: func(_, newObj interface{}) { | ||
| handleTLSProfileEvent(newObj, &initialProfile, shutdown) | ||
| }, | ||
| DeleteFunc: func(obj interface{}) { | ||
| handleTLSProfileEvent(obj, &initialProfile, shutdown) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify control flow: read the key sections mentioned in the review
echo "=== Section 1: Lines 96-140 (startup context) ==="
sed -n '96,140p' cmd/machine-api-operator/start.go
echo -e "\n=== Section 2: Lines 143-153 (cache sync with fatal) ==="
sed -n '143,153p' cmd/machine-api-operator/start.go
echo -e "\n=== Section 3: Lines 299-370 (setupTLSProfileWatcher) ==="
sed -n '299,370p' cmd/machine-api-operator/start.goRepository: openshift/machine-api-operator
Length of output: 4655
Prevent fatal crash when TLS watcher triggers shutdown before cache sync completes.
shutdown() can be called from APIServer events after the TLS watcher is registered but before initMachineAPIInformers() cache sync finishes. This closes stopCh, causing cache.WaitForCacheSync() to return false, which unconditionally triggers klog.Fatal() at line 150—converting an intended graceful restart into a hard crash.
Suggested hardening
func initMachineAPIInformers(ctx *ControllerContext) {
mInformer := ctx.MachineInformerFactory.Machine().V1beta1().Machines().Informer()
msInformer := ctx.MachineInformerFactory.Machine().V1beta1().MachineSets().Informer()
ctx.MachineInformerFactory.Start(ctx.Stop)
if !cache.WaitForCacheSync(ctx.Stop,
mInformer.HasSynced,
msInformer.HasSynced) {
+ select {
+ case <-ctx.Stop:
+ klog.V(2).Info("Skipping Machine API informer sync due to shutdown")
+ return
+ default:
+ klog.Fatal("Failed to sync caches for Machine api informers")
+ }
}
klog.Info("Synced up machine api informer caches")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/machine-api-operator/start.go` around lines 299 - 316, The code must
avoid converting a shutdown-triggered cache sync failure into a hard crash: in
the initMachineAPIInformers() path where cache.WaitForCacheSync(...) is
currently followed by klog.Fatal on false, change the logic to detect a shutdown
caused by setupTLSProfileWatcher/handleTLSProfileEvent calling shutdown() (i.e.,
the stop channel being closed or context cancelled) and return an error or nil
to allow graceful restart instead of calling klog.Fatal; update
initMachineAPIInformers() to check the stop channel or context before deciding
to fatal, and ensure setupTLSProfileWatcher/handleTLSProfileEvent continue to
call shutdown() as before.
Contributor
|
@RadekManak: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
:8443) using controller-runtime’s metrics server with delegated authn/authz (WithAuthenticationAndAuthorization), and remove the MAOkube-rbac-proxysidecar.APIServer/clusterTLS profile on startup,kube-rbac-proxysidecars (machine,machineset,mhc) by generating--tls-min-versionand profile-derived--tls-cipher-suitesargs.METRICS_PORT=8443) for direct secure serving,config.openshift.io/apiservers.controller-runtime,openshift/api,openshift/client-go, etc.),go-build.shroot-dir handling fix,pkg/webhooks/machine_webhook.go, context import updates).Notes
kube-rbac-proxyand continue using the existingnamespace/metricsauthorization model./metricsauth uses delegated token/SAR checks; scraper access relies on existing cluster-monitoringprometheus-k8scluster RBAC.Summary by CodeRabbit
New Features
Chores
Tests