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

Support DNS Provider Management #1026

Merged
merged 58 commits into from
Aug 2, 2021
Merged

Support DNS Provider Management #1026

merged 58 commits into from
Aug 2, 2021

Conversation

grolu
Copy link
Contributor

@grolu grolu commented May 25, 2021

What this PR does / why we need it:
This PR adds DNS Provider Management to the Gardener Dashboard.
This enables the end users to add and manage their DNS Secrets via the Dashboard. Furthermore cluster DNS settings can be configured during cluster creation as well as afterwards, if possible.

Capture

Screenshot 2021-06-16 at 10 30 01

Screenshot 2021-06-16 at 09 58 14

Screenshot 2021-06-16 at 09 53 40

Screenshot 2021-06-16 at 09 53 22

Which issue(s) this PR fixes:
Fixes #769 Fixes #616 Fixes #406

Special notes for your reviewer:
@mandelsoft The secrets page will get a redesign soon.
@holgerkoser Please give feedback to the store based eventing approach I introduced with this PR for the new DNS components (@/components/ShootDns/)

Release note:

External DNS Provider Support
- Add and manage DNS Provider Secrets
- Configure Shoot DNS Providers

@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 May 25, 2021
@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 May 25, 2021
@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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@@ -73,7 +73,8 @@ export default {
}
},
reset () {
this.setDnsConfiguration(this.shootSpec.dns)
const noCustomDomain = !this.isCustomShootDomain
Copy link
Member

Choose a reason for hiding this comment

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

do not understand why we need this (see comment above)

@@ -106,6 +106,10 @@ export default {
createMode: {
type: Boolean,
default: false
},
noCustomDomain: {
Copy link
Member

Choose a reason for hiding this comment

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

do not understand why we need this (see comment above)

Comment on lines 171 to 172
primaryProviderId,
noCustomDomain
Copy link
Member

Choose a reason for hiding this comment

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

!primaryProviderId === noCustomDomain

state.dnsDomain = domain
state.dnsProviders = keyBy(providers, 'id')
state.dnsProviderIds = map(providers, 'id')
state.dnsPrimaryProviderId = primaryProviderId
state.dnsPrimaryProviderValid = !state.dnsDomain || !!state.dnsPrimaryProviderId
state.dnsPrimaryProviderValid = !state.dnsDomain || !!state.dnsPrimaryProviderId || noCustomDomain
Copy link
Member

Choose a reason for hiding this comment

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

since !primaryProviderId === noCustomDomain this means !!state.dnsPrimaryProviderId || noCustomDomain is always true?

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 add a property:

state.dnsPrimaryProviderRequired =  !!primaryProviderId

but do not quite understand the reasoning behind it

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 is not the case. This statement is only true for already created shoots. This is the whole purpose of my change. For created shoots without a custom domain, the domain field is populated by the custom domain, while dnsPrimaryProviderId undefined. However, the information if the shoot is already created is not avaialble in the store (or am i wrong?). Of course, we could also store this information but I thought it would be easier to understand if we provide the actual information in a central place (mixin).
Same applies for your comment above:

I absolutely do not understand why we need to introduce this component parameter. I though the goal of my last my last PR was to get rid of these kind of component parameters. All the necessary information is already available on the store.

We could get the information by evaluating both the dnsPrimaryProviderId store state and the created prop but I wanted to keep the logic in a central place. I did not put it inside the store, as we also need the computed prop for the details card which has nothing to do with the store... ?
Maybe we need to discuss this online

@gardener-robot
Copy link

@grolu You need rebase this pull request with latest master branch. Please check.

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Jul 30, 2021
@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 Jul 30, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 Jul 30, 2021
…enh/dns-secrets

# Conflicts:
#	frontend/src/components/ShootDns/ManageDns.vue
#	frontend/src/store/modules/shootStaging.js
@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 Jul 30, 2021
@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 Jul 30, 2021
@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 Jul 30, 2021
@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 Jul 30, 2021
@gardener-robot
Copy link

@mandelsoft You have pull request review open invite, please check

Comment on lines 25 to 26
@input="onUpdateSecret"
@update:valid="onSecretValid"></select-secret>
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
@input="onUpdateSecret"
@update:valid="onSecretValid"></select-secret>
@input="onUpdateSecret"
:valid="secretValid"
@update:valid="onSecretValid"></select-secret>

@gardener-robot-ci-3 gardener-robot-ci-3 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 Aug 2, 2021
# Conflicts:
#	frontend/src/components/ShootWorkers/MachineImage.vue
@gardener-robot gardener-robot removed the needs/rebase Needs git rebase label Aug 2, 2021
@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 Aug 2, 2021
@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 Aug 2, 2021
Copy link
Member

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Aug 2, 2021
@grolu grolu merged commit 87f78e5 into master Aug 2, 2021
@grolu grolu deleted the enh/dns-secrets branch August 2, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Nobody worked on this for 12 months (final aging stage) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
7 participants