ESO-511:Add TLS profile for the operator from openshift API - #178
Open
siddhibhor-56 wants to merge 2 commits into
Open
ESO-511:Add TLS profile for the operator from openshift API#178siddhibhor-56 wants to merge 2 commits into
siddhibhor-56 wants to merge 2 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-511 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe operator now resolves OpenShift TLS profiles, configures metrics and webhook TLS settings, watches cluster API server changes, and adds TLS argument helpers with validation and tests. RBAC, manifests, and Go dependencies were updated to support the integration. ChangesTLS profile integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant APIServer
participant TLSProfileResolver
participant MetricsAndWebhook
Operator->>APIServer: Fetch cluster APIServer resource
APIServer-->>TLSProfileResolver: Return honored TLS profile
TLSProfileResolver-->>Operator: Return TLS configuration
Operator->>MetricsAndWebhook: Apply TLS settings
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/external-secrets-operator/main.go`:
- Around line 211-243: Move the construction of server options involving
webhook.NewServer and metricsServerOptions.TLSOpts until after the cluster TLS
profile block completes. Ensure the applyClusterTLS append operations update the
final slices assigned to both server options, while preserving the existing TLS
resolution and application behavior.
In `@go.mod`:
- Around line 8-18: Resolve the OSV findings by updating or removing the
affected dependencies declared or selected by go.mod, including
k8s.io/kubernetes, google.golang.org/grpc, OpenTelemetry, cel-go,
golang.org/x/net, and golang.org/x/text, ensuring the listed vulnerable versions
are no longer selected. Regenerate go.sum and vendor, run go mod verify for all
four modules, and rerun the OSV scan to confirm no findings remain.
In `@pkg/controller/external_secrets/install_external_secrets.go`:
- Around line 38-56: Remove the unused TLS profile resolution and logging around
tlsprofile.ResolveHonoredTLSProfile in the external-secrets reconciliation flow,
and remove the corresponding APIServer watch in the controller. Do not fail
reconciliation based on a profile that createOrApplyDeployments does not
consume; defer TLS handling until deployment rendering supports the required
arguments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+211
to
+243
| // Resolve cluster TLS profile for the operator's own serving endpoints. | ||
| // This uses an uncached client because the manager cache is not started yet. | ||
| uncachedClient, err := client.New(restConfig, client.Options{Scheme: scheme}) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to create uncached client for TLS profile resolution") | ||
| os.Exit(1) | ||
| } | ||
| tlsSpec, err := tlsprofile.ResolveHonoredTLSProfile( | ||
| ctx, | ||
| tlsprofile.NewClientReaderAPIServerFetch(tlsprofile.NewClientReaderObjectGetter(uncachedClient)), | ||
| "external-secrets-operator", | ||
| tlsprofile.FetchErrorPropagateExceptNotFound, | ||
| ) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to resolve cluster TLS profile") | ||
| os.Exit(1) | ||
| } | ||
| if tlsSpec != nil { | ||
| tlsCfg, err := tlsprofile.ClientTLSConfig(tlsSpec, nil) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to build TLS config from cluster profile") | ||
| os.Exit(1) | ||
| } | ||
| setupLog.Info("applying cluster TLS profile to operator serving endpoints", | ||
| "minTLSVersion", tlsSpec.MinTLSVersion) | ||
| applyClusterTLS := func(c *tls.Config) { | ||
| c.MinVersion = tlsCfg.MinVersion | ||
| c.CipherSuites = tlsCfg.CipherSuites | ||
| c.CurvePreferences = tlsCfg.CurvePreferences | ||
| } | ||
| metricsTLSOpts = append(metricsTLSOpts, applyClusterTLS) | ||
| webhookTLSOpts = append(webhookTLSOpts, applyClusterTLS) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'webhook.NewServer|metricsServerOptions\.TLSOpts|append\(metricsTLSOpts|append\(webhookTLSOpts' cmd/external-secrets-operator/main.goRepository: openshift/external-secrets-operator
Length of output: 1491
Apply TLS options before constructing the server options.
webhook.NewServer and metricsServerOptions.TLSOpts capture the current slice headers. The later append calls do not update those stored headers. The resolved TLS profile therefore does not reach either server.
Assign both option slices after all TLS option appends complete.
🤖 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 `@cmd/external-secrets-operator/main.go` around lines 211 - 243, Move the
construction of server options involving webhook.NewServer and
metricsServerOptions.TLSOpts until after the cluster TLS profile block
completes. Ensure the applyClusterTLS append operations update the final slices
assigned to both server options, while preserving the existing TLS resolution
and application behavior.
Comment on lines
+8
to
+18
| github.com/openshift/api v0.0.0-20260807110950-72ae4424ef35 | ||
| github.com/openshift/library-go v0.0.0-20260807194649-ee0a87843dda | ||
| go.uber.org/zap v1.27.1 | ||
| k8s.io/api v0.35.6 | ||
| k8s.io/apiextensions-apiserver v0.35.0 | ||
| k8s.io/apimachinery v0.35.6 | ||
| k8s.io/client-go v0.35.6 | ||
| k8s.io/kubernetes v1.35.6 | ||
| k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 | ||
| sigs.k8s.io/controller-runtime v0.23.3 | ||
| k8s.io/api v0.36.3 | ||
| k8s.io/apiextensions-apiserver v0.36.2 | ||
| k8s.io/apimachinery v0.36.3 | ||
| k8s.io/client-go v0.36.2 | ||
| k8s.io/klog/v2 v2.140.0 | ||
| k8s.io/kubernetes v1.36.3 | ||
| k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 | ||
| sigs.k8s.io/controller-runtime v0.24.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for module in . cmd/external-secrets-operator test tools; do
echo "== $module =="
(
cd "$module"
go mod tidy -diff
go mod verify
go list -m -json all
) |
jq -c 'select(.Path != null and .Version != null) |
{package: {name: .Path, ecosystem: "Go"}, version: .Version}' |
while IFS= read -r query; do
curl -fsS https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
--data "$query" |
jq -c --arg module "$module" \
'select((.vulns // []) | length > 0) |
{module: $module, vulns: .vulns}'
done
doneRepository: openshift/external-secrets-operator
Length of output: 1260
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '== module files =='
git ls-files '*go.mod' '*go.sum' | sort
printf '%s\n' '== repository layout =='
git ls-files | rg '(^|/)(vendor/|go\.work$|go\.mod$|go\.sum$)' | sed -n '1,240p'
printf '%s\n' '== root go.mod =='
cat -n go.mod
printf '%s\n' '== root go.sum status =='
if test -f go.sum; then
wc -l go.sum
git status --short -- go.sum
else
echo 'go.sum not found'
fiRepository: openshift/external-secrets-operator
Length of output: 18031
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '== go.work =='
cat -n go.work
for file in cmd/external-secrets-operator/go.mod test/go.mod tools/go.mod; do
printf '\n== %s ==\n' "$file"
cat -n "$file"
done
printf '\n== vendored module metadata ==\n'
if test -f vendor/modules.txt; then
sed -n '1,80p' vendor/modules.txt
printf '%s\n' '...'
tail -80 vendor/modules.txt
else
echo 'vendor/modules.txt not found'
fi
printf '\n== module versions declared in go.mod files ==\n'
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path(".").glob("**/go.mod")):
print(f"== {path} ==")
for line in path.read_text().splitlines():
match = re.match(r"^\s*(\S+)\s+(v\S+)(?:\s+//.*)?$", line)
if match:
print(f"{match.group(1)} {match.group(2)}")
PYRepository: openshift/external-secrets-operator
Length of output: 50391
🏁 Script executed:
#!/usr/bin/env bash
set +e
for module in . cmd/external-secrets-operator test tools; do
printf '\n== %s: go mod verify ==\n' "$module"
(
cd "$module" || exit 1
GOWORK=off go mod verify
)
printf 'status=%s\n' "$?"
printf '== %s: go list -m -json all with module mode ==\n' "$module"
(
cd "$module" || exit 1
GOWORK=off go list -mod=mod -m -json all >/tmp/module-list.json
status=$?
printf 'status=%s modules=%s\n' "$status" "$(wc -l </tmp/module-list.json)"
if test "$status" -eq 0; then
jq -s 'length' /tmp/module-list.json
else
sed -n '1,12p' /tmp/module-list.json
fi
exit "$status"
)
doneRepository: openshift/external-secrets-operator
Length of output: 165
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
import json
import re
import urllib.request
from pathlib import Path
header = re.compile(r"^# (\S+) (\S+)$")
modules = {}
for line in Path("vendor/modules.txt").read_text().splitlines():
match = header.match(line)
if match and "=>" not in line:
modules[match.group(1)] = match.group(2)
sum_entries = set()
for sum_path in sorted(Path(".").glob("**/go.sum")):
for line in sum_path.read_text().splitlines():
fields = line.split()
if len(fields) >= 2:
name, version = fields[0], fields[1]
if name.endswith("/go.mod"):
name = name[:-len("/go.mod")]
sum_entries.add((name, version))
missing = sorted(
(name, version)
for name, version in modules.items()
if (name, version) not in sum_entries
or (name, version + "/go.mod") not in {
(n, v) for n, v in sum_entries
}
)
print(f"vendored_modules={len(modules)}")
print(f"go_sum_files={len(list(Path('.').glob('**/go.sum')))}")
print(f"modules_missing_a_sum_entry={len(missing)}")
for name, version in missing[:80]:
print(f"MISSING {name} {version}")
items = [
{"package": {"name": name, "ecosystem": "Go"}, "version": version}
for name, version in sorted(modules.items())
]
vulnerabilities = []
errors = []
for start in range(0, len(items), 100):
payload = json.dumps({"queries": items[start:start + 100]}).encode()
request = urllib.request.Request(
"https://api.osv.dev/v1/querybatch",
data=payload,
headers={"Content-Type": "application/json"},
method="POST",
)
try:
with urllib.request.urlopen(request, timeout=30) as response:
result = json.load(response)
except Exception as exc:
errors.append(f"batch {start}:{start + 100}: {exc}")
continue
for item, result_item in zip(items[start:start + 100], result.get("results", [])):
for vuln in result_item.get("vulns", []):
vulnerabilities.append({
"module": item["package"]["name"],
"version": item["version"],
"id": vuln.get("id"),
"summary": vuln.get("summary"),
})
print(f"osv_queries={len(items)}")
print(f"osv_vulnerabilities={len(vulnerabilities)}")
for vuln in vulnerabilities:
print("VULN " + json.dumps(vuln, sort_keys=True))
print(f"osv_errors={len(errors)}")
for error in errors:
print("OSV_ERROR " + error)
PYRepository: openshift/external-secrets-operator
Length of output: 928
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY' >/tmp/osv-querybatch.json
import json
import re
from pathlib import Path
modules = {}
for line in Path("vendor/modules.txt").read_text().splitlines():
match = re.match(r"^# (\S+) (\S+)$", line)
if match and "=>" not in line:
modules[match.group(1)] = match.group(2)
items = [
{"package": {"name": name, "ecosystem": "Go"}, "version": version}
for name, version in sorted(modules.items())
]
for start in range(0, len(items), 100):
print(json.dumps({"queries": items[start:start + 100]}))
PY
: > /tmp/osv-results.jsonl
status=0
while IFS= read -r query; do
response=$(curl -fsS -k \
https://api.osv.dev/v1/querybatch \
-H 'Content-Type: application/json' \
--data "$query") || {
echo 'OSV request failed' >&2
status=1
continue
}
printf '%s\n' "$response" >> /tmp/osv-results.jsonl
done </tmp/osv-querybatch.json
python3 - <<'PY'
import json
from pathlib import Path
batches = [json.loads(line) for line in Path("/tmp/osv-results.jsonl").read_text().splitlines()]
vulnerabilities = []
for batch in batches:
for result in batch.get("results", []):
for vuln in result.get("vulns", []):
vulnerabilities.append((vuln.get("id"), vuln.get("summary")))
print(f"osv_batches={len(batches)}")
print(f"osv_vulnerabilities={len(vulnerabilities)}")
for vuln_id, summary in vulnerabilities:
print(f"VULN {vuln_id}: {summary}")
PY
exit "$status"Repository: openshift/external-secrets-operator
Length of output: 543
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
import json
from pathlib import Path
for line in Path("/tmp/osv-results.jsonl").read_text().splitlines():
batch = json.loads(line)
for result in batch.get("results", []):
vulns = result.get("vulns", [])
if not vulns:
continue
# querybatch preserves query order, so identify the package from the
# corresponding query payload when available in the saved request file.
print(json.dumps(result, sort_keys=True))
PYRepository: openshift/external-secrets-operator
Length of output: 1173
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
import json
import re
import urllib.request
from pathlib import Path
modules = {}
for line in Path("vendor/modules.txt").read_text().splitlines():
match = re.match(r"^# (\S+) (\S+)$", line)
if match and "=>" not in line:
modules[match.group(1)] = match.group(2)
items = [
(name, version)
for name, version in sorted(modules.items())
]
vulnerability_ids = set()
for batch_index, line in enumerate(Path("/tmp/osv-results.jsonl").read_text().splitlines()):
results = json.loads(line).get("results", [])
for offset, result in enumerate(results):
ids = [v["id"] for v in result.get("vulns", [])]
if ids:
name, version = items[batch_index * 100 + offset]
print(f"{name} {version}: {', '.join(ids)}")
vulnerability_ids.update(ids)
print("\n== OSV details ==")
for vuln_id in sorted(vulnerability_ids):
request = urllib.request.Request(
f"https://api.osv.dev/v1/vulns/{vuln_id}",
headers={"Accept": "application/json"},
)
try:
with urllib.request.urlopen(request, timeout=30) as response:
vuln = json.load(response)
except Exception as exc:
print(f"{vuln_id}: ERROR {exc}")
continue
affected = []
for entry in vuln.get("affected", []):
ranges = []
for item in entry.get("ranges", []):
ranges.append({
"type": item.get("type"),
"events": item.get("events"),
})
affected.append({
"package": entry.get("package"),
"ranges": ranges,
})
print(json.dumps({
"id": vuln_id,
"summary": vuln.get("summary"),
"affected": affected,
}, sort_keys=True))
PYRepository: openshift/external-secrets-operator
Length of output: 2734
🏁 Script executed:
#!/usr/bin/env bash
set -u
for id in \
GO-2022-0635 GO-2022-0646 GHSA-gcjh-h69q-9w9g GO-2026-5158 \
GHSA-hfvc-g4fc-pqh GO-2026-5426 GO-2026-5932 GO-2026-5942 \
GO-2026-5970 GHSA-hrxh-6v49-42gf GO-2026-6061 GO-2025-3521 \
GO-2025-3547
do
curl -fsS -k "https://api.osv.dev/v1/vulns/$id" |
jq -c '{
id,
summary,
database_specific,
affected: [
.affected[] |
{
package: .package,
ranges: [.ranges[] | {type, events}]
}
]
}'
doneRepository: openshift/external-secrets-operator
Length of output: 5306
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
import re
from pathlib import Path
targets = {
"github.com/aws/aws-sdk-go",
"github.com/google/cel-go",
"go.opentelemetry.io/otel",
"go.opentelemetry.io/otel/sdk",
"golang.org/x/crypto",
"golang.org/x/net",
"golang.org/x/text",
"google.golang.org/grpc",
"k8s.io/kubernetes",
}
for path in sorted(Path(".").glob("**/go.mod")):
matches = []
for line in path.read_text().splitlines():
fields = line.split()
if len(fields) >= 2 and fields[0] in targets:
matches.append((fields[0], fields[1], "indirect" in line))
if matches:
print(path)
for name, version, indirect in matches:
print(f" {name} {version} ({'indirect' if indirect else 'direct'})")
PYRepository: openshift/external-secrets-operator
Length of output: 1149
Resolve the OSV findings before merge.
OSV flags these selected versions: k8s.io/kubernetes v1.36.3, google.golang.org/grpc v1.79.3, go.opentelemetry.io/otel v1.41.0, go.opentelemetry.io/otel/sdk v1.40.0, github.com/google/cel-go v0.26.0, golang.org/x/net before v0.56.0, and golang.org/x/text before v0.39.0. Update or remove the affected modules, regenerate go.sum and vendor, then run go mod verify for all four modules and repeat the OSV scan.
🤖 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 `@go.mod` around lines 8 - 18, Resolve the OSV findings by updating or removing
the affected dependencies declared or selected by go.mod, including
k8s.io/kubernetes, google.golang.org/grpc, OpenTelemetry, cel-go,
golang.org/x/net, and golang.org/x/text, ensuring the listed vulnerable versions
are no longer selected. Regenerate go.sum and vendor, run go mod verify for all
four modules, and rerun the OSV scan to confirm no findings remain.
Source: Path instructions
Comment on lines
+38
to
+56
| // Resolve cluster TLS profile for operand deployments. | ||
| // TODO: once the upstream external-secrets operand supports --tls-min-version, | ||
| // --tls-ciphers, and --tls-curve-preferences flags, pass tlsSpec to | ||
| // createOrApplyDeployments and inject the flags into container args. | ||
| tlsSpec, err := tlsprofile.ResolveHonoredTLSProfile( | ||
| r.ctx, | ||
| tlsprofile.NewClientReaderAPIServerFetch(r.CtrlClient), | ||
| "external-secrets", | ||
| tlsprofile.FetchErrorPropagateExceptNotFound, | ||
| ) | ||
| if err != nil { | ||
| r.log.Error(err, "failed to resolve cluster TLS profile") | ||
| return err | ||
| } | ||
| if tlsSpec != nil { | ||
| r.log.V(2).Info("resolved cluster TLS profile for operand deployments", | ||
| "minTLSVersion", tlsSpec.MinTLSVersion, | ||
| "cipherCount", len(tlsSpec.Ciphers)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not fail reconciliation for a profile that deployments do not consume.
tlsSpec is only logged. Lines 104-107 still call createOrApplyDeployments without TLS arguments. The APIServer watch in pkg/controller/external_secrets/controller.go therefore causes reconciliations that cannot update operand TLS settings.
Until the upstream operand supports these flags, remove this resolution path and its watch. Otherwise, pass the resolved profile into deployment rendering and add the generated TLS arguments.
🤖 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/external_secrets/install_external_secrets.go` around lines 38
- 56, Remove the unused TLS profile resolution and logging around
tlsprofile.ResolveHonoredTLSProfile in the external-secrets reconciliation flow,
and remove the corresponding APIServer watch in the controller. Do not fail
reconciliation based on a profile that createOrApplyDeployments does not
consume; defer TLS handling until deployment rendering supports the required
arguments.
siddhibhor-56
force-pushed
the
tls-new-main
branch
from
August 11, 2026 15:33
a0d6be4 to
181fe7b
Compare
|
@siddhibhor-56: all tests passed! 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
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
2 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.
apiserver.config.openshift.io/clusterand honoring the configuredtlsSecurityProfileandtlsAdherencesettings.Summary by CodeRabbit
New Features
Bug Fixes
Tests