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

test: Change test to check for kubelet.service file existence as well. #3144

Merged
merged 2 commits into from
May 5, 2023

Conversation

maxwolffe
Copy link
Contributor

What type of PR is this?
/kind test

What this PR does / why we need it:
This PR adds a little more context to the test added in #3104 for issue #2815

The issue #2815 describes the full set of currently required data for custom images which don't rely on the CSE script to execute.

Those are:

  • /etc/default/kubelet
  • /var/lib/kubelet/bootstrap-kubeconfig
  • /etc/kubernetes/certs/ca.crt

The Databricks usecase also has a silly dependency on kubelet.service which I have removed and will roll out in the next month. I've added it as a required file for now, but happy to not if it's possible to otherwise prevent it from being removed in the near term?

/opt/azure/containers/kubelet.sh also shouldn't be required, but currently is because it's referenced by the kubelet.service.

Which issue(s) this PR fixes:
None

Requirements:

Special notes for your reviewer:
First PR in this repo so apologies if I'm missing something / happy to make changes.

I've confirmed that tests failed first, then passed with this change.

I have a potentially dumb question about TLSBootstrapping being enabled. Under what conditions is it NOT enabled?

Found this -

return tlsBootstrapToken != nil
- but perhaps someone has context already?

Thanks!

Release note:

none

Copy link
Contributor

@alexeldeib alexeldeib left a comment

Choose a reason for hiding this comment

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

lgtm

@maxwolffe maxwolffe temporarily deployed to test May 3, 2023 17:24 — with GitHub Actions Inactive
@maxwolffe maxwolffe force-pushed the add_kubelet_service branch from 52195e7 to 17b8f76 Compare May 3, 2023 23:10
@maxwolffe
Copy link
Contributor Author

@alexeldeib - updated the commit messages to pass linting - could you re-approve tests at your convenience?

Also - e2e tests failed on the last run due to az login failed - any other follow up I need to make there?

@maxwolffe maxwolffe temporarily deployed to test May 4, 2023 23:36 — with GitHub Actions Inactive
@alexeldeib
Copy link
Contributor

alexeldeib commented May 4, 2023

unfortunately we have some annoying rbac issues running the e2e/generated tests from forks today :(

the lint is fine after rebase, I'm about to break it again by clicking the rebase button but will squash-merge and ignore the other bit for now.

@alexeldeib alexeldeib temporarily deployed to test May 5, 2023 00:35 — with GitHub Actions Inactive
@alexeldeib alexeldeib enabled auto-merge (squash) May 5, 2023 00:35
@alexeldeib
Copy link
Contributor

I have a potentially dumb question about TLSBootstrapping being enabled. Under what conditions is it NOT enabled?

tl;dr - always today. the token itself is generated by RP, so it's possible the field in the struct is empty. but that's never the case today anymore afaik.

@alexeldeib alexeldeib merged commit 83bf40f into Azure:master May 5, 2023
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