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

Stop passing in ClusterCA and Endpoint information for Windows nodes #337

Merged
merged 2 commits into from
Nov 22, 2021

Conversation

backjo
Copy link
Collaborator

@backjo backjo commented Nov 12, 2021

The Windows EKS bootstrap script currently contains a bug related to ServiceCidr calculations when passing in ClusterCA and APIEndpoint information - see awslabs/amazon-eks-ami#805. Until the bootstrap script allows us to provide the ServiceCIDR, we should stop passing in the ClusterCA and Endpoint information to avoid incorrect CIDRs from being calculated.

…for ServiceCidr calculation

Signed-off-by: Jonah Back <jonah@jonahback.com>
@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #337 (5663c71) into master (5215605) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   51.08%   51.09%   +0.01%     
==========================================
  Files          33       33              
  Lines        4504     4505       +1     
==========================================
+ Hits         2301     2302       +1     
  Misses       2062     2062              
  Partials      141      141              
Impacted Files Coverage Δ
controllers/provisioners/provisioners.go 100.00% <ø> (ø)
controllers/provisioners/eks/eks.go 87.50% <100.00%> (+0.26%) ⬆️
controllers/provisioners/eks/helpers.go 91.39% <100.00%> (ø)

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 5215605...5663c71. Read the comment docs.

@eytan-avisror
Copy link
Collaborator

Since it's a bug workaround, should we have a flag to control this behavior? e.g. --disable-windows-cluster-ca-detection or similar? it might be too specific, but that way we won't need another release when it's fixed and we end up giving more flexibility
WDYT

@backjo
Copy link
Collaborator Author

backjo commented Nov 12, 2021

@eytan-avisror it depends on how they fix it - if the fix is to require passing in a ServiceCidr parameter, we'd need another update to propagate that information. I'm fine flagging it for now.

Signed-off-by: Jonah Back <jonah@jonahback.com>
@backjo
Copy link
Collaborator Author

backjo commented Nov 18, 2021

@eytan-avisror - added a flag for the controller to allow toggling injection.

@eytan-avisror eytan-avisror merged commit 306c823 into master Nov 22, 2021
@tekenstam tekenstam deleted the fix/WindowsServiceCidr branch September 18, 2023 05:48
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.

2 participants