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

chore: Update azure-aci to use nodeutil instead of node-cli #277

Closed
wants to merge 2 commits into from

Conversation

cpuguy83
Copy link
Contributor

Fix webhook certificate auth with nodeutil

  1. Routes were not setup and causing panics
  2. The CA was not being loaded into the webhook auth and as such only
    token auth was available.

@cpuguy83 cpuguy83 requested review from feiskyer and helayoty August 30, 2022 23:24
@cpuguy83 cpuguy83 temporarily deployed to test August 30, 2022 23:24 Inactive
@cpuguy83 cpuguy83 temporarily deployed to test August 30, 2022 23:24 Inactive
@cpuguy83
Copy link
Contributor Author

cpuguy83 commented Aug 30, 2022

Big diff due to undoing the revert.
The actual fix is in the 2nd commit: 33a161c

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Merging #277 (ff32cfd) into master (803f4b7) will decrease coverage by 0.24%.
The diff coverage is 37.66%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   42.79%   42.54%   -0.25%     
==========================================
  Files          35       35              
  Lines        3491     3516      +25     
==========================================
+ Hits         1494     1496       +2     
- Misses       1816     1837      +21     
- Partials      181      183       +2     
Impacted Files Coverage Δ
provider/podsTracker.go 0.00% <0.00%> (ø)
provider/aci.go 36.08% <34.21%> (-0.13%) ⬇️
provider/metrics/metrics.go 80.74% <80.00%> (-1.97%) ⬇️
provider/config.go 68.29% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@helayoty helayoty requested review from Fei-Guo and removed request for feiskyer August 31, 2022 20:30
@cpuguy83 cpuguy83 temporarily deployed to test August 31, 2022 23:44 Inactive
flags.StringVar(&clusterDomain, "cluster-domain", clusterDomain, "kubernetes cluster-domain")
flag.DurationVar(&startupTimeout, "startup-timeout", startupTimeout, "How long to wait for the virtual-kubelet to start")
flags.BoolVar(&disableTaint, "disable-taint", disableTaint, "disable the node taint")
flag.StringVar(&operatingSystem, "os", operatingSystem, "Operating System (Linux/Windows)")
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
flag.StringVar(&operatingSystem, "os", operatingSystem, "Operating System (Linux/Windows)")
flags.StringVar(&operatingSystem, "os", operatingSystem, "Operating System (Linux/Windows)")

flags.StringVar(&nodeName, "nodename", nodeName, "kubernetes node name")
flags.StringVar(&cfgPath, "provider-config", cfgPath, "cloud provider configuration file")
flags.StringVar(&clusterDomain, "cluster-domain", clusterDomain, "kubernetes cluster-domain")
flag.DurationVar(&startupTimeout, "startup-timeout", startupTimeout, "How long to wait for the virtual-kubelet to start")
Copy link
Member

Choose a reason for hiding this comment

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

Not used anymore and needs to be removed.

@cpuguy83 cpuguy83 temporarily deployed to test September 1, 2022 21:38 Inactive
@helayoty helayoty added the kind/enhancement New feature or request label Sep 9, 2022
@helayoty helayoty temporarily deployed to test September 12, 2022 19:34 Inactive
@helayoty helayoty temporarily deployed to test September 12, 2022 19:34 Inactive
1. Routes were not setup and causing panics
2. The CA was not being loaded into the webhook auth and as such only
   token auth was available.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@helayoty helayoty temporarily deployed to test September 12, 2022 19:45 Inactive
@helayoty helayoty temporarily deployed to test September 12, 2022 19:45 Inactive
@helayoty helayoty changed the title Fix auth chore: Update azure-aci to use nodeutil instead of node-cli Sep 14, 2022
@helayoty helayoty closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants