Skip to content
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

improvement(memory-edc): remove EDC_VAULT_SECRETS rewrite #1040

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

KilianHaag
Copy link
Contributor

@KilianHaag KilianHaag commented Feb 9, 2024

WHAT

Removes the rewrite of ENV variable "SECRETS" to edc.vault.secrets and instead uses the EDC_VAULT_SECRETS directly in the memory chart and Dockerfile.

WHY

With the current setup, this is the only ENV variable, that is rewritten. While checking the documentation of the edc components, this rewrite is not apparent. In short: I don't see any reason for it, except potential confusion.

FURTHER NOTES

Also fixes the documentation regarding the seeding of secrets with semicolon separated k:v list

Closes #1039

Copy link

sonarcloud bot commented Feb 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Feb 14, 2024

I see three slight problems with this PR, not even looking at the code:

  1. Insufficient PR description. Please provide proper PR description, i.e. what you changed, and why you changed it and why in this way. It just makes reviewers' lives much easier.
  2. There's a failing test. If its a flaky test, pls raise an issue, otherwise pls fix the test.
  3. Generally, every code change should be accompanied by test code. Granted, this is not trivial to do here, and since we're not dealing with production code I'm fine with it. But then the PR desc should contain a note to that effect.

@KilianHaag
Copy link
Contributor Author

I created #1058 for the potentially flaky test and changed the description.

In this case I can not think of a test, as the changes should not alter the existing behavior.
But the deployment-test / test-in-memory should be covering this, as I expect the deployment to fail, if secrets are missing in the (memory-)vault.

@wolf4ood
Copy link
Contributor

@KilianHaag I've restarted the test meanwhile

@KilianHaag
Copy link
Contributor Author

@wolf4ood Thanks for triggering them again.
Seems like all the tests for this PR are successful now, but (sorry for the stupid question) what are these 2 runs under my name:
image

I don't understand how they got triggered.

@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Feb 18, 2024

I expect the deployment to fail, if secrets are missing in the (memory-)vault.

@KilianHaag it will not, as the deployment test only tests that the runtime can be deployed and booted up. Missing secrets could potentially go unnoticed.

@@ -252,7 +252,7 @@ spec:
###########

# see extension https://github.com/eclipse-tractusx/tractusx-edc/tree/develop/edc-extensions/hashicorp-vault
Copy link
Contributor

Choose a reason for hiding this comment

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

this URL is not valid anymore.

@paullatzelsperger paullatzelsperger merged commit eb250ba into eclipse-tractusx:main Feb 18, 2024
26 checks passed
@KilianHaag
Copy link
Contributor Author

@paullatzelsperger I might be mistaken, but when I explicitly omit the '--set vault.secrets="client-secret:$(cat client.secret)"' from the test deployment, the result will be a CrashLoopBackOff:
Caused by: java.lang.NullPointerException: Client secret could not be retrieved at java.base/java.util.Objects.requireNonNull(Unknown Source) at org.eclipse.tractusx.edc.iam.ssi.miw.SsiMiwOauth2ClientExtension.createConfiguration(SsiMiwOauth2ClientExtension.java:75) at org.eclipse.tractusx.edc.iam.ssi.miw.SsiMiwOauth2ClientExtension.oauth2Client(SsiMiwOauth2ClientExtension.java:67) ... 23 more

So what I meant is: If my change would have made all secret(s) unavailable, the deployment should fail, as the client-secret is retrieved during startup.

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

Successfully merging this pull request may close these issues.

improvement: Remove rewriting of ENV variable in memory Dockerfile and chart
3 participants