Skip to content

Implement OCP TLS profile compliance for cloud providers#2647

Open
tpantelis wants to merge 5 commits intostolostron:mainfrom
tpantelis:tls_compliance
Open

Implement OCP TLS profile compliance for cloud providers#2647
tpantelis wants to merge 5 commits intostolostron:mainfrom
tpantelis:tls_compliance

Conversation

@tpantelis
Copy link
Copy Markdown
Collaborator

@tpantelis tpantelis commented Apr 2, 2026

Add TLS profile fetching from the OpenShift API Server and apply it to each cloud provider's HTTP client (AWS, AZURE, GCP AND RHOS) to ensure compliance with cluster TLS security policies.

See commits for details.

Related to https://redhat.atlassian.net/browse/ACM-30182

Summary by CodeRabbit

  • New Features

    • Cloud providers now honor the cluster TLS security profile for outbound connections.
  • Bug Fixes

    • Unified TLS/HTTP client handling across providers for more consistent, secure connections and clearer error reporting.
    • Provider initialization now reuses shared reporters and fails earlier on client configuration errors.
  • Tests

    • Added TLS package tests and suite covering profile handling, client construction, and error cases.
  • Chores

    • Updated module dependencies.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4af6e88f-1d8b-4120-963a-d3cefc4d27cf

📥 Commits

Reviewing files that changed from the base of the PR and between 40547ac and 2ac06bb.

📒 Files selected for processing (4)
  • go.mod
  • pkg/cloud/azure/azure.go
  • pkg/cloud/cloud.go
  • pkg/cloud/gcp/gcp.go
✅ Files skipped from review due to trivial changes (2)
  • pkg/cloud/cloud.go
  • pkg/cloud/azure/azure.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • go.mod

📝 Walkthrough

Walkthrough

Adds a TLS helper to derive TLS configs from the OpenShift APIServer TLSSecurityProfile and return configured HTTP clients; updates AWS/Azure/GCP/RHOS providers to reuse an event recorder and inject configured HTTP clients/transports; updates go.mod dependencies.

Changes

Cohort / File(s) Summary
Module Dependencies
go.mod
Promoted several indirect deps to direct (github.com/Azure/azure-sdk-for-go/sdk/azcore, github.com/aws/aws-sdk-go-v2/config), added github.com/openshift/controller-runtime-common, bumped k8s.io/utils, sigs.k8s.io/controller-runtime, sigs.k8s.io/structured-merge-diff/v6, and added gomodules.xyz/jsonpatch/v2 as indirect.
TLS Package & Tests
pkg/cloud/tls/config.go, pkg/cloud/tls/config_test.go, pkg/cloud/tls/tls_suite_test.go
New package providing GetConfigFromAPIServer(ctx,dynamicClient,reporter,insecureSkipVerify) (*tls.Config, error) and GetConfiguredHTTPClient(...) (*http.Client, error) which derive TLS settings from the OpenShift APIServer TLSSecurityProfile; includes Ginkgo test suite and cases for multiple profiles and failure modes.
AWS Provider
pkg/cloud/aws/aws.go
NewProvider creates/reuses a single event-recorder wrapper, obtains a configured http.Client via tls.GetConfiguredHTTPClient(...), passes it into cloud-prepare/client construction (config.WithHTTPClient(...)), extracts cloud option derivation into getCloudOptions, and reuses the reporter instance.
Azure Provider & Registration
pkg/cloud/azure/azure.go, pkg/cloud/cloud.go
azure.NewProvider signature changed to require context.Context; provider builds a configured http.Client and sets Azure SDK client transport (policy.ClientOptions/arm.ClientOptions.Transport) to it. pkg/cloud/cloud.go updated to call azure.NewProvider(context.TODO(), info).
GCP Provider
pkg/cloud/gcp/gcp.go
Creates and reuses a single event-recorder wrapper; newClient updated to accept dynamic.Interface and reporter, builds a configured http.Client via TLS package, wraps transport with OAuth2 token source, and constructs GCP client with option.WithHTTPClient(httpClient).
RHOS Provider
pkg/cloud/rhos/rhos.go
Introduces shared event recorder passed into newClient; newClient signature now accepts (ctx, dynamicClient, reporter, credentialsSecret), configures providerClient.HTTPClient via tls.GetConfiguredHTTPClient(...) before authentication, and returns wrapped errors instead of logging.
Misc / Tests
pkg/cloud/...
Added TLS package tests and suite registration; no other public API changes besides Azure provider signature update.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Provider as Cloud Provider
  participant Dynamic as Kubernetes DynamicClient
  participant TLSPkg as pkg/cloud/tls
  participant HTTP as http.Client
  participant SDK as Cloud SDK Client

  Provider->>Dynamic: GET apiserver.config.openshift.io/cluster
  Dynamic-->>Provider: APIServer (unstructured)
  Provider->>TLSPkg: GetConfigFromAPIServer(ctx, dynamicClient, reporter, insecure)
  TLSPkg->>TLSPkg: convert -> derive tls.Config (versions, ciphers)
  TLSPkg-->>HTTP: return configured *http.Client (Transport.TLSClientConfig)
  Provider->>SDK: construct SDK client with provided HTTP client/transport
  SDK-->>Provider: client uses configured TLS for outbound calls
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇
I nibbled ciphers by the server's glow,
Wove one client from whispers soft and low.
One recorder, one transport, snug and spry,
Hops secure through clouds beneath the sky. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Implement OCP TLS profile compliance for cloud providers' accurately describes the main change: adding TLS profile compliance across all cloud providers by implementing a new TLS configuration system and applying it to AWS, Azure, GCP, and RHOS providers.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tpantelis tpantelis changed the title Implement OCP TLS profile compliance for RHOS provider Implement OCP TLS profile compliance for cloud provider Apr 2, 2026
@tpantelis tpantelis changed the title Implement OCP TLS profile compliance for cloud provider Implement OCP TLS profile compliance for cloud providers Apr 2, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/cloud/gcp/gcp.go (1)

50-53: ⚠️ Potential issue | 🔴 Critical

Logic bug: instanceType assignment is inverted.

The condition checks if instanceType != "" (non-empty) but then overwrites it with gwInstanceType. This should check for empty string to apply the default.

🐛 Proposed fix
 	instanceType := info.GatewayConfig.GCP.InstanceType
-	if instanceType != "" {
+	if instanceType == "" {
 		instanceType = gwInstanceType
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/gcp/gcp.go` around lines 50 - 53, The assignment for instanceType
is inverted: currently it overwrites a non-empty
info.GatewayConfig.GCP.InstanceType with gwInstanceType; change the conditional
to check for an empty string (info.GatewayConfig.GCP.InstanceType == "" or
instanceType == "") and only assign gwInstanceType when the original value is
empty so that instanceType uses the provided config value when present and falls
back to gwInstanceType when absent.
🧹 Nitpick comments (2)
pkg/cloud/tls/config_test.go (1)

77-79: Remove unused ctx parameter from BeforeEach.

Several BeforeEach functions accept ctx context.Context but don't use it. This applies to lines 77, 92, 119, and 134.

♻️ Proposed fix for line 77
-		BeforeEach(func(ctx context.Context) {
+		BeforeEach(func() {
 			apiServer.Spec.TLSSecurityProfile.Type = configv1.TLSProfileModernType
 		})

Similar changes needed for lines 92, 119, and 134.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/tls/config_test.go` around lines 77 - 79, Remove the unused ctx
parameter from the test BeforeEach closures in pkg/cloud/tls/config_test.go:
change the BeforeEach(func(ctx context.Context) { ... }) blocks to
BeforeEach(func() { ... }) for each occurrence (the closures that set
apiServer.Spec.TLSSecurityProfile.Type and the other three similar BeforeEach
blocks), and ensure any inner code does not reference ctx; update the function
signatures so they match the Ginkgo BeforeEach without parameters.
pkg/cloud/cloud.go (1)

48-64: Consider propagating context through ProviderFn signature.

All provider registrations use context.TODO() because the ProviderFn type doesn't accept a context. Since Get() is called during request handling (where a proper context exists), consider updating ProviderFn to accept context.Context:

type ProviderFn func(context.Context, *provider.Info) (Provider, error)

This would allow proper context propagation from the caller rather than using context.TODO().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/cloud.go` around lines 48 - 64, Update the ProviderFn signature to
accept a context (change type ProviderFn to func(context.Context,
*provider.Info) (Provider, error)), then update all RegisterProvider invocations
(the AWS/GCP/OpenStack/Azure registrations) to pass the incoming context instead
of using context.TODO(), update the implementation callers such as Get() to
accept/forward a context when invoking ProviderFn, and ensure any provider
constructors (e.g., aws.NewProvider, gcp.NewProvider, rhos.NewProvider,
azure.NewProvider) calls match the new signature so context is properly
propagated to Provider creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/cloud/gcp/gcp.go`:
- Around line 157-158: Run gofumpt on the file to fix the formatting lint
failure for the newClient signature: reformat the function declaration for
newClient(ctx context.Context, dynamicClient dynamic.Interface, rep
submreporter.Interface, credentialsSecret *corev1.Secret) so parameter alignment
and spacing match gofumpt's rules (or run `gofumpt -w` on the file); ensure the
signature and surrounding braces/indentation conform to the formatter so the
linter error is resolved.

In `@pkg/cloud/rhos/rhos.go`:
- Around line 206-208: The code unsafely type-asserts
http.DefaultTransport.(*http.Transport) which can panic if DefaultTransport has
been replaced; change the block to perform a safe type assertion (t, ok :=
http.DefaultTransport.(*http.Transport)) and if ok use t.Clone(), otherwise
create a new &http.Transport{} as a fallback, then set TLSClientConfig =
tlsConfig and assign providerClient.HTTPClient = http.Client{Transport:
transport}; reference symbols: http.DefaultTransport, *http.Transport, Clone(),
tlsConfig, transport, providerClient.HTTPClient.

---

Outside diff comments:
In `@pkg/cloud/gcp/gcp.go`:
- Around line 50-53: The assignment for instanceType is inverted: currently it
overwrites a non-empty info.GatewayConfig.GCP.InstanceType with gwInstanceType;
change the conditional to check for an empty string
(info.GatewayConfig.GCP.InstanceType == "" or instanceType == "") and only
assign gwInstanceType when the original value is empty so that instanceType uses
the provided config value when present and falls back to gwInstanceType when
absent.

---

Nitpick comments:
In `@pkg/cloud/cloud.go`:
- Around line 48-64: Update the ProviderFn signature to accept a context (change
type ProviderFn to func(context.Context, *provider.Info) (Provider, error)),
then update all RegisterProvider invocations (the AWS/GCP/OpenStack/Azure
registrations) to pass the incoming context instead of using context.TODO(),
update the implementation callers such as Get() to accept/forward a context when
invoking ProviderFn, and ensure any provider constructors (e.g.,
aws.NewProvider, gcp.NewProvider, rhos.NewProvider, azure.NewProvider) calls
match the new signature so context is properly propagated to Provider creation.

In `@pkg/cloud/tls/config_test.go`:
- Around line 77-79: Remove the unused ctx parameter from the test BeforeEach
closures in pkg/cloud/tls/config_test.go: change the BeforeEach(func(ctx
context.Context) { ... }) blocks to BeforeEach(func() { ... }) for each
occurrence (the closures that set apiServer.Spec.TLSSecurityProfile.Type and the
other three similar BeforeEach blocks), and ensure any inner code does not
reference ctx; update the function signatures so they match the Ginkgo
BeforeEach without parameters.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 131bf7a1-b1f5-4ea1-9915-fb8220a4485e

📥 Commits

Reviewing files that changed from the base of the PR and between 716b1ac and fcdf09f.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • go.mod
  • pkg/cloud/aws/aws.go
  • pkg/cloud/azure/azure.go
  • pkg/cloud/cloud.go
  • pkg/cloud/gcp/gcp.go
  • pkg/cloud/rhos/rhos.go
  • pkg/cloud/tls/config.go
  • pkg/cloud/tls/config_test.go
  • pkg/cloud/tls/tls_suite_test.go

@tpantelis tpantelis force-pushed the tls_compliance branch 2 times, most recently from fdbd762 to 56c60ac Compare April 3, 2026 02:31
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/cloud/rhos/rhos.go (1)

189-199: ⚠️ Potential issue | 🟠 Major

Configure the RHOS transport before Keystone authentication.

Line 189 calls openstack.AuthenticatedClient(), which performs the initial Keystone authentication (version discovery and token request) before lines 194–199 create and assign the custom HTTP client. This means the first network operation uses the default transport, bypassing cloud.Verify and the cluster's TLS profile settings. Use openstack.NewClient() followed by assigning the configured HTTP client, then call openstack.Authenticate() to ensure the custom transport handles all authentication requests.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/rhos/rhos.go` around lines 189 - 199, The code currently calls
openstack.AuthenticatedClient which performs Keystone auth before the custom
transport is set, causing initial requests to bypass TLS settings; replace that
flow by creating the client with openstack.NewClient, set
providerClient.HTTPClient to the HTTP client returned by
tls.GetConfiguredHTTPClient (honoring cloud.Verify / TLS profile), then call
openstack.Authenticate to perform authentication using the configured transport.
Locate and update usage of openstack.AuthenticatedClient,
tls.GetConfiguredHTTPClient, providerClient.HTTPClient and switch to
openstack.NewClient + openstack.Authenticate so all auth network calls use the
custom transport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@pkg/cloud/rhos/rhos.go`:
- Around line 189-199: The code currently calls openstack.AuthenticatedClient
which performs Keystone auth before the custom transport is set, causing initial
requests to bypass TLS settings; replace that flow by creating the client with
openstack.NewClient, set providerClient.HTTPClient to the HTTP client returned
by tls.GetConfiguredHTTPClient (honoring cloud.Verify / TLS profile), then call
openstack.Authenticate to perform authentication using the configured transport.
Locate and update usage of openstack.AuthenticatedClient,
tls.GetConfiguredHTTPClient, providerClient.HTTPClient and switch to
openstack.NewClient + openstack.Authenticate so all auth network calls use the
custom transport.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0f8ee93d-66aa-428f-95e7-4bc91731a602

📥 Commits

Reviewing files that changed from the base of the PR and between fcdf09f and 56c60ac.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • go.mod
  • pkg/cloud/aws/aws.go
  • pkg/cloud/azure/azure.go
  • pkg/cloud/cloud.go
  • pkg/cloud/gcp/gcp.go
  • pkg/cloud/rhos/rhos.go
  • pkg/cloud/tls/config.go
  • pkg/cloud/tls/config_test.go
  • pkg/cloud/tls/tls_suite_test.go
✅ Files skipped from review due to trivial changes (1)
  • pkg/cloud/gcp/gcp.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • pkg/cloud/cloud.go
  • pkg/cloud/tls/tls_suite_test.go
  • pkg/cloud/tls/config_test.go
  • go.mod
  • pkg/cloud/azure/azure.go

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@pkg/cloud/tls/config.go`:
- Around line 51-65: The code can silently fall back to Go's default ciphers
when openshifttls.NewTLSConfigFromProfile filters out all pre-TLS1.3 suites:
after calling tlsConfigFunc(tlsConfig) check the profile/tls version intent and
the resulting tlsConfig.CipherSuites; if the requested MinVersion is < TLS1.3
(i.e. profile expects TLS1.0–1.2) and tlsConfig.CipherSuites is empty (or all
were listed in unsupportedCiphers), surface a hard failure via reporter.Error
(or return an error) instead of proceeding — use the returned unsupportedCiphers
slice and tlsConfig.CipherSuites to detect this condition and include
identifying context in the error message so the caller can fail fast rather than
silently using Go's defaults.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f6015932-2d6c-4682-8023-0174a52abef8

📥 Commits

Reviewing files that changed from the base of the PR and between 56c60ac and 2224a0b.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • go.mod
  • pkg/cloud/aws/aws.go
  • pkg/cloud/azure/azure.go
  • pkg/cloud/cloud.go
  • pkg/cloud/gcp/gcp.go
  • pkg/cloud/rhos/rhos.go
  • pkg/cloud/tls/config.go
  • pkg/cloud/tls/config_test.go
  • pkg/cloud/tls/tls_suite_test.go
✅ Files skipped from review due to trivial changes (2)
  • pkg/cloud/tls/tls_suite_test.go
  • pkg/cloud/tls/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • pkg/cloud/cloud.go
  • pkg/cloud/azure/azure.go
  • go.mod
  • pkg/cloud/gcp/gcp.go

Add TLS profile fetching from the OpenShift API Server and apply it to
the RHOS cloud provider's HTTP client to ensure compliance with cluster
TLS security policies.

Implementation:
- Created pkg/cloud/tls package to fetch TLS profiles from apiserver.config.openshift.io/cluster
- Uses controller-runtime-common/pkg/tls for profile spec extraction and conversion
- Respects InsecureSkipVerify from clouds.yaml while applying TLS restrictions
- Updated RHOS provider to use TLS helper instead of hardcoded InsecureSkipVerify
- Added comprehensive unit tests covering all profile types

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Apply cluster TLS profile to AWS SDK HTTP client to ensure compliance
with OCP TLS profile requirements.

Implementation:
- Reuse existing pkg/cloud/tls package to Fetch the TLS profile from
  the API server
- Create HTTP client with cluster TLS configuration
- Pass custom HTTP client to AWS SDK via config.WithHTTPClient()

The AWS SDK v2 accepts custom HTTP clients via LoadOptions, allowing us
to inject the cluster TLS profile without modifying the cloud-prepare
library.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
...and avoid gocyclo linter error.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@tpantelis
Copy link
Copy Markdown
Collaborator Author

/override ci/prow/sonarcloud

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 3, 2026

@tpantelis: Overrode contexts on behalf of tpantelis: ci/prow/sonarcloud

Details

In response to this:

/override ci/prow/sonarcloud

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/cloud/gcp/gcp.go (1)

156-180: ⚠️ Potential issue | 🔴 Critical

The current implementation will fail all GCP API calls with 401 authentication errors at runtime.

tls.GetConfiguredHTTPClient() returns a bare *http.Client with TLS settings only—no authentication. Per official google.golang.org/api documentation, option.WithHTTPClient takes precedence and causes option.WithCredentials(creds) to be ignored entirely. The service account credentials are dropped, leaving only the TLS-configured client with no auth capability.

To fix this, create the TLS client first, then wrap its transport with oauth2.Transport to apply authentication while preserving TLS settings, and pass only the wrapped client via option.WithHTTPClient (remove option.WithCredentials).

Suggested fix
-	creds, err := google.CredentialsFromJSONWithType(ctx, authJSON, google.ServiceAccount, dns.CloudPlatformScope)
-	if err != nil {
-		return "", nil, errors.Wrap(err, "error retrieving credentials")
-	}
-
-	httpClient, err := tls.GetConfiguredHTTPClient(ctx, dynamicClient, rep, false)
+	baseHTTPClient, err := tls.GetConfiguredHTTPClient(ctx, dynamicClient, rep, false)
 	if err != nil {
 		return "", nil, errors.Wrap(err, "unable to create HTTP client")
 	}
+
+	oauthCtx := context.WithValue(ctx, oauth2.HTTPClient, baseHTTPClient)
+	creds, err := google.CredentialsFromJSONWithType(oauthCtx, authJSON, google.ServiceAccount, dns.CloudPlatformScope)
+	if err != nil {
+		return "", nil, errors.Wrap(err, "error retrieving credentials")
+	}
+
+	httpClient := *baseHTTPClient
+	httpClient.Transport = &oauth2.Transport{
+		Base:   baseHTTPClient.Transport,
+		Source: creds.TokenSource,
+	}
 
 	// Create a GCP client with the credentials and custom HTTP client.
 	computeClient, err := gcpclient.NewClient(ctx, creds.ProjectID, []option.ClientOption{
-		option.WithCredentials(creds),
-		option.WithHTTPClient(httpClient),
+		option.WithHTTPClient(&httpClient),
 	})

Add "golang.org/x/oauth2" to imports.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/gcp/gcp.go` around lines 156 - 180, The GCP client is losing
authentication because option.WithHTTPClient overrides option.WithCredentials;
in newClient, call tls.GetConfiguredHTTPClient first to get httpClient, then
wrap its Transport with an oauth2.Transport using creds.TokenSource(ctx) (ensure
you import "golang.org/x/oauth2"), set httpClient.Transport =
&oauth2.Transport{Source: creds.TokenSource(ctx), Base: httpClient.Transport},
and then call gcpclient.NewClient with only option.WithHTTPClient(httpClient)
(remove option.WithCredentials). This preserves TLS config from
tls.GetConfiguredHTTPClient while applying the service account auth from creds.
pkg/cloud/azure/azure.go (1)

62-89: ⚠️ Potential issue | 🟠 Major

Wire the TLS transport into Azure auth as well.

This only applies the custom client to ARM requests. azidentity.NewEnvironmentCredential(nil) still builds its own pipeline for AAD token acquisition, so the auth path won't honor the cluster TLS profile. Create the credential after httpClient and pass the same transport via EnvironmentCredentialOptions.

Possible fix
-	credentials, err := azidentity.NewEnvironmentCredential(nil)
-	if err != nil {
-		return nil, errors.Wrap(err, "unable to create the Azure credentials")
-	}
-
 	rep := reporter.NewEventRecorderWrapper("AzureCloudProvider", info.EventRecorder)
 
 	httpClient, err := tls.GetConfiguredHTTPClient(ctx, info.DynamicClient, rep, false)
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to create HTTP client")
 	}
+
+	credentials, err := azidentity.NewEnvironmentCredential(&azidentity.EnvironmentCredentialOptions{
+		ClientOptions: policy.ClientOptions{
+			Transport: httpClient,
+		},
+	})
+	if err != nil {
+		return nil, errors.Wrap(err, "unable to create the Azure credentials")
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cloud/azure/azure.go` around lines 62 - 89, The credential is being
created before wiring the custom TLS HTTP transport so
azidentity.NewEnvironmentCredential(nil) will not honor the cluster TLS profile;
move creation of the credential to after tls.GetConfiguredHTTPClient(...) and
call azidentity.NewEnvironmentCredential with an EnvironmentCredentialOptions
that injects the same httpClient/transport you got from
tls.GetConfiguredHTTPClient (so TokenCredential in azure.CloudInfo uses the
custom transport). Update references to azidentity.NewEnvironmentCredential,
tls.GetConfiguredHTTPClient, the credentials variable, and
azure.CloudInfo.TokenCredential accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@pkg/cloud/azure/azure.go`:
- Around line 62-89: The credential is being created before wiring the custom
TLS HTTP transport so azidentity.NewEnvironmentCredential(nil) will not honor
the cluster TLS profile; move creation of the credential to after
tls.GetConfiguredHTTPClient(...) and call azidentity.NewEnvironmentCredential
with an EnvironmentCredentialOptions that injects the same httpClient/transport
you got from tls.GetConfiguredHTTPClient (so TokenCredential in azure.CloudInfo
uses the custom transport). Update references to
azidentity.NewEnvironmentCredential, tls.GetConfiguredHTTPClient, the
credentials variable, and azure.CloudInfo.TokenCredential accordingly.

In `@pkg/cloud/gcp/gcp.go`:
- Around line 156-180: The GCP client is losing authentication because
option.WithHTTPClient overrides option.WithCredentials; in newClient, call
tls.GetConfiguredHTTPClient first to get httpClient, then wrap its Transport
with an oauth2.Transport using creds.TokenSource(ctx) (ensure you import
"golang.org/x/oauth2"), set httpClient.Transport = &oauth2.Transport{Source:
creds.TokenSource(ctx), Base: httpClient.Transport}, and then call
gcpclient.NewClient with only option.WithHTTPClient(httpClient) (remove
option.WithCredentials). This preserves TLS config from
tls.GetConfiguredHTTPClient while applying the service account auth from creds.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 94d2b77b-8904-41cd-a609-79c913f7385b

📥 Commits

Reviewing files that changed from the base of the PR and between 2224a0b and 40547ac.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • go.mod
  • pkg/cloud/aws/aws.go
  • pkg/cloud/azure/azure.go
  • pkg/cloud/cloud.go
  • pkg/cloud/gcp/gcp.go
  • pkg/cloud/rhos/rhos.go
  • pkg/cloud/tls/config.go
  • pkg/cloud/tls/config_test.go
  • pkg/cloud/tls/tls_suite_test.go
✅ Files skipped from review due to trivial changes (1)
  • pkg/cloud/tls/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • pkg/cloud/cloud.go
  • pkg/cloud/tls/tls_suite_test.go
  • go.mod
  • pkg/cloud/rhos/rhos.go

Apply cluster TLS profile to GCP SDK HTTP client to ensure compliance
with OCP TLS profile requirements.

Implementation:
- Reuse existing pkg/cloud/tls package to fetch the TLS profile from
  the API server
- Create HTTP client with cluster TLS configuration
- Pass custom HTTP client to GCP SDK via option.WithHTTPClient()

The GCP SDK accepts custom HTTP clients via ClientOption, allowing us
to inject the cluster TLS profile without modifying the cloud-prepare
library.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Apply cluster TLS profile to Azure SDK HTTP client to ensure compliance
with OCP TLS profile requirements.

Implementation:
- Reuse existing pkg/cloud/tls package to fetch the TLS profile from
  the API server
- Create HTTP client with cluster TLS configuration
- Pass custom HTTP client to Azure SDK via ClientOptions.Transport

The Azure SDK accepts custom HTTP clients via policy.ClientOptions,
allowing us to inject the cluster TLS profile without modifying the
cloud-prepare library.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@tpantelis
Copy link
Copy Markdown
Collaborator Author

/override ci/prow/sonarcloud

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 3, 2026

@tpantelis: Overrode contexts on behalf of tpantelis: ci/prow/sonarcloud

Details

In response to this:

/override ci/prow/sonarcloud

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.

@tpantelis
Copy link
Copy Markdown
Collaborator Author

/cc @aswinsuryan

@aswinsuryan
Copy link
Copy Markdown
Contributor

changes looks good , but we will need to test this in each provider once this goes in,

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 8, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aswinsuryan
Once this PR has been reviewed and has the lgtm label, please assign skitt for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tpantelis
Copy link
Copy Markdown
Collaborator Author

/cc @vthapar

@openshift-ci openshift-ci bot requested a review from vthapar April 8, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants