Cluster-api: Populate Type field of CAPI secrets

[wfernandes]

User Story

As an operator I would like to ensure that controllers are reading in secrets of a specified type in order to ensure integrity and validity.

Detailed Description
This issue is to discuss how to apply the Type field for Secrets created by the kubeadm control plane and cluster controllers pre-v1alpha3.

Currently, only the new secrets created by the bootstrap kubeadm controller will contain the Type. For all previously created secrets without the Type specified, we need to find a way to "update" them to include the specified Type.

Once this is done a follow up issue #2479 can handle to actual restriction of the controllers handling secrets with the specified Type.

Anything else you would like to add:

References:
https://github.com/kubernetes-sigs/cluster-api/issues/2440 issue
https://github.com/kubernetes-sigs/cluster-api/pull/2443#issuecomment-592211662 PR

/kind feature

[k8s-ci-robot]

@wfernandes: The label(s) kind/ cannot be applied, because the repository doesn't have them

In response to this:

User Story

As an operator I would like to ensure that controllers are reading in secrets of a specified type in order to ensure integrity and validity.

Detailed Description
This issue is to discuss how to apply the Type field for Secrets created by the kubeadm control plane and cluster controllers pre-v1alpha3.

Currently, only the new secrets created by the bootstrap kubeadm controller will contain the Type. For all previously created secrets without the Type specified, we need to find a way to "update" them to include the specified Type.

Once this is done a follow up issue (TODO: ADD ISSUE HERE) can handle to actual restriction of the controllers handling secrets with the specified Type.

Anything else you would like to add:

References:
https://github.com/kubernetes-sigs/cluster-api/issues/2440 issue
https://github.com/kubernetes-sigs/cluster-api/pull/2443#issuecomment-592211662 PR

/kind feature

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@wfernandes: The label(s) kind/ cannot be applied, because the repository doesn't have them

In response to this:

User Story

As an operator I would like to ensure that controllers are reading in secrets of a specified type in order to ensure integrity and validity.

Detailed Description
This issue is to discuss how to apply the Type field for Secrets created by the kubeadm control plane and cluster controllers pre-v1alpha3.

Currently, only the new secrets created by the bootstrap kubeadm controller will contain the Type. For all previously created secrets without the Type specified, we need to find a way to "update" them to include the specified Type.

Once this is done a follow up issue #2479 can handle to actual restriction of the controllers handling secrets with the specified Type.

Anything else you would like to add:

References:
https://github.com/kubernetes-sigs/cluster-api/issues/2440 issue
https://github.com/kubernetes-sigs/cluster-api/pull/2443#issuecomment-592211662 PR

/kind feature

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vincepri]

/milestone Next

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[vincepri]

/milestone v0.4.0

[vincepri]

/priority important-soon

[srm09]

/assign

[srm09]

Here is a doc outlining the scope and type of changes needed:
Google Doc: Type field of CAPI Secrets

[srm09]

/retitle Populate Type field of CAPI secrets

[srm09]

As per the discussion with @wfernandes and @vincepri , this is the updated scope of the issue:

1. Update the util functions for secret generation to set the Type field for Secrets
2. Add extra logic to the reconcile loops to populate the Type field for Secrets created by pre-v1alpha4 controllers.

[srm09]

Update on the issue:
Regarding the secrets created by pre-v1alpha4 controllers,
The Type field for Secrets is immutable, thus cannot be changed via Patch/Update calls to the API server.

• A way around is to delete/recreate(back to back) the Secret which might require a privilege escalation for the capi-kubeadm service account.
• Another way could be to backup(copy to a secret with a diff name)-delete(the secret)-create(a new secret with the same name and original data)

The first way seems destructive and can lead to a loss of cluster access if the controller crashes midway. The second way might be a better solution and could be done as a step (by the management cluster operator) while upgrading from v1a3 to v1a4.

What do y'all think about this?

[ncdc]

Is this timeline accurate?

1. v1alpha2 did not set the bootstrap secret type / controllers don't inspect the secret type
2. v1alpha3 sets the bootstrap secret type (at least for CABPK) / controllers still don't inspect the secret type
3. v1alpha4 continues to set the bootstrap secret type (CABPK) / controllers ... ?

If so, I think we can proceed to update the infrastructure provider contract to only accept bootstrap secrets that have the correct type set. I think it will be extremely unlikely that a controller talking v1alpha4 would encounter an _unused/unprocessed_ bootstrap secret created in the v1alpha2 time frame (i.e., without the secret type) - right? Similarly, I would not expect a situation where an infrastructure provider talking v1alpha4 would need to process a v1alpha3 bootstrap CR that doesn't have the secret type set (for bootstrap providers other than CABPK that maybe aren't setting the type in v1alpha3).

Are there other secrets where we are or need to be setting the type & checking for it?

[vincepri]

I think it will be extremely unlikely that a controller talking v1alpha4 would encounter an unused/unprocessed bootstrap secret created in the v1alpha2 time frame

This is true for the bootstrap provider, but all the other secrets generated by Cluster API won't fall into that category. For example the kubeconfig secret, CA, etc

[ncdc]

Right, that's why I was asking about other secrets 😄.

I like @srm09's second suggestion, to find a non-destructive way to migrate the data. I am wondering if we should put this logic in the proper/respective controllers instead of in the management cluster operator, mainly because we can't guarantee that everyone who has existing CAPI clusters will be using the operator going forward. WDYT?

[srm09]

We tried going down that route, of updating the Type field in the controllers itself. Tried to delete and recreate which failed due to RBAC permissions for the kubeadm service account. So, the other thing that comes to mind is:
For instance, lets consider a secret named foo

• Create a clone of the existing secret named foo-new with the Type field setup correctly
• Rename the secret foo to foo-backup
• Rename the secret foo-new to foo

Does this seem like a good way to move forward with?

[detiber]

@srm09 metadata.name is also immutable, so a renaming approach will suffer the same issues as delete/recreate

[detiber]

The safest bet will likely be to have a step that is performed prior to upgrading components to v1alpha4. This could be automated in the case someone is using the management cluster operator, or can be a manually performed step in the case they are using clusterctl or the clusterctl library.

Ideally we would be able to do this within the controllers that are creating and/or consuming these secrets as part of the migration, but I'm not sure it makes sense in this specific case because granting cluster wide delete permissions for secrets via RBAC seems excessive for a one time migration.

[srm09]

Yes, escalation of privileges does not make sense for this migration. @vincepri also had the same idea of adding it as a manual step for folks not using management cluster operator.
Plus, he also suggested that if/when the check by the controllers for secret types is implemented, we can put it under a feature flag for v1a4 before making it mandatory in v1a5. This would allow CAPI users some time to update the secret types.

[srm09]

I am gonna create a PR which just updates the current secret generation code under the util directory. And once we have consensus around the approach for pre-v1alpha4 secrets, I can make the necessary changes for those as well.