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

Add NAT AZ Cordon tool #56

Merged
merged 6 commits into from
Sep 15, 2021
Merged

Conversation

eytan-avisror
Copy link
Collaborator

@eytan-avisror eytan-avisror commented Sep 9, 2021

No description provided.

Eytan Avisror added 2 commits September 8, 2021 10:18
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
@eytan-avisror eytan-avisror requested a review from a team as a code owner September 9, 2021 19:24
@eytan-avisror eytan-avisror changed the title Add AZ Cordon tool Add NAT AZ Cordon tool Sep 9, 2021
@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #56 (c0f2aa9) into master (7a30831) will increase coverage by 1.44%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   58.26%   59.71%   +1.44%     
==========================================
  Files           7        9       +2     
  Lines        1198     1390     +192     
==========================================
+ Hits          698      830     +132     
- Misses        417      467      +50     
- Partials       83       93      +10     
Impacted Files Coverage Δ
pkg/reaper/cordon/aznat/types.go 52.17% <52.17%> (ø)
pkg/reaper/cordon/aznat/aznat.go 73.97% <73.97%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a30831...c0f2aa9. Read the comment docs.

Eytan Avisror added 3 commits September 9, 2021 12:52
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
…o az-nat-cordon

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Copy link

@sahilbadla27 sahilbadla27 left a comment

Choose a reason for hiding this comment

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

LGTM

for _, r := range routes {
if restore {
if gws, ok := zoneMap[r.ZoneID]; ok {
r.NewGateway = gws[0]
Copy link
Member

Choose a reason for hiding this comment

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

What if there are multiple NATs in the same AZ? Could the original NAT be remembered by adding a tag? If a different NAT is restored than what was used originally then perhaps dependencies that whitelist a specific NAT IP will be broken? Perhaps this can be done as a followup feature?

Copy link
Collaborator Author

@eytan-avisror eytan-avisror Sep 14, 2021

Choose a reason for hiding this comment

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

Technically there is no difference between one NAT and another as long as they are in the same zone and are both healthy, hence we just chose one of existing NATs under that scope.
We can add tagging, but I am not sure there are use cases where that matters

@eytan-avisror eytan-avisror merged commit 92b6912 into keikoproj:master Sep 15, 2021
shaoxt pushed a commit to shaoxt/governor that referenced this pull request Dec 21, 2021
* az cordon tool

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

* fixes and unit tests

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

* documentation & fixes

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

* remove comments / newlines

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: shaoxt <shaoxt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants