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

Allow master to touch volumes tagged with kubernetes.io/cluster/<clusterName>:owned #11729

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Jun 9, 2021

This fixes a specific situation where CSIMigrationAWS gets enabled then disabled, step by step details are below.

Basically, kops KCM in-tree EBS plugin and CSI EBS plugin need to agree on what tags to add to provisioned volumes and what IAM policies to ship with such that those provisioned volumes can be managed by either KCM or CSI. Otherwise when migration gets toggled, KCM/CSI won't be allowed to Attach/Delete volumes that were provisioned by CSI/KCM before the toggle. For more details, see: kubernetes-sigs/aws-ebs-csi-driver#927 (comment)

  1. Enable CSIMigrationAWS
  2. Deploy CSI EBS plugin and provide the clusterName via the k8s-tag-cluster-id flag. This tells it to create volumes with the kubernetes.io/cluster/:owned tag, to partially mimic KCM volume creation behavior. Partially because KCM creates volumes with the legacy KubernetesCluster: tag too.
  3. With the default StorageClass with provisioner kubernetes.io/aws-ebs installed, create a PVC.
  4. Since migration is enabled, CSI EBS plugin acts as provisioner kubernetes.io/aws-ebs. It creates a volume with tag kubernetes.io/cluster/:owned and corresponding PV. This volume can be attached, detached, and deleted by CSI.
  5. Disable CSIMigrationAWS (let's say I decide I don't like CSI...I expect my existing volumes with spec awsElasticBlockStore to go back to being managed by the in-tree plugin instead of CSI)
  6. The volume created in step 3 cannot be attached or deleted by KCM because it is only allowed to attach or delete volumes with the legacy KubernetesCluster: tag. (This goes against my expectation.)

So to fix step 6, this PR grants master instance on which KCM is running permission to touch volumes tagged with kubernetes.io/cluster/:owned in addition to the legacy KubernetesCluster: tag.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 9, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 Jun 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit a62c214 into kubernetes:master Jun 10, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 10, 2021
Comment on lines +913 to +914
"ec2:CreateRoute", // aws.go
"ec2:DeleteRoute", // aws.go
Copy link
Member

Choose a reason for hiding this comment

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

These aren't related to volumes and the EBS CSI driver so I'm curious why they're being included here.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a mistake to me. But I am planning on moving the EBS specific stuff into a dedicated function to support IRSA. Can remove it as part of that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I just copied the existing KubernetesCluster policy without questioning it

@olemarkus
Copy link
Member

I believe we add legacy tags to volumes provisioned by the CSI driver as well.

@johngmyers
Copy link
Member

We should be moving away from depending on the legacy tags. In a release we should probably drop the old policy statement.

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 16, 2021

@olemarkus , the CSI driver needs this patch to add legacy tags kubernetes-sigs/aws-ebs-csi-driver#932. The CSI driver installed by kops will need to have the --k8s-tag-cluster-id argument set on it if not already, I'll check

@olemarkus
Copy link
Member

We pass it in through --extra-tags. It is a generic list of tags that users can configure on all resources kOps/k8s provision. By default it contains the KubernetesCluster tag.

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants