Tokenize Credential Request AWS#782
Open
desmax74 wants to merge 3 commits into
Open
Conversation
Reviewer's GuideIntegrates OpenShift Cloud Credential Operator (CCO) support into the Trusted Profile Analyzer Helm chart and operator, adding cloud-provider-specific credential requests (AWS/GCP), STS/manual token support, and RDS IAM auth wiring, along with corresponding RBAC, docs, and tests. Sequence diagram for CCO-managed credentials and RDS IAM authsequenceDiagram
actor Admin
participant RhtpaOperator as rhtpa_operator
participant CCO as cloud_credential_operator
participant Cloud as CloudProvider
participant Secret as RhtpaCloudCredsSecret
participant TrustdPods as trustd_pods
participant S3
participant RDS
Admin->>RhtpaOperator: Apply TrustedProfileAnalyzer CR
RhtpaOperator->>RhtpaOperator: Render credentialrequest.yaml
RhtpaOperator->>CCO: create CredentialsRequest
CCO->>Cloud: provision credentials (mint/passthrough/manual)
Cloud-->>CCO: cloud credentials
CCO->>Secret: create Secret <release-name>-cloud-creds
Note over TrustdPods,Secret: Pod startup using Helm templates
RhtpaOperator->>TrustdPods: deploy pods with<br/>trustification.cco.volumes<br/>trustification.cco.volumeMounts<br/>trustification.cco.envVars<br/>trustification.cco.rds.envVars<br/>trustification.storage.envVars
Secret-->>TrustdPods: aws_access_key_id, aws_secret_access_key
alt non-manual CCO mode
TrustdPods->>S3: use TRUSTD_S3_ACCESS_KEY / SECRET_KEY
TrustdPods->>RDS: use TRUSTD_DB_PASSWORD (no IAM auth)
else RDS IAM auth enabled
TrustdPods->>RDS: use TRUSTD_DB_IAM_AUTH with AWS_* env vars
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Helm template for
credentialrequest.yamlhas inconsistent indentation and mixed static/dynamicstatementEntries/predefinedRolesblocks, which may produce invalid or unexpected YAML; consider simplifying to either fully static defaults or a cleanly mergedtoYamlblock with carefully aligned indentation. - The
deployment.yamlcurrently hardcodes an AWS-specific secret and environment variables; ifcloudProvidercan be GCP/Azure, it would be better to conditionally inject provider-specific env vars or secrets to avoid misconfigured deployments for non-AWS providers. - The values for AWS (
s3:*) and the hardcodeds3:GetObject/PutObject/ListBucketactions in the credentials request may be redundant or conflicting; aligning the default actions with what the template actually renders will make the permissions model clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Helm template for `credentialrequest.yaml` has inconsistent indentation and mixed static/dynamic `statementEntries`/`predefinedRoles` blocks, which may produce invalid or unexpected YAML; consider simplifying to either fully static defaults or a cleanly merged `toYaml` block with carefully aligned indentation.
- The `deployment.yaml` currently hardcodes an AWS-specific secret and environment variables; if `cloudProvider` can be GCP/Azure, it would be better to conditionally inject provider-specific env vars or secrets to avoid misconfigured deployments for non-AWS providers.
- The values for AWS (`s3:*`) and the hardcoded `s3:GetObject`/`PutObject`/`ListBucket` actions in the credentials request may be redundant or conflicting; aligning the default actions with what the template actually renders will make the permissions model clearer and easier to maintain.
## Individual Comments
### Comment 1
<location path="helm-charts/redhat-trusted-profile-analyzer/values.yaml" line_range="82-85" />
<code_context>
+ cloudProvider: aws # aws | gcp | azure
+ cloudCredentials:
+ aws:
+ statementEntries:
+ - effect: Allow
+ action: [ "s3:*" ]
+ resource: "*"
+ gcp:
</code_context>
<issue_to_address>
**🚨 issue (security):** AWS credentials default to full S3 access, which is overly permissive
The default `cloudCredentials.aws.statementEntries` grant `"s3:*"` on all resources, which is effectively admin-level S3 access and may be unacceptable in locked-down environments. Please scope the default to specific buckets and limited actions, or require users to provide these permissions explicitly rather than shipping such broad defaults.
</issue_to_address>
### Comment 2
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/credentialrequest.yaml" line_range="11-20" />
<code_context>
+ name: {{ .Release.Name }}-cloud-creds
+ namespace: {{ .Release.Namespace }}
+
+ {{- if eq .Values.cloudProvider "aws" }}
+ providerSpec:
+ apiVersion: cloudcredential.openshift.io/v1
+ kind: AWSProviderSpec
+ statementEntries:
+ - effect: Allow
+ action:
+ - "s3:GetObject"
+ - "s3:PutObject"
+ - "s3:ListBucket"
+ resource: "*"
+ {{- toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6 }}
+ {{- else if eq .Values.cloudProvider "gcp" }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Indentation and mixed static/dynamic `statementEntries` may yield invalid or duplicated YAML
The `providerSpec` block is over-indented relative to the `if`, and `statementEntries` is both hardcoded and redefined via `toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6`. This can break the YAML structure or create a malformed `statementEntries` field. Please align `providerSpec` with its peers and use a single, consistently indented `statementEntries` definition (either fully templated or merged cleanly).
</issue_to_address>
### Comment 3
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/credentialrequest.yaml" line_range="23-27" />
<code_context>
+ - "s3:ListBucket"
+ resource: "*"
+ {{- toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6 }}
+ {{- else if eq .Values.cloudProvider "gcp" }}
+ providerSpec:
+ apiVersion: cloudcredential.openshift.io/v1
+ kind: GCPProviderSpec
+ predefinedRoles:
+ {{- toYaml .Values.cloudCredentials.gcp.permissions | nindent 6 }}
+ {{- end }}
</code_context>
<issue_to_address>
**issue (bug_risk):** GCP `predefinedRoles` seems to be used with permission strings instead of role names
Here `predefinedRoles` is populated from `cloudCredentials.gcp.permissions`, but those values look like permission strings (e.g. `compute.instances.list`) instead of IAM role IDs (e.g. `roles/...`). Since `GCPProviderSpec.predefinedRoles` requires role names, this is likely to fail or not grant the required access. Either switch to the appropriate field for listing permissions (e.g. `serviceAccountPermissions`) or map these permissions to suitable predefined roles before using this field.
</issue_to_address>
### Comment 4
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/deployment.yaml" line_range="11-20" />
<code_context>
+ containers:
+ - name: my-operator
+ env:
+ - name: AWS_ACCESS_KEY_ID
+ valueFrom:
+ secretKeyRef:
+ name: {{ .Release.Name }}-cloud-creds
+ key: aws_access_key_id
+ - name: AWS_SECRET_ACCESS_KEY
+ valueFrom:
+ secretKeyRef:
+ name: {{ .Release.Name }}-cloud-creds
+ key: aws_secret_access_key
</code_context>
<issue_to_address>
**issue (bug_risk):** Deployment always expects AWS-style secrets even when using non-AWS providers
Env vars are always populated from `{{ .Release.Name }}-cloud-creds` using AWS-specific keys. For `cloudProvider: gcp` (or others), the credentialsRequest will likely create a different secret schema, so this pod won’t receive valid creds. Please gate these env vars on the provider, or introduce provider-specific secret layouts/templates so non-AWS providers don’t break this deployment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
desmax74
force-pushed
the
occ
branch
3 times, most recently
from
b1a1eab to
49cc78e
Compare
desmax74
force-pushed
the
occ
branch
4 times, most recently
from
bd39ddd to
5c4f03e
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 13 issues, and left some high level feedback:
- In
templates/credentialrequest.yaml, the AWS/GCP branches assume.Values.cloudCredentials.aws.statementEntriesor.Values.cloudCredentials.gcp.permissionsare always present; consider guarding these withifchecks or providing sane defaults so that settingcloudProviderwithout the correspondingcloudCredentialssub-structure does not cause a Helm render error. - The
TRUSTD_S3_ACCESS_KEY/TRUSTD_S3_SECRET_KEYenv var wiring in_storage.tplalways referencesaws_access_key_id/aws_secret_access_keyfrom the CCO secret even whencloudProviderisgcp; if GCP CCO produces a different secret shape, you may need provider-specific key selection rather than hard‑coding AWS-style keys. - The new
config/rbac/clusterrole.yamldefines anamespaceon aClusterRole, which is ignored for cluster-scoped resources; it would be clearer to drop the namespace field to avoid confusion about the scope of these permissions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `templates/credentialrequest.yaml`, the AWS/GCP branches assume `.Values.cloudCredentials.aws.statementEntries` or `.Values.cloudCredentials.gcp.permissions` are always present; consider guarding these with `if` checks or providing sane defaults so that setting `cloudProvider` without the corresponding `cloudCredentials` sub-structure does not cause a Helm render error.
- The `TRUSTD_S3_ACCESS_KEY`/`TRUSTD_S3_SECRET_KEY` env var wiring in `_storage.tpl` always references `aws_access_key_id`/`aws_secret_access_key` from the CCO secret even when `cloudProvider` is `gcp`; if GCP CCO produces a different secret shape, you may need provider-specific key selection rather than hard‑coding AWS-style keys.
- The new `config/rbac/clusterrole.yaml` defines a `namespace` on a `ClusterRole`, which is ignored for cluster-scoped resources; it would be clearer to drop the namespace field to avoid confusion about the scope of these permissions.
## Individual Comments
### Comment 1
<location path="test/integration/cco_rbac_test.go" line_range="110" />
<code_context>
+ assert.Equal(t, "cco-credentialsrequest-access", binding.RoleRef.Name, "roleRef name should match the CCO ClusterRole")
+}
+
+func TestCCOClusterRoleBindingSubjects(t *testing.T) {
+ bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Make subject assertions robust to multiple or reordered subjects by checking all entries
This test currently assumes the first subject is the operator ServiceAccount, so added or reordered subjects could cause false failures or hide regressions. Instead, iterate over `binding.Subjects` and assert that at least one subject has the expected ServiceAccount name and kind (e.g., via a small helper like `any(binding.Subjects, matchesSA)`) so the test remains robust while still verifying the binding targets the correct ServiceAccount.
Suggested implementation:
```golang
func TestCCOClusterRoleBindingSubjects(t *testing.T) {
bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
binding := loadClusterRoleBinding(t, bindingPath)
expectedSAName := "cluster-cloud-controller-manager-operator"
found := false
for _, subject := range binding.Subjects {
if subject.Kind == "ServiceAccount" && subject.Name == expectedSAName {
found = true
break
}
}
assert.Truef(t, found, "expected at least one ServiceAccount subject named %q in ClusterRoleBinding %q", expectedSAName, binding.Metadata.Name)
```
1. Ensure there is a helper `loadClusterRoleBinding(t *testing.T, path string)` (or equivalent) that returns the parsed ClusterRoleBinding used by the other CCO RBAC tests. If the name differs, adjust the call accordingly.
2. If the expected ServiceAccount name is defined as a constant or used elsewhere in this test file, replace the hardcoded `"cluster-cloud-controller-manager-operator"` with that constant to keep things DRY and consistent.
3. Confirm that `binding.Metadata.Name` and `binding.Subjects` are the correct fields based on the struct type you are using (likely `rbacv1.ClusterRoleBinding` or a local wrapper); adjust field access if your type differs.
4. Make sure the necessary imports (`testing`, `path/filepath`, and `github.com/stretchr/testify/assert`) are present in the file’s import block.
</issue_to_address>
### Comment 2
<location path="extensions/README.md" line_range="9" />
<code_context>
+```console
+oc get clusteroperator cloud-credential
+```
+it show somethig like
+
+```console
</code_context>
<issue_to_address>
**issue (typo):** Fix verb agreement and spelling in "it show somethig like".
Change to "it shows something like" to fix the verb form and spelling.
```suggestion
it shows something like
```
</issue_to_address>
### Comment 3
<location path="extensions/README.md" line_range="15" />
<code_context>
+NAME VERSION AVAILABLE PROGRESSING DEGRADED SINCE
+cloud-credential 4.x.x True False False ...
+```
+Pod' status
+```console
+oc get pods -n openshift-cloud-credential-operator
</code_context>
<issue_to_address>
**issue (typo):** Correct the apostrophe in "Pod' status".
Use "Pod status" or "Pods status" instead; the current apostrophe is incorrect.
```suggestion
Pod status
```
</issue_to_address>
### Comment 4
<location path="extensions/README.md" line_range="34-35" />
<code_context>
+oc get cloudcredential cluster -o yaml
+```
+On spec.credentialsMode will be the configured setting (Mint,
+Passthrough, Manual, o empty for default).
+
+## How the Operator interact with the CCO
</code_context>
<issue_to_address>
**issue (typo):** Replace "o" with "or" in the list of modes.
It should read: `Passthrough, Manual, or empty for default).`
```suggestion
On spec.credentialsMode will be the configured setting (Mint,
Passthrough, Manual, or empty for default).
```
</issue_to_address>
### Comment 5
<location path="extensions/README.md" line_range="37" />
<code_context>
+On spec.credentialsMode will be the configured setting (Mint,
+Passthrough, Manual, o empty for default).
+
+## How the Operator interact with the CCO
+
+1. Operator declare permissions needed in a CredentialsRequest CR in the namespace openshift-cloud-credential-operator
</code_context>
<issue_to_address>
**issue (typo):** Fix subject–verb agreement in the section title.
Please change the heading text to “How the Operator interacts with the CCO”.
```suggestion
## How the Operator interacts with the CCO
```
</issue_to_address>
### Comment 6
<location path="extensions/README.md" line_range="39" />
<code_context>
+
+## How the Operator interact with the CCO
+
+1. Operator declare permissions needed in a CredentialsRequest CR in the namespace openshift-cloud-credential-operator
+
+```console
</code_context>
<issue_to_address>
**issue (typo):** Use "declares" instead of "declare" for subject–verb agreement.
Because "Operator" is singular, the verb form should be "declares."
Suggested implementation:
```
1. Operator declares permissions needed in a CredentialsRequest CR in the namespace openshift-cloud-credential-operator
```
If similar list items or descriptions elsewhere in this README use "Operator declare" with the singular "Operator," they should also be updated to "Operator declares" for consistent subject–verb agreement.
</issue_to_address>
### Comment 7
<location path="extensions/README.md" line_range="62" />
<code_context>
+ resource: "*"
+```
+
+2. CCO process the CR and create a Kubernetes Secret with the cloud credentails
+ in the namespace specified in spec.secretRef.
+
</code_context>
<issue_to_address>
**issue (typo):** Fix verb form and spelling in "process" and "credentails".
Consider: “CCO processes the CR and creates a Kubernetes Secret with the cloud credentials …” to fix the verb agreement and spelling.
Suggested implementation:
```
2. CCO processes the CR and creates a Kubernetes Secret with the cloud credentials
in the namespace specified in spec.secretRef.
```
If this same sentence or similar wording appears elsewhere in the documentation (e.g., in other language versions or duplicated sections), the same corrections to verb agreement (“processes”, “creates”) and spelling (“credentials”) should be applied there as well for consistency.
</issue_to_address>
### Comment 8
<location path="extensions/README.md" line_range="65-66" />
<code_context>
+2. CCO process the CR and create a Kubernetes Secret with the cloud credentails
+ in the namespace specified in spec.secretRef.
+
+3. Operator reads the Secret and use the credentials to interact with the cloud API.
+ Operator must tolerate the not immediate availability of the secret becasue take time to create.
+
+## How integrate the CCO with the Helm Chart Operator
</code_context>
<issue_to_address>
**issue (typo):** Improve verb agreement and fix the typo "becasue" in the description.
For example: “Operator reads the Secret and uses the credentials to interact with the cloud API. The Operator must tolerate the non-immediate availability of the Secret because it takes time to create.” This corrects the verb, spelling, and missing word.
```suggestion
3. Operator reads the Secret and uses the credentials to interact with the cloud API.
The Operator must tolerate the non-immediate availability of the Secret because it takes time to create.
```
</issue_to_address>
### Comment 9
<location path="extensions/README.md" line_range="68" />
<code_context>
+3. Operator reads the Secret and use the credentials to interact with the cloud API.
+ Operator must tolerate the not immediate availability of the secret becasue take time to create.
+
+## How integrate the CCO with the Helm Chart Operator
+
+1. Define Credential Request in the chart
</code_context>
<issue_to_address>
**issue (typo):** Add "to" for correct phrasing in the section title.
Consider rephrasing the header as: "How to integrate the CCO with the Helm Chart Operator."
```suggestion
## How to integrate the CCO with the Helm Chart Operator
```
</issue_to_address>
### Comment 10
<location path="extensions/README.md" line_range="74-75" />
<code_context>
+2.
+2. Configure Deployment to use the secret created by the CCO
+
+3. Handling the delay of the creation of the secret
+ first approact Init container
+```console
+ initContainers:
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "approact" and clarify the phrase about the first approach.
Consider correcting "approact" and rephrasing this line to something like "first approach: init container" for clarity.
```suggestion
3. Handling the delay of the creation of the secret
first approach: init container
```
</issue_to_address>
### Comment 11
<location path="extensions/README.md" line_range="92" />
<code_context>
+```
+ second approach Retry in the Operator's code, but this is available on a full Go Operator
+
+4. Support different cloud provider with values configurations
+
+5. RBAC needed to create the CredentialsRequest
</code_context>
<issue_to_address>
**issue (typo):** Use the plural "providers" instead of "provider".
Because you mention “different” cloud vendors, this bullet should read: “Support different cloud providers with values configurations.”
```suggestion
4. Support different cloud providers with values configurations
```
</issue_to_address>
### Comment 12
<location path="extensions/README.md" line_range="94-95" />
<code_context>
+4. Support different cloud provider with values configurations
+
+5. RBAC needed to create the CredentialsRequest
+6. Supporto Manual Mode (STS/WIF) if the cluster use mode o STS, CCO didn't create the secret automatically.
+ In STS mode the user must :
+ 1. Extract CredentialsRequest from chart
</code_context>
<issue_to_address>
**issue (typo):** Fix multiple small language issues in the "Supporto Manual Mode" sentence.
Consider: “Support Manual Mode (STS/WIF): if the cluster uses STS mode, CCO doesn’t create the secret automatically.” This corrects “Supporto,” subject-verb agreement, “o” → “or,” and the tense of “didn’t.”
```suggestion
5. RBAC needed to create the CredentialsRequest
6. Support Manual Mode (STS/WIF): if the cluster uses STS mode, CCO doesn’t create the secret automatically.
```
</issue_to_address>
### Comment 13
<location path="extensions/README.md" line_range="99" />
<code_context>
+ In STS mode the user must :
+ 1. Extract CredentialsRequest from chart
+ 2. Use ccoctl tool to generate the credentials
+ 3. Create manually the secrets before install the chart
+
</code_context>
<issue_to_address>
**issue (typo):** Improve word order and verb form in "Create manually the secrets before install the chart".
Change this to: "Create the secrets manually before installing the chart."
```suggestion
3. Create the secrets manually before installing the chart.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
desmax74
force-pushed
the
occ
branch
3 times, most recently
from
191abed to
13fc004
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In
credentialrequest.yaml, rendering assumes.Values.cloudCredentials.aws.statementEntries/.cloudCredentials.gcp.permissionsare always set whencloudProvideris set; consider adding guards or defaults so the chart doesn’t fail to render if these subkeys are omitted or empty. - The
_storage.tplhelper always referencesaws_access_key_id/aws_secret_access_keykeys from the CCO secret even whencloudProviderisgcp; if GCP is intended to be supported, this template likely needs to branch on provider or use provider-agnostic keys. - There are multiple string constants for common field names (e.g.,
fieldMetrics,fieldStorage, etc.) spread across e2e and integration tests; consider centralizing these in a shared test helper to avoid divergence and make future refactors simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `credentialrequest.yaml`, rendering assumes `.Values.cloudCredentials.aws.statementEntries` / `.cloudCredentials.gcp.permissions` are always set when `cloudProvider` is set; consider adding guards or defaults so the chart doesn’t fail to render if these subkeys are omitted or empty.
- The `_storage.tpl` helper always references `aws_access_key_id` / `aws_secret_access_key` keys from the CCO secret even when `cloudProvider` is `gcp`; if GCP is intended to be supported, this template likely needs to branch on provider or use provider-agnostic keys.
- There are multiple string constants for common field names (e.g., `fieldMetrics`, `fieldStorage`, etc.) spread across e2e and integration tests; consider centralizing these in a shared test helper to avoid divergence and make future refactors simpler.
## Individual Comments
### Comment 1
<location path="go.mod" line_range="3" />
<code_context>
module github.com/trustification/trusted-profile-analyzer-operator
-go 1.25.0
+go 1.26
require (
</code_context>
<issue_to_address>
**issue (bug_risk):** Targeting Go 1.26 is likely invalid and will break builds with current toolchains
Since Go 1.26 is not released yet, `go 1.26` will make `go` tool invocations (including CI) fail with an `unknown major version` error. Please instead target the latest stable version supported by your toolchain and pipeline (e.g. a current 1.22.x).
</issue_to_address>
### Comment 2
<location path="config/rbac/clusterrole.yaml" line_range="2-5" />
<code_context>
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+ name: cco-credentialsrequest-access
+ namespace: placeholder
+rules:
+ - apiGroups: ["cloudcredential.openshift.io"]
</code_context>
<issue_to_address>
**nitpick:** ClusterRole resources are cluster-scoped and shouldn't include a namespace field
Kubernetes will ignore this field on a ClusterRole, but it can still mislead readers and trigger warnings in tooling. Please remove the namespace field from this manifest (and any other cluster-scoped RBAC resources) to keep it clean and avoid unnecessary validation noise.
</issue_to_address>
### Comment 3
<location path="test/integration/helm_chart_test.go" line_range="182" />
<code_context>
assert.NoError(t, err, ".helmignore should exist")
}
+
+func TestValuesSchemaContainsCCOFields(t *testing.T) {
+ schemaPath := filepath.Join("helm-charts", "redhat-trusted-profile-analyzer", "values.schema.json")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend schema test to cover ccoMode/cloudCredentials structure and constraints, not just presence
Right now this only checks the new top-level CCO fields and the `cloudProvider` enum. To better guard against regressions, please also assert:
- `ccoMode`’s type (and allowed values if it’s an enum/oneOf).
- That `cloudCredentials` is an object and, for each provider (e.g. `aws`, `gcp`), the expected sub-structure exists (e.g. `statementEntries`/`permissions` types).
- Any key required vs optional constraints needed for backwards compatibility.
This will ensure the test fails if the schema shape (types/enums/required fields) changes, not only when properties are removed.
Suggested implementation:
```golang
func TestValuesSchemaContainsCCOFields(t *testing.T) {
schemaPath := filepath.Join("helm-charts", "redhat-trusted-profile-analyzer", "values.schema.json")
data, err := os.ReadFile(schemaPath)
require.NoError(t, err, "values.schema.json should be readable")
var schema map[string]interface{}
err = yaml.Unmarshal(data, &schema)
require.NoError(t, err, "values.schema.json should be valid JSON")
// top-level properties
props, ok := schema["properties"].(map[string]interface{})
require.True(t, ok, "schema.properties should be an object")
// ccoMode field: type + enum (if present)
ccoModeSchema, ok := props["ccoMode"].(map[string]interface{})
require.True(t, ok, "schema.properties.ccoMode should be an object")
ccoModeType, ok := ccoModeSchema["type"].(string)
require.True(t, ok, "schema.properties.ccoMode.type should be a string")
assert.Equal(t, "string", ccoModeType, "ccoMode should be of type string")
if enumRaw, hasEnum := ccoModeSchema["enum"]; hasEnum {
enumValues, ok := enumRaw.([]interface{})
require.True(t, ok, "schema.properties.ccoMode.enum should be an array")
require.NotEmpty(t, enumValues, "schema.properties.ccoMode.enum should not be empty")
// If specific enums are defined, we assert the known values for compatibility.
// Adjust this list if the schema defines a different set of allowed modes.
expectedModes := map[string]struct{}{
"manual": {},
"managed": {},
"mint": {},
"passthrough": {},
"disabled": {},
}
for _, v := range enumValues {
s, ok := v.(string)
require.True(t, ok, "ccoMode enum values should be strings")
_, known := expectedModes[s]
assert.Truef(t, known, "unexpected ccoMode enum value %q", s)
}
}
// cloudCredentials object and provider-specific structure
cloudCredsSchema, ok := props["cloudCredentials"].(map[string]interface{})
require.True(t, ok, "schema.properties.cloudCredentials should be an object")
cloudCredsType, ok := cloudCredsSchema["type"].(string)
require.True(t, ok, "schema.properties.cloudCredentials.type should be a string")
assert.Equal(t, "object", cloudCredsType, "cloudCredentials should be of type object")
cloudCredsProps, ok := cloudCredsSchema["properties"].(map[string]interface{})
require.True(t, ok, "schema.properties.cloudCredentials.properties should be an object")
// AWS structure: cloudCredentials.aws.statementEntries
if awsRaw, hasAWS := cloudCredsProps["aws"]; hasAWS {
awsSchema, ok := awsRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.aws should be an object")
awsProps, ok := awsSchema["properties"].(map[string]interface{})
require.True(t, ok, "cloudCredentials.aws.properties should be an object")
statementEntriesSchemaRaw, hasStatementEntries := awsProps["statementEntries"]
require.True(t, hasStatementEntries, "cloudCredentials.aws.properties.statementEntries should exist")
statementEntriesSchema, ok := statementEntriesSchemaRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.aws.properties.statementEntries should be an object")
statementEntriesType, ok := statementEntriesSchema["type"].(string)
require.True(t, ok, "cloudCredentials.aws.properties.statementEntries.type should be a string")
assert.Equal(t, "array", statementEntriesType, "cloudCredentials.aws.statementEntries should be an array")
if itemsRaw, hasItems := statementEntriesSchema["items"]; hasItems {
itemsSchema, ok := itemsRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.aws.statementEntries.items should be an object")
// We expect each statement entry to be an object.
itemType, hasItemType := itemsSchema["type"].(string)
if hasItemType {
assert.Equal(t, "object", itemType, "cloudCredentials.aws.statementEntries.items.type should be object")
}
}
// Check required keys for AWS if defined to ensure backward compatibility.
if requiredRaw, hasRequired := awsSchema["required"]; hasRequired {
requiredFields, ok := requiredRaw.([]interface{})
require.True(t, ok, "cloudCredentials.aws.required should be an array")
require.NotEmpty(t, requiredFields, "cloudCredentials.aws.required should not be empty")
}
}
// GCP structure: cloudCredentials.gcp.permissions
if gcpRaw, hasGCP := cloudCredsProps["gcp"]; hasGCP {
gcpSchema, ok := gcpRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.gcp should be an object")
gcpProps, ok := gcpSchema["properties"].(map[string]interface{})
require.True(t, ok, "cloudCredentials.gcp.properties should be an object")
permissionsSchemaRaw, hasPermissions := gcpProps["permissions"]
require.True(t, hasPermissions, "cloudCredentials.gcp.properties.permissions should exist")
permissionsSchema, ok := permissionsSchemaRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.gcp.properties.permissions should be an object")
permissionsType, ok := permissionsSchema["type"].(string)
require.True(t, ok, "cloudCredentials.gcp.properties.permissions.type should be a string")
assert.Equal(t, "array", permissionsType, "cloudCredentials.gcp.permissions should be an array")
if itemsRaw, hasItems := permissionsSchema["items"]; hasItems {
itemsSchema, ok := itemsRaw.(map[string]interface{})
require.True(t, ok, "cloudCredentials.gcp.permissions.items should be an object")
itemType, hasItemType := itemsSchema["type"].(string)
if hasItemType {
assert.Equal(t, "string", itemType, "cloudCredentials.gcp.permissions.items.type should be string")
}
}
// Check required keys for GCP if defined.
if requiredRaw, hasRequired := gcpSchema["required"]; hasRequired {
requiredFields, ok := requiredRaw.([]interface{})
require.True(t, ok, "cloudCredentials.gcp.required should be an array")
require.NotEmpty(t, requiredFields, "cloudCredentials.gcp.required should not be empty")
}
}
// cloudCredentials required vs optional constraints:
// If the schema defines a required array for cloudCredentials, assert it contains
// the keys we rely on for backwards compatibility.
if requiredRaw, hasRequired := cloudCredsSchema["required"]; hasRequired {
requiredFields, ok := requiredRaw.([]interface{})
require.True(t, ok, "cloudCredentials.required should be an array")
requiredSet := map[string]struct{}{}
for _, r := range requiredFields {
s, ok := r.(string)
require.True(t, ok, "cloudCredentials.required entries should be strings")
requiredSet[s] = struct{}{}
}
// Adjust these expectations if specific providers must/must-not be required.
// For now, we only assert that required is not empty if it exists.
require.NotEmpty(t, requiredSet, "cloudCredentials.required should not be empty when present")
}
}
```
1. The exact `ccoMode` enum values and the provider-specific field names (`statementEntries`, `permissions`, etc.) may differ from what is implemented in your `values.schema.json`. You should align the `expectedModes` map and the nested property names/types with the actual schema.
2. If additional cloud providers (e.g. `azure`) are present in `cloudCredentials.properties`, you may want to extend this test with similar structural assertions for those providers.
3. If your schema intentionally does not define `required` arrays for some objects, you can relax or remove the `require.NotEmpty` assertions for those scopes to better reflect the intended optionality.
</issue_to_address>
### Comment 4
<location path="test/e2e/cco_helm_rendering_test.go" line_range="243" />
<code_context>
+ assert.Nil(t, cr, "CredentialsRequest should NOT be rendered without cloudProvider")
+}
+
+func TestHelmRenderStorageEnvVarsWithCCO(t *testing.T) {
+ if testing.Short() {
+ t.Skip(skipE2ETest)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a GCP variant of the storage env var test to ensure CCO-managed GCP credentials are wired correctly when supported
Since this PR mentions both AWS and GCP CCO flows, please either add a corresponding GCP-focused test (using `gcpValues()` or similar with CCO enabled and asserting the expected GCP credential secrets in the `Deployment`), or add a brief comment here clarifying that env var wiring is intentionally limited to AWS only.
Suggested implementation:
```golang
metadata, _ := cr[fieldMetadata].(map[string]interface{})
func TestHelmRenderStorageEnvVarsWithCCOGCP(t *testing.T) {
if testing.Short() {
t.Skip(skipE2ETest)
}
chartPath := getChartPath(t)
rendered := renderHelmChart(t, chartPath, gcpValues())
docs := splitYAMLDocs(rendered)
deploy := findDocByKindAndName(t, docs, kindDeployment, "observatorium-operator")
require.NotNil(t, deploy, "Deployment should be rendered for GCP")
spec, _ := deploy["spec"].(map[string]interface{})
template, _ := spec["template"].(map[string]interface{})
podSpec, _ := template["spec"].(map[string]interface{})
containers, _ := podSpec["containers"].([]interface{})
require.NotEmpty(t, containers, "Deployment should have containers")
container, _ := containers[0].(map[string]interface{})
envVars, _ := container["env"].([]interface{})
// Assert that the expected CCO-managed GCP credentials secret is wired into the env vars.
require.Condition(t, func() bool {
for _, e := range envVars {
env, _ := e.(map[string]interface{})
name, _ := env["name"].(string)
if name == "GOOGLE_APPLICATION_CREDENTIALS" {
valueFrom, _ := env["valueFrom"].(map[string]interface{})
secretRef, _ := valueFrom["secretKeyRef"].(map[string]interface{})
secretName, _ := secretRef["name"].(string)
secretKey, _ := secretRef["key"].(string)
return secretName != "" && secretKey != ""
}
}
return false
}, "GCP CCO-managed credentials env var should be present")
}
```
To fully integrate this test with the existing suite, you should:
1. Ensure `gcpValues()` enables the CCO-managed GCP credentials flow in the same way that `awsValues()` enables the AWS CCO flow (e.g. setting the corresponding `cco`/`cloudProvider` values).
2. Confirm that `findDocByKindAndName` exists and matches the helper signature used elsewhere; if not, introduce a helper similar to `findDocByKind` that also filters on the `metadata.name` field.
3. Optionally, refine the `GOOGLE_APPLICATION_CREDENTIALS` env var name and expected secret name/key to exactly match what the Helm chart renders for CCO-managed GCP credentials.
</issue_to_address>
### Comment 5
<location path="test/e2e/cco_helm_rendering_test.go" line_range="312" />
<code_context>
+ t.Fatal("server deployment not found in rendered output")
+}
+
+func TestHelmRenderStorageEnvVarsWithoutCCO(t *testing.T) {
+ if testing.Short() {
+ t.Skip(skipE2ETest)
</code_context>
<issue_to_address>
**nitpick:** Use the ccoSecretName constant instead of a hardcoded substring when asserting no CCO secret references
The assertion currently checks for the substring "cloud-creds" while the constant is `ccoSecretName = "test-release-cloud-creds"`. To avoid divergence and keep the test resilient to naming changes, assert against `ccoSecretName` directly (e.g. `assert.NotContains(t, rendered, ccoSecretName, ...)`).
</issue_to_address>
### Comment 6
<location path="extensions/README.md" line_range="52-54" />
<code_context>
+ namespace: my-operator-namespace
+ providerSpec:
+ apiVersion: cloudcredential.openshift.io/v1
+ kind: AWSProviderSpec # esempio per AWS
+ statementEntries:
+ - effect: Allow
</code_context>
<issue_to_address>
**suggestion (typo):** Use an English comment instead of "esempio" for consistency.
Specifically, update `# esempio per AWS` to an English version such as `# example for AWS` to match the rest of the documentation.
```suggestion
apiVersion: cloudcredential.openshift.io/v1
kind: AWSProviderSpec # example for AWS
statementEntries:
```
</issue_to_address>
### Comment 7
<location path="extensions/README.md" line_range="89-90" />
<code_context>
+ sleep 5
+ done
+```
+ second approach Retry in the Operator's code, but this is available on a full Go Operator
+
+4. Support different cloud providers with values configurations
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify and fix grammar in the description of the second approach.
For example: "Second approach: retry in the Operator's code, but this is only available for a full Go Operator."
```suggestion
```
Second approach: retry in the Operator's code, but this is only available for a full Go Operator.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
desmax74
force-pushed
the
occ
branch
3 times, most recently
from
d511877 to
8c9d943
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_storage.tplhelper always wiresTRUSTD_S3_ACCESS_KEY/TRUSTD_S3_SECRET_KEYtoaws_access_key_id/aws_secret_access_key, which looks AWS-specific; whencloudProvider: gcpis used the generated secret will likely have different keys, so the template should branch on provider and use provider-appropriate secret keys. - In
templates/credentialrequest.yamlthe template assumes.Values.cloudCredentials.aws/.Values.cloudCredentials.gcpare populated whenevercloudProvideris set; consider adding guards or defaults so the chart either fails fast with a clear message or omits theCredentialsRequestwhen these blocks are missing, instead of rendering incomplete specs. - The CCO helper
_cco.tplonly emits manual-mode env vars for AWS; ifcloudProvider: gcpis set withccoMode: manualthe chart will be in an unsupported/partially configured state, so you may want to either explicitly reject that combination or add a GCP manual-mode branch.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_storage.tpl` helper always wires `TRUSTD_S3_ACCESS_KEY`/`TRUSTD_S3_SECRET_KEY` to `aws_access_key_id` / `aws_secret_access_key`, which looks AWS-specific; when `cloudProvider: gcp` is used the generated secret will likely have different keys, so the template should branch on provider and use provider-appropriate secret keys.
- In `templates/credentialrequest.yaml` the template assumes `.Values.cloudCredentials.aws` / `.Values.cloudCredentials.gcp` are populated whenever `cloudProvider` is set; consider adding guards or defaults so the chart either fails fast with a clear message or omits the `CredentialsRequest` when these blocks are missing, instead of rendering incomplete specs.
- The CCO helper `_cco.tpl` only emits manual-mode env vars for AWS; if `cloudProvider: gcp` is set with `ccoMode: manual` the chart will be in an unsupported/partially configured state, so you may want to either explicitly reject that combination or add a GCP manual-mode branch.
## Individual Comments
### Comment 1
<location path="test/e2e/cco_helm_rendering_test.go" line_range="227-236" />
<code_context>
+func TestHelmRenderGCPCredentialsRequest(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add GCP-specific tests for how the generated secret is consumed by Deployments
Right now this only checks that the GCP `CredentialsRequest` has the expected provider spec. Unlike the AWS tests, it doesn’t verify that the resulting secret is actually used by the Deployments.
To align with the AWS coverage, please add tests that:
- Render the chart with `cloudProvider = gcp` and valid `cloudCredentials.gcp`.
- Find the relevant `Deployment` manifests (server/importer) in the rendered output.
- Assert that the env vars / volume mounts for storage credentials reference the secret name and keys created by the GCP `CredentialsRequest`.
That will validate the full GCP wiring, not just the CR generation.
Suggested implementation:
```golang
func TestHelmRenderGCPCredentialsRequest(t *testing.T) {
if testing.Short() {
t.Skip(skipE2ETest)
}
chartPath := getChartPath(t)
rendered := renderHelmChart(t, chartPath, gcpValues())
docs := splitYAMLDocs(rendered)
cr := findCredentialsRequest(t, docs)
require.NotNil(t, cr, "CredentialsRequest should be rendered for GCP")
// Extract the secret name and keys from the GCP CredentialsRequest so we can
// assert that the Deployments are wired up to use it.
secretName, secretKeyClientEmail, secretKeyPrivateKey := extractGCPCredentialsRequestSecretRef(t, cr)
// Verify that the server Deployment consumes the generated credentials secret
serverDeployment := findDeployment(t, docs, "server") // align name with AWS tests / chart
require.NotNil(t, serverDeployment, "server Deployment should be rendered for GCP")
assertDeploymentUsesGCPCredentialsSecret(
t,
serverDeployment,
secretName,
secretKeyClientEmail,
secretKeyPrivateKey,
)
// Verify that the importer Deployment (if rendered for GCP) also consumes the same secret.
// If the importer is not rendered for GCP in this chart, this assertion can be relaxed
// or the test can be updated to match the actual behavior.
importerDeployment := findDeployment(t, docs, "importer") // align name with AWS tests / chart
if importerDeployment != nil {
assertDeploymentUsesGCPCredentialsSecret(
t,
importerDeployment,
secretName,
secretKeyClientEmail,
secretKeyPrivateKey,
)
}
}
// extractGCPCredentialsRequestSecretRef extracts the secret name and the relevant
// key names from the GCP CredentialsRequest. This mirrors the logic used by the
// AWS tests to keep provider coverage aligned.
func extractGCPCredentialsRequestSecretRef(t *testing.T, cr map[string]interface{}) (secretName, keyClientEmail, keyPrivateKey string) {
spec, ok := cr["spec"].(map[string]interface{})
require.True(t, ok, "CredentialsRequest.spec should be a map")
secretRef, ok := spec["secretRef"].(map[string]interface{})
require.True(t, ok, "CredentialsRequest.spec.secretRef should be a map")
name, ok := secretRef["name"].(string)
require.True(t, ok, "CredentialsRequest.spec.secretRef.name should be a string")
// The keys below should match what the GCP provider/ccoctl expects to write
// into the generated Secret for workload identity credentials.
keyClientEmail = "service_account_email"
keyPrivateKey = "service_account_key"
return name, keyClientEmail, keyPrivateKey
}
// assertDeploymentUsesGCPCredentialsSecret validates that the given Deployment
// has env vars / volume mounts wired to the provided credentials Secret.
// This is the GCP equivalent of the AWS wiring tests, ensuring end-to-end coverage
// from CredentialsRequest -> Secret -> Deployment.
func assertDeploymentUsesGCPCredentialsSecret(
t *testing.T,
deployment map[string]interface{},
secretName string,
keyClientEmail string,
keyPrivateKey string,
) {
containers := getDeploymentContainers(t, deployment)
// Depending on how the chart is structured, credentials may be passed as
// env vars, volume mounts, or both. The assertions below assume env vars
// similar to the AWS tests and should be aligned with the actual chart.
for _, c := range containers {
env := getContainerEnv(t, c)
// Assert env vars sourcing from the credentials Secret.
assertEnvVarFromSecret(t, env, "GCP_SERVICE_ACCOUNT_EMAIL", secretName, keyClientEmail)
assertEnvVarFromSecret(t, env, "GCP_SERVICE_ACCOUNT_KEY", secretName, keyPrivateKey)
}
}
```
The snippet above assumes the existence of several helper functions that likely already exist in this file for the AWS tests (or in shared test utilities):
1. `findDeployment(t, docs, name string) map[string]interface{}` – used in AWS tests to locate a Deployment by name in the rendered YAML.
2. `getDeploymentContainers(t, deployment map[string]interface{}) []map[string]interface{}` – extracts the `.spec.template.spec.containers` list from a Deployment.
3. `getContainerEnv(t, container map[string]interface{}) []map[string]interface{}` – extracts the `.env` list from a container spec.
4. `assertEnvVarFromSecret(t, env []map[string]interface{}, envName, secretName, secretKey string)` – asserts that an env var with the given `envName` exists and references `secretName` / `secretKey` via `valueFrom.secretKeyRef`.
To fully integrate this change and align it with the existing AWS coverage, you should:
1. Reuse the actual helper names already present in the AWS tests instead of `findDeployment`, `getDeploymentContainers`, `getContainerEnv`, and `assertEnvVarFromSecret` if they differ.
2. Update the Deployment identifiers `"server"` and `"importer"` to match the real chart’s Deployment names used in the AWS tests (for example, `"rhtap-operator-server"` or similar).
3. Adjust the secret key names (`"service_account_email"` and `"service_account_key"`) and env var names (`"GCP_SERVICE_ACCOUNT_EMAIL"`, `"GCP_SERVICE_ACCOUNT_KEY"`) to match the actual keys and environment variable names used by the GCP storage configuration in your Helm chart.
4. If the importer Deployment is not rendered for GCP, either:
- Remove the importer-related section, or
- Change the conditional logic to match the chart behavior (e.g. assert it is `nil` and document that only the server uses the credentials).
Once these are aligned with the existing AWS tests and chart conventions, the GCP tests will validate the full wiring from the `CredentialsRequest` through to the Deployments’ consumption of the generated Secret.
</issue_to_address>
### Comment 2
<location path="extensions/README.md" line_range="20" />
<code_context>
+oc get pods -n openshift-cloud-credential-operator
+```
+
+Credential requests checks
+```console
+oc get credentialsrequests -n openshift-cloud-credential-operator
</code_context>
<issue_to_address>
**suggestion (typo):** Rephrase this heading for clearer grammar (singular vs plural).
The phrase "Credential requests checks" is awkward. Consider "CredentialRequest checks" or "Credential request checks" for clearer, grammatically correct wording.
```suggestion
CredentialRequest checks
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+227
to
+236
| func TestHelmRenderGCPCredentialsRequest(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip(skipE2ETest) | ||
| } | ||
|
|
||
| chartPath := getChartPath(t) | ||
| rendered := renderHelmChart(t, chartPath, gcpValues()) | ||
| docs := splitYAMLDocs(rendered) | ||
|
|
||
| cr := findCredentialsRequest(t, docs) |
There was a problem hiding this comment.
suggestion (testing): Add GCP-specific tests for how the generated secret is consumed by Deployments
Right now this only checks that the GCP CredentialsRequest has the expected provider spec. Unlike the AWS tests, it doesn’t verify that the resulting secret is actually used by the Deployments.
To align with the AWS coverage, please add tests that:
- Render the chart with
cloudProvider = gcpand validcloudCredentials.gcp. - Find the relevant
Deploymentmanifests (server/importer) in the rendered output. - Assert that the env vars / volume mounts for storage credentials reference the secret name and keys created by the GCP
CredentialsRequest.
That will validate the full GCP wiring, not just the CR generation.
Suggested implementation:
func TestHelmRenderGCPCredentialsRequest(t *testing.T) {
if testing.Short() {
t.Skip(skipE2ETest)
}
chartPath := getChartPath(t)
rendered := renderHelmChart(t, chartPath, gcpValues())
docs := splitYAMLDocs(rendered)
cr := findCredentialsRequest(t, docs)
require.NotNil(t, cr, "CredentialsRequest should be rendered for GCP")
// Extract the secret name and keys from the GCP CredentialsRequest so we can
// assert that the Deployments are wired up to use it.
secretName, secretKeyClientEmail, secretKeyPrivateKey := extractGCPCredentialsRequestSecretRef(t, cr)
// Verify that the server Deployment consumes the generated credentials secret
serverDeployment := findDeployment(t, docs, "server") // align name with AWS tests / chart
require.NotNil(t, serverDeployment, "server Deployment should be rendered for GCP")
assertDeploymentUsesGCPCredentialsSecret(
t,
serverDeployment,
secretName,
secretKeyClientEmail,
secretKeyPrivateKey,
)
// Verify that the importer Deployment (if rendered for GCP) also consumes the same secret.
// If the importer is not rendered for GCP in this chart, this assertion can be relaxed
// or the test can be updated to match the actual behavior.
importerDeployment := findDeployment(t, docs, "importer") // align name with AWS tests / chart
if importerDeployment != nil {
assertDeploymentUsesGCPCredentialsSecret(
t,
importerDeployment,
secretName,
secretKeyClientEmail,
secretKeyPrivateKey,
)
}
}
// extractGCPCredentialsRequestSecretRef extracts the secret name and the relevant
// key names from the GCP CredentialsRequest. This mirrors the logic used by the
// AWS tests to keep provider coverage aligned.
func extractGCPCredentialsRequestSecretRef(t *testing.T, cr map[string]interface{}) (secretName, keyClientEmail, keyPrivateKey string) {
spec, ok := cr["spec"].(map[string]interface{})
require.True(t, ok, "CredentialsRequest.spec should be a map")
secretRef, ok := spec["secretRef"].(map[string]interface{})
require.True(t, ok, "CredentialsRequest.spec.secretRef should be a map")
name, ok := secretRef["name"].(string)
require.True(t, ok, "CredentialsRequest.spec.secretRef.name should be a string")
// The keys below should match what the GCP provider/ccoctl expects to write
// into the generated Secret for workload identity credentials.
keyClientEmail = "service_account_email"
keyPrivateKey = "service_account_key"
return name, keyClientEmail, keyPrivateKey
}
// assertDeploymentUsesGCPCredentialsSecret validates that the given Deployment
// has env vars / volume mounts wired to the provided credentials Secret.
// This is the GCP equivalent of the AWS wiring tests, ensuring end-to-end coverage
// from CredentialsRequest -> Secret -> Deployment.
func assertDeploymentUsesGCPCredentialsSecret(
t *testing.T,
deployment map[string]interface{},
secretName string,
keyClientEmail string,
keyPrivateKey string,
) {
containers := getDeploymentContainers(t, deployment)
// Depending on how the chart is structured, credentials may be passed as
// env vars, volume mounts, or both. The assertions below assume env vars
// similar to the AWS tests and should be aligned with the actual chart.
for _, c := range containers {
env := getContainerEnv(t, c)
// Assert env vars sourcing from the credentials Secret.
assertEnvVarFromSecret(t, env, "GCP_SERVICE_ACCOUNT_EMAIL", secretName, keyClientEmail)
assertEnvVarFromSecret(t, env, "GCP_SERVICE_ACCOUNT_KEY", secretName, keyPrivateKey)
}
}The snippet above assumes the existence of several helper functions that likely already exist in this file for the AWS tests (or in shared test utilities):
findDeployment(t, docs, name string) map[string]interface{}– used in AWS tests to locate a Deployment by name in the rendered YAML.getDeploymentContainers(t, deployment map[string]interface{}) []map[string]interface{}– extracts the.spec.template.spec.containerslist from a Deployment.getContainerEnv(t, container map[string]interface{}) []map[string]interface{}– extracts the.envlist from a container spec.assertEnvVarFromSecret(t, env []map[string]interface{}, envName, secretName, secretKey string)– asserts that an env var with the givenenvNameexists and referencessecretName/secretKeyviavalueFrom.secretKeyRef.
To fully integrate this change and align it with the existing AWS coverage, you should:
- Reuse the actual helper names already present in the AWS tests instead of
findDeployment,getDeploymentContainers,getContainerEnv, andassertEnvVarFromSecretif they differ. - Update the Deployment identifiers
"server"and"importer"to match the real chart’s Deployment names used in the AWS tests (for example,"rhtap-operator-server"or similar). - Adjust the secret key names (
"service_account_email"and"service_account_key") and env var names ("GCP_SERVICE_ACCOUNT_EMAIL","GCP_SERVICE_ACCOUNT_KEY") to match the actual keys and environment variable names used by the GCP storage configuration in your Helm chart. - If the importer Deployment is not rendered for GCP, either:
- Remove the importer-related section, or
- Change the conditional logic to match the chart behavior (e.g. assert it is
niland document that only the server uses the credentials).
Once these are aligned with the existing AWS tests and chart conventions, the GCP tests will validate the full wiring from the CredentialsRequest through to the Deployments’ consumption of the generated Secret.
| oc get pods -n openshift-cloud-credential-operator | ||
| ``` | ||
|
|
||
| Credential requests checks |
There was a problem hiding this comment.
suggestion (typo): Rephrase this heading for clearer grammar (singular vs plural).
The phrase "Credential requests checks" is awkward. Consider "CredentialRequest checks" or "Credential request checks" for clearer, grammatically correct wording.
Suggested change
| Credential requests checks | |
| CredentialRequest checks |
Signed-off-by: desmax74 <mdessi@redhat.com>
Signed-off-by: desmax74 <mdessi@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/integration/cco_rbac_test.go" line_range="111" />
<code_context>
+ assert.Equal(t, "cco-credentialsrequest-access", binding.RoleRef.Name, "roleRef name should match the CCO ClusterRole")
+}
+
+func TestCCOClusterRoleBindingSubjects(t *testing.T) {
+ bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Make subject assertions resilient to ordering by matching the desired ServiceAccount rather than indexing the first subject.
This test currently assumes the desired ServiceAccount is always at `binding.Subjects[0]`, so it will break if subjects are reordered or new ones are added.
Instead, iterate over `binding.Subjects` and assert that at least one subject has `Kind == "ServiceAccount"` and `Name == "rhtpa-operator-controller-manager"` (and the expected namespace, if applicable). This keeps the test focused on verifying that the operator’s ServiceAccount is bound without relying on subject ordering.
Suggested implementation:
```golang
func TestCCOClusterRoleBindingSubjects(t *testing.T) {
bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
data, err := os.ReadFile(bindingPath)
require.NoError(t, err, "should be able to read CCO ClusterRoleBinding manifest")
var binding rbacv1.ClusterRoleBinding
err = yaml.Unmarshal(data, &binding)
require.NoError(t, err, "should be able to unmarshal CCO ClusterRoleBinding")
foundOperatorSA := false
for _, subject := range binding.Subjects {
if subject.Kind == "ServiceAccount" &&
subject.Name == "rhtpa-operator-controller-manager" {
foundOperatorSA = true
break
}
}
assert.True(t, foundOperatorSA,
"ClusterRoleBinding should bind the operator ServiceAccount 'rhtpa-operator-controller-manager'")
```
To compile and run this test, ensure the following:
1. The necessary imports are present at the top of `test/integration/cco_rbac_test.go`:
- `os` from the standard library
- `k8s.io/api/rbac/v1` as `rbacv1`
- `sigs.k8s.io/yaml` (or the YAML package you use elsewhere in this file)
- `github.com/stretchr/testify/assert` and `github.com/stretchr/testify/require` if not already imported.
2. If the file already has a helper for loading `ClusterRoleBinding` YAML (e.g., `loadClusterRoleBinding(t, path)`), you may want to replace the explicit `os.ReadFile`/`yaml.Unmarshal` block with a call to that helper for consistency.
3. If the subject’s namespace should also be asserted, extend the `if` condition inside the loop to include `subject.Namespace == "<expected-namespace>"` and update the failure message accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert.Equal(t, "cco-credentialsrequest-access", binding.RoleRef.Name, "roleRef name should match the CCO ClusterRole") | ||
| } | ||
|
|
||
| func TestCCOClusterRoleBindingSubjects(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Make subject assertions resilient to ordering by matching the desired ServiceAccount rather than indexing the first subject.
This test currently assumes the desired ServiceAccount is always at binding.Subjects[0], so it will break if subjects are reordered or new ones are added.
Instead, iterate over binding.Subjects and assert that at least one subject has Kind == "ServiceAccount" and Name == "rhtpa-operator-controller-manager" (and the expected namespace, if applicable). This keeps the test focused on verifying that the operator’s ServiceAccount is bound without relying on subject ordering.
Suggested implementation:
func TestCCOClusterRoleBindingSubjects(t *testing.T) {
bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
data, err := os.ReadFile(bindingPath)
require.NoError(t, err, "should be able to read CCO ClusterRoleBinding manifest")
var binding rbacv1.ClusterRoleBinding
err = yaml.Unmarshal(data, &binding)
require.NoError(t, err, "should be able to unmarshal CCO ClusterRoleBinding")
foundOperatorSA := false
for _, subject := range binding.Subjects {
if subject.Kind == "ServiceAccount" &&
subject.Name == "rhtpa-operator-controller-manager" {
foundOperatorSA = true
break
}
}
assert.True(t, foundOperatorSA,
"ClusterRoleBinding should bind the operator ServiceAccount 'rhtpa-operator-controller-manager'")To compile and run this test, ensure the following:
- The necessary imports are present at the top of
test/integration/cco_rbac_test.go:osfrom the standard libraryk8s.io/api/rbac/v1asrbacv1sigs.k8s.io/yaml(or the YAML package you use elsewhere in this file)github.com/stretchr/testify/assertandgithub.com/stretchr/testify/requireif not already imported.
- If the file already has a helper for loading
ClusterRoleBindingYAML (e.g.,loadClusterRoleBinding(t, path)), you may want to replace the explicitos.ReadFile/yaml.Unmarshalblock with a call to that helper for consistency. - If the subject’s namespace should also be asserted, extend the
ifcondition inside the loop to includesubject.Namespace == "<expected-namespace>"and update the failure message accordingly.
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.
https://redhat.atlassian.net/browse/TC-4306
Assisted-by: Claude: Opus 4.6
Summary by Sourcery
Introduce cloud-provider-specific credential requests and wire them into the trusted profile analyzer Helm chart.
New Features:
Summary by Sourcery
Integrate Cloud Credentials Operator support for cloud-provider-specific S3 credentials in the trusted profile analyzer operator and Helm chart.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Integrate OpenShift Cloud Credential Operator support into the trusted profile analyzer operator and Helm chart to provision cloud-provider-specific credentials and enable AWS token-based auth and optional RDS IAM authentication.
New Features:
Enhancements:
Tests: