Repository navigation
Add pluggable ClusterTLSPolicy extension point for helm-operator - #7121
Open
mytreya-rh wants to merge 4 commits into
Open
mytreya-rh wants to merge 4 commits into
mytreya-rh wants to merge 4 commits into
Conversation
mytreya-rh
force-pushed
the
add-cluster-tls-policy-extension-point
branch
from
August 14, 2026 06:35
bfb274c to
26edce7
Compare
Bumps sigs.k8s.io/controller-runtime v0.21.0 -> v0.24.1 and
k8s.io/{api,apiextensions-apiserver,apimachinery,cli-runtime,client-go,kubectl}
v0.33.9 -> v0.36.2, plus transitive dependency updates picked up by
`go mod tidy`.
This is a routine dependency bump with no feature content. It's a
prerequisite for downstream distributions that want to adopt
github.com/openshift/controller-runtime-common/pkg/tls (which requires
controller-runtime >= v0.22.5) or similar packages built against newer
controller-runtime/k8s.io APIs.
Also fixes fallout the bump surfaces:
- internal/olm/client/client_test.go's errClient test double needs a new
Apply method to satisfy controller-runtime's client.Client interface
(client.Writer gained Apply in this version range).
- internal/helm/controller/controller.go:65 and
internal/olm/operator/uninstall.go:198 are pre-existing call sites that
`make test-sanity`'s golangci-lint now flags as SA1019 (deprecated),
because the bump changes what staticcheck can see as deprecated:
controller-runtime's GetEventRecorderFor is newly deprecated in this
version range (it wasn't in v0.21.0), and k8s.io/kubectl's
slice.ContainsString was already deprecated in v0.33.9 but its doc
comment didn't follow the convention staticcheck requires until v0.36.2
reformatted it. slice.ContainsString is swapped for the stdlib
slices.Contains (identical behavior, no modifier func was used);
GetEventRecorderFor is left as-is with a //nolint:staticcheck (matching
controller-runtime's own internal usage) since migrating
HelmOperatorReconciler off the old events API is a larger, unrelated
change.
Co-authored-by: Cursor <cursoragent@cursor.com>
mytreya-rh
force-pushed
the
add-cluster-tls-policy-extension-point
branch
from
August 14, 2026 06:59
26edce7 to
3e54911
Compare
The prior commit left HelmOperatorReconciler.EventRecorder on the
deprecated Manager.GetEventRecorderFor/record.EventRecorder pair,
suppressed with a //nolint:staticcheck, calling the full migration a
larger, unrelated change. Do that migration now, as its own commit, so
staticcheck (SA1019) stays clean without a nolint annotation.
Switch HelmOperatorReconciler.EventRecorder to
k8s.io/client-go/tools/events.EventRecorder (populated via the new
Manager.GetEventRecorder) and update both Eventf call sites to the new
signature: Eventf(regarding, related, eventtype, reason, action, note,
args...). The related object is always nil here since there's no
separate related object modeled for override-value events. The reason
string ("OverrideValuesInUse") is reused as the action value since this
code doesn't otherwise model a distinct action; happy to adjust if
maintainers prefer a different action string.
No go.mod/go.sum/vendor changes: k8s.io/client-go/tools/events is
already a transitive dependency via controller-runtime's
Manager.GetEventRecorder.
Co-authored-by: Cursor <cursoragent@cursor.com>
Adds internal/cmd/helm-operator/run/tlspolicy.go, defining a generic ClusterTLSPolicy interface (Apply/Watch) and RegisterClusterTLSPolicy, plus minimal hook call-sites in cmd.go. This lets a distribution of the helm-operator plug in centrally-managed TLS configuration (e.g. sourced from cluster-wide config) without operator-sdk itself knowing about any specific source. No implementation is registered by default, so this is a no-op for anyone who doesn't call RegisterClusterTLSPolicy: safe, backward compatible, and free of new dependencies. For example, an OpenShift distribution can register an implementation that reads apiservers.config.openshift.io/cluster and applies its TLS security profile to the metrics server, watching for changes to trigger a graceful restart. Co-authored-by: Cursor <cursoragent@cursor.com>
Covers registration, overwrite behavior, and the fail-open contract documented on ClusterTLSPolicy.Apply. Co-authored-by: Cursor <cursoragent@cursor.com>
mytreya-rh
force-pushed
the
add-cluster-tls-policy-extension-point
branch
from
August 14, 2026 08:06
3e54911 to
2ae8cb2
Compare
mytreya-rh
added a commit
to mytreya-rh/ocp-release-operator-sdk
that referenced
this pull request
Oct 9, 2026
Addresses CodeRabbit and @chiragkyal review feedback on PR openshift#460, except CK-2/CK-3/CK-13 (tracked separately: CK-2 is blocked on upstream PR operator-framework/operator-sdk#7121, CK-3 needs more discussion on Watch() failure semantics, CK-13 wants a dedicated e2e suite - a larger follow-up) and comments that didn't hold up to verification (see .work/pr460-review-comments-analysis.md for the full per-comment writeup): internal/helm/openshifttls/tls.go, tls_test.go: - Apply now returns an error immediately if the client used to fetch the TLS profile can't even be constructed, instead of silently falling back to the default profile. manager.New (called right after Apply, with the same *rest.Config) would fail moments later anyway in that case, so this doesn't change overall operator resilience; it just stops masking the real error. - fetchProfile now also reports whether the profile was actually read from a live APIServer object. Watch uses this to skip registering its profile-change watcher when it wasn't, instead of unconditionally starting a cache watch for a resource that may not exist (which can stall manager startup waiting for informer sync on vanilla Kubernetes). - defaultProfile() simplified: GetTLSProfileSpec(nil) never errors. - clusterTLSPolicy gains an injectable newClient field (defaulting to ctrlclient.New) purely so tests can exercise Apply() itself instead of re-implementing its callback logic; ctrlclient.New performs live API discovery during construction and can't be driven directly against a fake/unreachable *rest.Config in a unit test. - Tests rewritten to call clusterTLSPolicy.Apply directly (via the injected client) rather than just NewTLSConfigFromProfile, made table-driven across Modern (MinVersion only) and Intermediate (MinVersion + CipherSuites) profiles, and extended with cases for the client-construction-failure and Watch-skip paths above. ci/tests/e2e-helm.sh: - Quote the $IMAGE and $ROOTDIR expansions in the deploy_operator make/pushd invocations (shellcheck SC2086). go.mod/go.sum/vendor: - Bump google.golang.org/grpc (indirect) 1.80.0 -> 1.82.1 and oras.land/oras-go/v2 (indirect) 2.6.0 -> 2.6.2 to pick up fixes for GHSA-hrxh-6v49-42gf/GO-2026-6061 and several oras-go/v2 advisories flagged by OSV Scanner against this PR. RBAC isolation (avoids ever touching upstream-shared files for this downstream-only permission again): - internal/plugins/helm/v1/scaffolds/internal/templates/config/rbac/manager_role.go reverted to its pre-PR (upstream) content. This Helm plugin scaffold template is only rendered by the `operator-sdk` CLI's init/create-api commands, which this fork doesn't use downstream (the CLI has been deprecated since OCP 4.19; this repo only builds the helm-operator base image) - there was no payoff for carrying a permanent diff against a shared file here. - testdata/helm/memcached-operator/config/rbac/role.yaml reverted to its pre-PR content; the config.openshift.io/apiservers rule now lives in two new, never-upstream files instead (openshift_tls_profile_role.yaml / _role_binding.yaml), referenced by a 2-line append to kustomization.yaml. `kustomize build config/default` was verified to still produce the same aggregate RBAC for the controller-manager ServiceAccount. - testdata/helm/memcached-operator/bundle/manifests/ memcached-operator.clusterserviceversion.yaml hand-edit reverted. Nothing in this repo's CI deploys or validates this CSV (`make deploy` here is plain kustomize, not OLM/bundle-based), and config/manifests/kustomization.yaml composes the bundle's CSV permissions from config/rbac at generation time, so a future `make bundle` run picks up the new ClusterRole/Binding automatically without needing a hand-edit. Verified: go build/vet/test ./... (excluding test/e2e, test/integration, which need a live cluster) and golangci-lint run (2.9.0, matching make test-sanity) are all clean after this commit. No GitHub comments were replied to as part of this change. Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
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
1 participant
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.
Summary
Depends on #7120 (dependency bump) — this branch is stacked on top of it, so the diff below currently includes both commits; once #7120 merges this will shrink to just the extension point.
Adds a generic extension point that lets a distribution or deployment of the helm-operator plug in centrally-managed TLS configuration for the metrics server.
internal/cmd/helm-operator/run/tlspolicy.go: aClusterTLSPolicyinterface (Apply/Watch) andRegisterClusterTLSPolicy.cmd.go: before constructing the manager, call the registered policy'sApply(if any) to augmentmanager.Options; after constructing it, callWatchso implementations can react to policy changes at runtime (e.g. cancelling the run context to trigger a graceful restart).init()callsRegisterClusterTLSPolicy(typically wired in via a blank import fromcmd/helm-operator/main.go). No new dependencies, no behavior change for anyone who doesn't use it.Test plan
go build ./...,go vet ./...pass.internal/cmd/helm-operator/run/tlspolicy_test.gocovering registration, overwrite behavior, and the fail-open contract documented onClusterTLSPolicy.Apply.Made with Cursor