-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Node metadata-concealment in GCE #8634
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geojaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
could you add terraform support as well? you should just need to add the Labels field to |
thanks for catching the omission in TF. I'll send back another one soon |
f7e8c9d
to
f8ba1d7
Compare
/retest |
/hold cancel |
@rifelpet is that what you were looking for re: terraform? |
yea, perfect 👍 |
I think this one is good for review now! |
/hold |
0f6f0ee
to
4dbdfaf
Compare
/hold cancel |
upup/models/cloudup/resources/addons/metadata-proxy.addons.k8s.io/v0.1.12.yaml
Outdated
Show resolved
Hide resolved
/retest |
this looks good to me now. my only concern is whether this requires users to take any external action when upgrading to the kops version that will include this PR. Will they need to enable something on the GCP side for this? If so we should mention it in the release notes. |
I think you're right- new IGs/clusters should default to this mode. I can create a release note and doc it. I guess we should also probably make it possible to "upgrade" to this by enabling it in existing IGs. Based on the current level of support for GCE, I think we could document how to enable this proxy if you want it, but try to steer folks towards using new IGs. I'll have to take another look to see what make most sense. |
@rifelpet added the release note with disclosure and upgrade instructions. how's that look? |
looks good. I think kops will apply the addon even for existing clusters that dont have the label, but the DaemonSet's labelSelector will prevent pods from getting scheduled on any nodes anyways, so i think thats fine. /lgtm |
/test pull-kops-verify-staticcheck |
Yes, it will create the addon, it'll just wait for the selector. I'm ok with that i think |
Fixes metadata-concealment tests in GCE
![image](https://user-images.githubusercontent.com/9451328/76169361-9db06c00-6134-11ea-9511-227a28c40f44.png)