Skip to content

CLOUDP-266544: Support local resource credentials #1782

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 7 commits into from
Sep 2, 2024

Conversation

josvazg
Copy link
Collaborator

@josvazg josvazg commented Aug 22, 2024

  • Add common infrastructure for adding local per CRD credentials at pkg/api/credentials.go.
  • Use the new infra from AtlasDatabaseUser's CRD and Controller.
  • Solve whether the credentials are taken from the current resource, the project or render nil.

Testing:

  • Unit tests on new ComputeSecret.
  • Conversion test fix: credentials must be ignored on comparisons between the Atlas and Kubernetes Spec.
  • Users e2e test: Second user takes locally defined credentials. Also if local credentials do not exist, reconciliation fails as expected.

All Submissions:

  • Have you signed our CLA?

@josvazg josvazg marked this pull request as draft August 22, 2024 17:13
@josvazg josvazg added the cloud-tests Run expensive Cloud Tests: Integration & E2E label Aug 23, 2024
Copy link
Contributor

github-actions bot commented Aug 23, 2024

@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 2bda370 to 3db3cde Compare August 23, 2024 07:44
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 3db3cde to 15ec9d1 Compare August 23, 2024 10:07
@josvazg josvazg changed the base branch from CLOUDP-266544/refactor-internal-deployments to main August 23, 2024 11:28
@josvazg josvazg marked this pull request as ready for review August 23, 2024 11:28
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 15ec9d1 to 8b8d117 Compare August 23, 2024 11:50
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 8b8d117 to 2d1bd44 Compare August 23, 2024 12:04
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 2d1bd44 to 514af19 Compare August 23, 2024 12:33
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch 2 times, most recently from 9f35b5a to 7f66bed Compare August 26, 2024 13:57
@josvazg josvazg requested review from s-urbaniak and roothorp August 27, 2024 06:52
Signed-off-by: jose.vazquez <[email protected]>
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from ee6aae4 to dde859c Compare August 27, 2024 14:46
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from dde859c to 02d62bb Compare August 27, 2024 14:52
Signed-off-by: jose.vazquez <[email protected]>
@josvazg josvazg force-pushed the CLOUDP-266544/per-resource-creds branch from 02d62bb to 9fa0baa Compare August 27, 2024 15:03
@josvazg
Copy link
Collaborator Author

josvazg commented Aug 28, 2024

One nit about the global secret fallback handling. The logic is quite confusing to the reader. Why not make it explicit, where:

type Provider interface {
...
	GlobalSecret() client.ObjectKey
}

and then just invoke:

credentialsSecret, err := customresource.ComputeSecret(r.AtlasProvider.GlobalSecret(), project, databaseUser)
...

where the fallback logic is explicit in ComputeSecret:

func ComputeSecret(globalSecret client.ObjectKey, project *akov2.AtlasProject, resource api.ResourceWithCredentials) (*client.ObjectKey, error) {
	if globalSecret.Name != "" {
		return &globalSecret, nil
	}
	return resolveConnectionSecret(project, resource)
}

There is less indirection and makes the fallback explicit.

In the end I removed the global fallback from this part of the resolution. That will still work as before.

The only loss is we do not have precise errors when there is no global fallback, but that scenario was problematic to open up so soon anyway. We should decide if the global fallback will be optional separately.

Comment on lines 279 to 280
ATLAS_LOCAL_PUBLIC_KEY: ${{ secrets.ATLAS_LOCAL_PUBLIC_KEY }}
ATLAS_LOCAL_PRIVATE_KEY: ${{ secrets.ATLAS_LOCAL_PRIVATE_KEY }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@josvazg Why do we need to duplicate API credentials?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, we do not need this credentials.

I can simplify, but then the gov test issues will get me, so I won't be able to merge today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@s-urbaniak
Copy link
Collaborator

rerunning e2e gov, let's evaluate if this is still a blocker.

@josvazg
Copy link
Collaborator Author

josvazg commented Sep 2, 2024

Our nightly test suite saw recovery today at midnight on the gov tests:
cloud-tests / test-e2e-gov / E2E Gov tests

@josvazg
Copy link
Collaborator Author

josvazg commented Sep 2, 2024

@s-urbaniak all green again, I closed CLOUDP-270948

@josvazg josvazg merged commit 8dbdcbf into main Sep 2, 2024
57 checks passed
josvazg added a commit that referenced this pull request Sep 5, 2024
s-urbaniak pushed a commit that referenced this pull request Sep 5, 2024
* Revert "CLOUDP-266544: Support local resource credentials (#1782)"

This reverts commit 8dbdcbf.

* Use project creds again
@roothorp roothorp deleted the CLOUDP-266544/per-resource-creds branch September 26, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud-tests Run expensive Cloud Tests: Integration & E2E
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants