Managed Application Credentials #450
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gardener-robot
added
kind/api-change
gardener-robot
added
area/security
gardener-robot-ci-2
added
reviewed/ok-to-test
kon-angelo
reviewed
Jun 4, 2022
Comment on lines
82
to
92
| if !appCredentialExists { | ||
| return m.removeApplicationCredentialStore(ctx, appCredential.secret) | ||
| } | ||
|
|
||
| if oldParentUserUsable { | ||
| if err := m.runGarbageCollection(ctx, oldParentUser, "", true); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return m.removeApplicationCredentialStore(ctx, appCredential.secret) |
There was a problem hiding this comment.
Maybe:
if appCredentialExists && oldParentUserUsable {
if err := m.runGarbageCollection(ctx, oldParentUser, "", true); err != nil {
return err
}
}
return m.removeApplicationCredentialStore(ctx, appCredential.secret)
| // application credential can be used. It will be tried to clean up with the | ||
| // old user if this one is usuable. | ||
| if newParentUser.isApplicationCredential() { | ||
| if appCredentialExists { |
There was a problem hiding this comment.
Do you need oldParentUsable check here too ? It seems similar to my previous comment so can they be consolidated e .g
if !m.config.Enabled || newParentUser.isApplicationCredential()
Comment on lines
128
to
122
| if parentChanged { | ||
| newAppCredential, err := m.createApplicationCredential(ctx, newParentUser) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Try to delete the old application credential owned by the old user. | ||
| // This might not work as the information about this user could be stale, | ||
| // because the user credentials are rotated, the user is not associated to | ||
| // Openstack project anymore or it is deleted. | ||
| if err := oldParentUser.identityClient.DeleteApplicationCredential(ctx, oldParentUser.id, appCredential.id); err != nil { | ||
| m.logger.Error(err, "could not delete application credential as the owning user has changed and information about owning user might be stale") | ||
| } | ||
|
|
||
| return m.storeApplicationCredential(ctx, newAppCredential, newParentUser) | ||
| } | ||
|
|
||
| if m.isExpired(appCredential) { | ||
| newAppCredential, err := m.createApplicationCredential(ctx, newParentUser) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return m.storeApplicationCredential(ctx, newAppCredential, newParentUser) | ||
| } | ||
|
|
||
| return m.storeApplicationCredential(ctx, appCredential, newParentUser) | ||
| } |
There was a problem hiding this comment.
I get the feeling that the code can be condensed a bit more e.g.
if parentChanged || m.isExpired(appCredential) || !appCredentialExists {
// Try to delete the old application credential owned by the old user.
// This might not work as the information about this user could be stale,
// because the user credentials are rotated, the user is not associated to
// Openstack project anymore or it is deleted.
if parentChanged{
if err := oldParentUser.identityClient.DeleteApplicationCredential(ctx, oldParentUser.id, appCredential.id); err != nil {
m.logger.Error(err, "could not delete application credential as the owning user has changed and information about owning user might be stale")
}
}
newAppCredential, err := m.createApplicationCredential(ctx, newParentUser)
if err != nil {
return err
}
if err := m.storeApplicationCredential(ctx, newAppCredential, newParentUser){
return err
}
}
if err := m.runGarbageCollection(ctx, newParentUser, appCredential.id, false); err != nil {
return err
}
It is easy to miss a case anyway so I am not sure how big of an improvement it is.
| } | ||
| } | ||
|
|
||
| func (m *Manager) runGarbageCollection(ctx context.Context, parent *parent, inUseAppCredentialID string, deleteInUseAppCredential bool) error { |
There was a problem hiding this comment.
From the uses in actuator_delete and actuator_reconcile it is not clear to me if we need both inUseAppCredentialID and deleteInUseAppCredential. The general idea is to not delete the current in-use things and also have a mode where you delete everything. Isn't the same if you pass an empty in-use credential to delete everything ?
There was a problem hiding this comment.
No we don't need both. It would be sufficient to pass the appCredentialID as a *string and check for nil. That's how I had it in the beginning and then changed to current version for better readability. Now I'm rather with you and changed is back.
… config in the controller config
dkistner
force-pushed
the
enh/managed-application-credentials
branch
from
119edf9
to
bd7691c
Compare
gardener-robot-ci-3
added
reviewed/ok-to-test
dkistner
force-pushed
the
enh/managed-application-credentials
branch
from
bd7691c
to
bab9674
Compare
gardener-robot-ci-2
added
reviewed/ok-to-test
|
@dkistner You need rebase this pull request with latest master branch. Please check. |
|
For the moment we will not continue with this PR. The extension and dependent components (mcm, ccm, csi etc.) are capable to work with application credentials. The lifecycle of the application credentials need for now be managed externally. /close |
gardener-robot
added
the
status/closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/area security
/kind enhancement
/platform openstack
What this PR does / why we need it:
This PR adds support for application credentials managed by the Openstack extension.
The application credentials will be owned, created and deleted by the (technical) user which is provided by the Shoot owner.
All api calls to the Openstack layer will be made by the application credential, except the ones that are required to manage the application credential itself.
Using a managed application credential will help to not disrupt a cluster while the (technical) user of the cluster is exchanged, expired or its secret is rotated.
The application credential will be renewed after a configured lifetime during the next Shoot reconciliation/deletion operation. Even if the extension is unable the manage the application credential the application credential will be invalidated after a configured period via Openstack means.
The managed application credential usage is disabled by default and can be enabled via the controller config of the extension.
Which issue(s) this PR fixes:
Fixes #429
Special notes for your reviewer:
The following tasks need to be done fulfilled before this PR can be merged:
The PR is opened as draft to start gathering feedback.
/invite @kon-angelo
/cc @donistz
Release note:
/squash