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

[WIP] Refactor Provider Credentials #2195

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

grolu
Copy link
Contributor

@grolu grolu commented Nov 21, 2024

This PR refactors the backend /cloudprovidersecrets API

  • Removed old endpoint that returned custom dashboard secret resource
  • Added new /cloudprovidercredentials endpoint that returns list of credentials (sets) that contain related SecretBinding, Secret (if own) and resolved Quota resources
  • Renamed secret store to credential store

This PR is also a preparation to support new CredentialsBinding resource

  • Started renaming secret to credential (store)
  • Work with SecretBinding as the leading resource, access referenced secret only when we need to access the actual secret data
  • In a follow-up PR we can then also read CredentialsBindings in the backend and return with the list. The dashboard can then work with CredentialsBindings and SecretBindings in the frontend as they are more or less interchangeable. Only the reference to the Secret / WorkloadIdentity data is different so we need to add some kind of abstraction / transformation there.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Tests need to be adapted

Release note:


@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Nov 21, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 21, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 21, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 21, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 21, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 21, 2024
Highlight secret row instead of open secret dialog
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 21, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 21, 2024
show secret data in edit mode
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 22, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 22, 2024
Comment on lines +26 to +27
const credentialsList = secretBindings.map(secretBinding => {
const secret = referencedSecrets.find(secret => secret.metadata.name === secretBinding.secretRef.name)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const credentialsList = secretBindings.map(secretBinding => {
const secret = referencedSecrets.find(secret => secret.metadata.name === secretBinding.secretRef.name)
const secretMap = new Map(referencedSecrets.map(secret => [secret.metadata.name, secret]))
const credentialsList = secretBindings.map(secretBinding => {
const secret = secretMap.get(secretBinding.secretRef.name)

return [
{
label: 'Domain Name',
value: atob(secretData.domainName),
Copy link
Member

Choose a reason for hiding this comment

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

Use Base64.decode(data) instead of atob

const gardenerExtensionStore = useGardenerExtensionStore()
const cloudProfileStore = useCloudProfileStore()

const credentialResourcesList = ref(null)
Copy link
Member

Choose a reason for hiding this comment

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

Rename to cloudProviderCredentials or cloudProviderCredentialList. I would never use Resource or Resources as part of the name. In kubernetes the term object and items is used. The term resource is use in http in the client - server context form server entities.


const secretBindingList = computed(() => {
return map(credentialResourcesList.value, ({ secretBinding, secret, quotas }) => {
Object.defineProperty(secretBinding, 'secretResource', {
Copy link
Member

Choose a reason for hiding this comment

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

rename to secret

configurable: false,
enumerable: false,
})
Object.defineProperty(secretBinding, 'quotaResources', {
Copy link
Member

Choose a reason for hiding this comment

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

rename to quotas

Comment on lines +67 to +69
writable: false,
configurable: false,
enumerable: false,
Copy link
Member

Choose a reason for hiding this comment

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

This is the default. We have not specified these default value in the past

@@ -455,14 +455,14 @@ export function shortRandomString (length) {
}

export function selfTerminationDaysForSecret (secret) {
const clusterLifetimeDays = function (quotas, scope) {
return get(find(quotas, scope), ['spec', 'clusterLifetimeDays'])
const clusterLifetimeDays = function (quotaResources, scope) {
Copy link
Member

Choose a reason for hiding this comment

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

Always use quota


exports.create = async function ({ user, body }) {
const client = user.client
const { coordinate: { namespace, name }, credential: { poviderType, secretData } } = body
Copy link
Member

Choose a reason for hiding this comment

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

I would not use the property coordinate. Use the relevant properties flat. Pass the params and not body.

const { namespace, name, poviderType, secretData } = params

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants