Skip to content

Fix CSI credentials naming mechanism #4619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 14, 2025

Conversation

psalajova
Copy link
Contributor

@psalajova psalajova commented Jul 3, 2025

As of now, the new CSI mutli-stage step credential system creates one SecretProviderClass (SPC) per credential, mounted to the test pod in as a CSI volumeMount. Since we need to shard the existing credential secrets, like this:

credentials:
  - collection: team1
    name: name
    mount_path: /var/run
  - collection: team1
    name: diff-name
    mount_path: /var/run

the existing system resulted in Kubernetes's Invalid value: "<mount_path>": must be unique errors (see e.g. this test run), because even if this results in a volumeMount with different name, the mount_paths cannot be the same.
Also, because the credential's name can be quite long, we were hitting character limit exceeded errors.

Solution:

  • Credentials with the same (collection, mountPath) are now grouped into a single SPC
  • SPC names are based on a hash of collection, mount path, and credential names to prevent overwrites when different steps use the same collection/mount combination
  • Truncation logic -- to stay within Kubernetes' 63-character DNS label limit -- was used for SPC and CSI volumes naming
  • For sidecar censoring, all credentials are created as individual SPCs, with different names and mount paths

@openshift-ci openshift-ci bot requested review from hector-vido and jmguzik July 3, 2025 14:35
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2025
@psalajova psalajova changed the title Fix spc according to naming conventions [WIP] Fix spc according to naming conventions Jul 4, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2025
@psalajova psalajova changed the title [WIP] Fix spc according to naming conventions Fix CSI credentials naming mechanism Jul 10, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2025
@psalajova psalajova force-pushed the fix-spc-naming branch 3 times, most recently from 6c14c40 to 9358bb1 Compare July 10, 2025 13:45
for _, step := range append(s.pre, append(s.test, s.post...)...) {
for _, credential := range step.Credentials {
name := fmt.Sprintf("%s-%s-spc", s.jobSpec.Namespace(), credential.Name)
if _, exists := toCreate[name]; exists {
if seenCredentials[credential.Name] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that this check happens in several places. If we forbid two credentials with the same name, wouldn't it be appropriate to add this policy to the code that validates a ci-operator configuration? pkg/validation/config.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is for the scenario when there's more steps in a workflow which use the same credentials. E.g. this workflow -- has steps stackrox-stackrox-begin and stackrox-stackrox-end, and both have the stackrox-automation-flavors credential in their definitions.

@psalajova
Copy link
Contributor Author

/test e2e

@danilo-gemoli
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2025
@openshift-ci-robot
Copy link
Contributor

/test remaining-required

Copy link
Contributor

openshift-ci bot commented Jul 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danilo-gemoli, psalajova

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [danilo-gemoli,psalajova]

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

Copy link
Contributor

openshift-ci bot commented Jul 14, 2025

@psalajova: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/breaking-changes 2bc827a link false /test breaking-changes

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 2a5b667 and 2 for PR HEAD 2bc827a in total

@openshift-merge-bot openshift-merge-bot bot merged commit bb359ba into openshift:main Jul 14, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants