Tokenize Credential Request GCP#856
Open
desmax74 wants to merge 4 commits into
Open
Conversation
Signed-off-by: desmax74 <mdessi@redhat.com>
…/manual)
Enable Cloud Credential Operator support for AWS with all standard OpenShift
CCO modes. Manual mode (STS) auto-configures projected SA token volumes and
AWS SDK credential chain env vars. The ccoMode field is fixed from broken {}
default to a proper string enum.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend CCO integration to GCP: add serviceAccountEmail to schemas and CredentialsRequest, GCP-specific GOOGLE_APPLICATION_CREDENTIALS env var for manual mode, and enable token-auth-gcp OLM annotation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's GuideAdds Cloud Credentials Operator (CCO) integration for AWS and GCP to the RHTPA Helm chart/operator, including schema and values support, CredentialRequest templating, runtime wiring for S3 credentials across CCO modes, RBAC to manage CredentialsRequest objects, OpenShift feature labels for token-based auth, and comprehensive e2e/integration tests plus fixtures for the new behavior. 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 1 issue, and left some high level feedback:
- In
_storage.tplthe CCO-backed secret keys are hard-coded toaws_access_key_id/aws_secret_access_keyfor all non-manualcloudProvidervalues; for GCP mint/passthrough this is unlikely to match the secret layout emitted by the GCP provider and should either be specialized per provider or disabled for GCP. - In
credentialrequest.yamlmanual mode does not guard against missingstsIAMRoleARN/serviceAccountEmail, which will render an invalidCredentialsRequest; consider using Helmrequired(or similar) to fail the render when these fields are omitted in manual mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_storage.tpl` the CCO-backed secret keys are hard-coded to `aws_access_key_id`/`aws_secret_access_key` for all non-manual `cloudProvider` values; for GCP mint/passthrough this is unlikely to match the secret layout emitted by the GCP provider and should either be specialized per provider or disabled for GCP.
- In `credentialrequest.yaml` manual mode does not guard against missing `stsIAMRoleARN`/`serviceAccountEmail`, which will render an invalid `CredentialsRequest`; consider using Helm `required` (or similar) to fail the render when these fields are omitted in manual mode.
## Individual Comments
### Comment 1
<location path="test/integration/cco_rbac_test.go" line_range="111-127" />
<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")
+
+ data, err := os.ReadFile(bindingPath)
+ require.NoError(t, err, "CCO ClusterRoleBinding should be readable")
+
+ var binding ClusterRoleBinding
+ err = yaml.Unmarshal(data, &binding)
+ require.NoError(t, err, "CCO ClusterRoleBinding should be valid YAML")
+
+ require.NotEmpty(t, binding.Subjects, "ClusterRoleBinding should have at least one subject")
+
+ assert.Equal(t, "ServiceAccount", binding.Subjects[0].Kind,
+ "subject should be a ServiceAccount")
+ assert.Equal(t, "rhtpa-operator-controller-manager", binding.Subjects[0].Name,
+ "subject should reference the operator's service account")
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid relying on the first subject in the ClusterRoleBinding; search for the expected service account explicitly
This test currently assumes the operator service account is always `binding.Subjects[0]`. If another subject is added before it, the test will fail despite the binding being correct. Instead, iterate over `binding.Subjects` and assert that at least one subject has `Kind == "ServiceAccount"` and `Name == "rhtpa-operator-controller-manager"`.
```suggestion
func TestCCOClusterRoleBindingSubjects(t *testing.T) {
bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml")
data, err := os.ReadFile(bindingPath)
require.NoError(t, err, "CCO ClusterRoleBinding should be readable")
var binding ClusterRoleBinding
err = yaml.Unmarshal(data, &binding)
require.NoError(t, err, "CCO ClusterRoleBinding should be valid YAML")
require.NotEmpty(t, binding.Subjects, "ClusterRoleBinding should have at least one subject")
found := false
for _, subject := range binding.Subjects {
if subject.Kind == "ServiceAccount" && subject.Name == "rhtpa-operator-controller-manager" {
found = true
break
}
}
assert.True(t, found, "ClusterRoleBinding should contain a ServiceAccount subject named rhtpa-operator-controller-manager")
}
```
</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
+111
to
+127
| func TestCCOClusterRoleBindingSubjects(t *testing.T) { | ||
| bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml") | ||
|
|
||
| data, err := os.ReadFile(bindingPath) | ||
| require.NoError(t, err, "CCO ClusterRoleBinding should be readable") | ||
|
|
||
| var binding ClusterRoleBinding | ||
| err = yaml.Unmarshal(data, &binding) | ||
| require.NoError(t, err, "CCO ClusterRoleBinding should be valid YAML") | ||
|
|
||
| require.NotEmpty(t, binding.Subjects, "ClusterRoleBinding should have at least one subject") | ||
|
|
||
| assert.Equal(t, "ServiceAccount", binding.Subjects[0].Kind, | ||
| "subject should be a ServiceAccount") | ||
| assert.Equal(t, "rhtpa-operator-controller-manager", binding.Subjects[0].Name, | ||
| "subject should reference the operator's service account") | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Avoid relying on the first subject in the ClusterRoleBinding; search for the expected service account explicitly
This test currently assumes the operator service account is always binding.Subjects[0]. If another subject is added before it, the test will fail despite the binding being correct. Instead, iterate over binding.Subjects and assert that at least one subject has Kind == "ServiceAccount" and Name == "rhtpa-operator-controller-manager".
Suggested change
| func TestCCOClusterRoleBindingSubjects(t *testing.T) { | |
| bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml") | |
| data, err := os.ReadFile(bindingPath) | |
| require.NoError(t, err, "CCO ClusterRoleBinding should be readable") | |
| var binding ClusterRoleBinding | |
| err = yaml.Unmarshal(data, &binding) | |
| require.NoError(t, err, "CCO ClusterRoleBinding should be valid YAML") | |
| require.NotEmpty(t, binding.Subjects, "ClusterRoleBinding should have at least one subject") | |
| assert.Equal(t, "ServiceAccount", binding.Subjects[0].Kind, | |
| "subject should be a ServiceAccount") | |
| assert.Equal(t, "rhtpa-operator-controller-manager", binding.Subjects[0].Name, | |
| "subject should reference the operator's service account") | |
| } | |
| func TestCCOClusterRoleBindingSubjects(t *testing.T) { | |
| bindingPath := filepath.Join("config", "rbac", "clusterrolebinding_cco.yaml") | |
| data, err := os.ReadFile(bindingPath) | |
| require.NoError(t, err, "CCO ClusterRoleBinding should be readable") | |
| var binding ClusterRoleBinding | |
| err = yaml.Unmarshal(data, &binding) | |
| require.NoError(t, err, "CCO ClusterRoleBinding should be valid YAML") | |
| require.NotEmpty(t, binding.Subjects, "ClusterRoleBinding should have at least one subject") | |
| found := false | |
| for _, subject := range binding.Subjects { | |
| if subject.Kind == "ServiceAccount" && subject.Name == "rhtpa-operator-controller-manager" { | |
| found = true | |
| break | |
| } | |
| } | |
| assert.True(t, found, "ClusterRoleBinding should contain a ServiceAccount subject named rhtpa-operator-controller-manager") | |
| } |
Signed-off-by: desmax74 <mdessi@redhat.com>
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-4308
Summary by Sourcery
Integrate OpenShift Cloud Credentials Operator support for managing cloud storage credentials across AWS and GCP, including manual STS/WIF modes, and advertise token-based auth support in the operator bundle.
New Features:
Enhancements:
Documentation:
Tests: