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

Don't add control-plane DNS permissions with UseServiceAccountIAM #11086

Conversation

justinsb
Copy link
Member

Should not be needed; dns-controller should run on the control-plane
node so there should not be a bootstrapping problem with the nodes.

Reverts #10529

Should not be needed; dns-controller should run on the control-plane
node so there should not be a bootstrapping problem with the nodes.

Reverts kubernetes#10529
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 20, 2021
@k8s-ci-robot k8s-ci-robot requested review from hakman and zetaab March 20, 2021 18:01
@justinsb
Copy link
Member Author

cc @rifelpet

I'm not quite sure what happened before; dns-controller should have had permissions to set up the DNS names, and it should run on the control-plane.

@justinsb
Copy link
Member Author

/assign @rifelpet

I think you have the best context here!

@rifelpet
Copy link
Member

I'm wondering if this would cause issues when enabling UseServiceAccountIAM on an existing cluster. kops update cluster --yes would immediately revoke the IAM permissions that dns-controller relies on, and then eventually dns-controller gets redeployed with the service account and the AWS_WEB_IDENTITY_TOKEN_FILE env var. During the time between those, DNS records won't be getting updated and this would likely be during cluster maintenance in which control plane instances are getting replaced and records need changing.

Do you think this is significant enough of an issue to warrant addressing?

@olemarkus
Copy link
Member

olemarkus commented Mar 20, 2021

I made the same change as part of #11088

If you run single-node CP and you roll the CP node, dns-controller is gone anyway.
If you run three-node CP and you roll a CP node, channels on one of the remaining nodes will update dns-controller long before the new node has booted. So I think this fairly safe.

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

Makes sense 👍🏻

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2b46042 into kubernetes:master Mar 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants