Skip to content

[HotFix] Ensure cryp4gh keys are loaded on startup. #1663

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbygdell
Copy link
Collaborator

@jbygdell jbygdell commented May 12, 2025

Related issue(s) and PR(s)

This PR closes #1597
This PR closes #1662.

Description

This PR fixes a bug in the code where Ingest and Verify would happily start even when no crypt4gh keys have been configured.

How to test

Remove the c4gh.privateKeys block from the config file and run make sda-s3-up

Extra info

Chart tests will fail since the charts are not updated to handle multiple keys yet. (WIP to be completed once this is merged.)

@jbygdell jbygdell force-pushed the hotfix/c4gh-key-config branch 2 times, most recently from 5f9be95 to 507e90c Compare May 12, 2025 08:17
@jbygdell jbygdell requested a review from a team May 12, 2025 08:47
@jbygdell jbygdell self-assigned this May 12, 2025
Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Looks good to me except for the comment about the error message handling. In addition, the deployment of helm chart (ingest and verify services) is failing because of missing c4gh keys.

@jbygdell jbygdell force-pushed the hotfix/c4gh-key-config branch from 507e90c to 24eae14 Compare May 12, 2025 14:10
@jbygdell
Copy link
Collaborator Author

In addition, the deployment of helm chart (ingest and verify services) is failing because of missing c4gh keys.

I know, it is in the PR description..
But it is a catch 22 thing, Merging this PR will make fixing the charts much easier since these two services will fail if not configured properly.

@jbygdell jbygdell force-pushed the hotfix/c4gh-key-config branch from 24eae14 to 69f7594 Compare May 13, 2025 08:01
Comment on lines +738 to +743
func (ts *TestSuite) TestDetectMisingC4GHKeys() {
viper.Set("c4gh.privateKeys", "")
privateKeys, err := config.GetC4GHprivateKeys()
assert.NoError(ts.T(), err)
assert.Equal(ts.T(), 0, len(privateKeys))
}
Copy link
Contributor

@aaperis aaperis May 15, 2025

Choose a reason for hiding this comment

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

What does this part check? Does it check that config.GetC4GHprivateKeys translates an empty string to an empty array?
I guess to ensure that the fix won't break the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Applications start without configured c4gh keys [Ingest] Should fail to start if no c4gh key is configured
4 participants