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

[GEP-17] Add owner checks and taking of final snapshots #383

Merged
merged 10 commits into from
Oct 21, 2021

Conversation

stoyanr
Copy link
Contributor

@stoyanr stoyanr commented Sep 30, 2021

What this PR does / why we need it:
Adds functionality to check if the owner domain name resolves to the specified owner ID and if not, take a final full snapshot and disable the cluster, as described in GEP-17.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

The backup-restore sidecar will now check if the owner domain name resolves to the specified owner ID and if not, take a final full snapshot and disable the cluster.

@stoyanr stoyanr requested a review from a team as a code owner September 30, 2021 16:21
@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 Sep 30, 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 Sep 30, 2021
@stoyanr stoyanr marked this pull request as draft September 30, 2021 16:21
@gardener-robot-ci-1 gardener-robot-ci-1 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 Sep 30, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Sep 30, 2021

Still in Draft because it depends on #369 but I would really appreciate early feedback if you have time.
/cc @plkokanov @kris94 @timuthy

@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 Oct 1, 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 Oct 1, 2021
@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 Oct 1, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Oct 1, 2021

Rebased on latest master and removed Draft.

@stoyanr stoyanr marked this pull request as ready for review October 1, 2021 14:00
@plkokanov
Copy link
Contributor

/assign

@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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 1, 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 Oct 7, 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 Oct 7, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Oct 7, 2021

@ishan16696 Addressed your latest feedback with a new commit.

Also added a new commit to make sure the owner checks are disabled (and not simply always succeeding), if the owner name or id are not specified.

Copy link
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Even though I haven't completely reviewed this PR, I already want to address one topic that has drawn my attention while reviewing gardener/etcd-druid#239.

pkg/server/init.go Outdated Show resolved Hide resolved
@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 Oct 15, 2021
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@stoyanr Thanks for the well-written PR! I have a few doubts/suggestions. Can you please address them? Thanks

pkg/snapshot/snapshotter/snapshotter.go Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter.go Show resolved Hide resolved
pkg/snapshot/copier/copier.go Show resolved Hide resolved
pkg/server/backuprestoreserver.go Show resolved Hide resolved
pkg/server/backuprestoreserver.go Outdated Show resolved Hide resolved
pkg/server/backuprestoreserver.go Show resolved Hide resolved
pkg/server/backuprestoreserver.go Show resolved Hide resolved
@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 Oct 19, 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 Oct 19, 2021
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@stoyanr Thanks for addressing my comments. /lgtm

Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Thanks for adapting the PR. Just one last thing I noticed when I skimmed over the changes.

go.mod Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 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 Oct 21, 2021
Copy link
Member

@timuthy timuthy 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/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Oct 21, 2021
@timuthy timuthy merged commit 035593f into gardener:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge/squash Should be merged via 'Squash and merge' 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
Development

Successfully merging this pull request may close these issues.

9 participants