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

Support optional secret containing only credentials but no user-data; accept Gardener secret data keys #578

Merged
merged 4 commits into from
Dec 2, 2020

Conversation

rfranzke
Copy link
Member

@rfranzke rfranzke commented Nov 29, 2020

/area open-source usability
/kind enhancement
/priority normal

What this PR does / why we need it:
This PR extends all in-tree and the out-of-tree machine classes with a new .{spec.}credentialsSecretRef field. It is an optional reference to a secret containing only the credentials, while today's .{spec.}secretRef still contains the user-data.
This is helpful for scenarios like Gardeners where the credentials for all machine classes are the same while only the user-data differs.

Additionally, all the secret keys used by Gardener are now also allowed as alternatives for the machine-controller-manager. This helps to not make mappings for the data keys.

Special notes for your reviewer:
The implementation is entirely backwards-compatible and optional, both for the in-tree and the out-of-tree implementations. The in-tree drivers have been simplified by only working with the secret data map instead of the whole v1.Secret object. The out-of-tree contract is untouched. If a credentials secret reference is provided then its content will be merged into the secret that is passed to the provider.

ℹ️ For simplification of the review process the PR is split into 4 different commits that could be reviewed individually.

/assign @hardikdr @prashanth26 @AxiomSamarth
/invite @hardikdr @prashanth26 @AxiomSamarth

/cc @vpnachev @timuthy

Release note:

All machine classes do now support an optional `.{spec.}credentialsSecretRef` field in addition to today's `.{spec.}secretRef` field. If `.{spec.}credentialsSecretRef` is non-nil then the provider credentials will be read out of this secret. The user-data for the machine bring-up is still required to be part of the secret referenced by `.{spec.}secretRef`.
Some machine class secrets are now supporting alternative data keys:
* The machine class secret for Alicloud machines does now also accept the data keys `accessKeyID` and `accessKeySecret` as alternatives for today's keys.
* The machine class secret for AWS machines does now also accept the data keys `accessKeyID` and `secretAccessKey` as alternatives for today's keys.
* The machine class secret for Azure machines does now also accept the data keys `clientID`, `clientSecret`, `subscriptionID` and `tenantID` as alternatives for today's keys.
* The machine class secret for GCP machines does now also accept the data key `serviceaccount.json` as alternatives for today's key.

@rfranzke rfranzke requested review from ggaurav10 and a team as code owners November 29, 2020 17:32
@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else labels Nov 29, 2020
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) labels Nov 29, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 29, 2020
@gardener-robot gardener-robot added area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/usability Usability related kind/enhancement Enhancement, improvement, extension priority/normal labels Nov 29, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2020
Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

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

/invite @amshuman-kr

Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

Thanks for the quick and extensive PR @rfranzke ! LGTM

The only question I have is that while this PR retains the signature of the OOT interfaces but changes the semantics. I.e. the secret object that is passed is no longer backed by a real secret object. As long as the OOT providers do not expect the secret object to be a real one in the apiserver, this should be fine. Ideally, they shouldn't. @prashanth26 Should we make this explicit in the documentation?

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Nov 30, 2020
@rfranzke
Copy link
Member Author

The only question I have is that while this PR retains the signature of the OOT interfaces but changes the semantics. I.e. the secret object that is passed is no longer backed by a real secret object. As long as the OOT providers do not expect the secret object to be a real one in the apiserver, this should be fine. Ideally, they shouldn't. @prashanth26 Should we make this explicit in the documentation?

Agreed, good point. I stumbled across this as well, and for the sake of not changing the contract I created a new v1.Secret object and merged all data into it. IMO the interface should be changed to only work with a secret data map and not with the whole secret object, similar to what I've done for the in-tree drivers. I guess all out-of-tree providers are only using the data of the secret anyways.

@amshuman-kr
Copy link
Collaborator

amshuman-kr commented Nov 30, 2020

IMO the interface should be changed to only work with a secret data map and not with the whole secret object, similar to what I've done for the in-tree drivers. I guess all out-of-tree providers are only using the data of the secret anyways.

I agree. That way the contract will be explicit. @prashanth26 Should we consider changing the OOT signature? We need not do it in this PR, of course.

@prashanth26
Copy link
Contributor

prashanth26 commented Nov 30, 2020

I agree with you guys. Something I probably overlooked while making the initial interfaces. Will try to incorporate this change in the coming releases. Yes, for now, let's stick to this.

Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

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

/lgtm
Waiting for approval from @amshuman-kr

Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

LGTM

Something I probably overlooked while making the initial interfaces.

We all did @prashanth26. Hindsight is 20-20 🙂 It is 2020 too 😉

Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

/lgtm

@prashanth26 prashanth26 merged commit fefbdb6 into gardener:master Dec 2, 2020
@rfranzke rfranzke deleted the enh/credentials-secret branch December 2, 2020 07:32
@prashanth26
Copy link
Contributor

prashanth26 commented Dec 2, 2020

I shall trigger an MCM release as the new field will have to be added in all the provider migration codes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/usability Usability related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants