Skip to content

pkg/destroy: data/aws: delete instance profiles even if they are detached #1268

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Mar 13, 2019
Merged

pkg/destroy: data/aws: delete instance profiles even if they are detached #1268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pkg/destroy/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ func NewAWS(logger logrus.FieldLogger, metadata *types.ClusterMetadata) (Destroy
}

return &aws.ClusterUninstaller{
Filters: filters,
Region: metadata.ClusterPlatformMetadata.AWS.Region,
Logger: logger,
Filters: filters,
Region: metadata.ClusterPlatformMetadata.AWS.Region,
Logger: logger,
ClusterID: metadata.InfraID,
Copy link
Member

@wking wking Mar 12, 2019

Choose a reason for hiding this comment

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

This will cause a trivial conflict with #1365, which is a higher-priority bugfix.

}, nil
}

Expand Down
62 changes: 51 additions & 11 deletions pkg/destroy/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ type ClusterUninstaller struct {
// }
//
// will match resources with (a:b and c:d) or d:e.
Filters []Filter // filter(s) we will be searching for
Logger logrus.FieldLogger
Region string
Filters []Filter // filter(s) we will be searching for
Logger logrus.FieldLogger
Region string
ClusterID string
}

func (o *ClusterUninstaller) validate() error {
Expand Down Expand Up @@ -110,7 +111,7 @@ func (o *ClusterUninstaller) Run() error {
logger: o.Logger,
}

return wait.PollImmediateInfinite(
err = wait.PollImmediateInfinite(
time.Second*10,
func() (done bool, err error) {
var loopError error
Expand Down Expand Up @@ -196,6 +197,16 @@ func (o *ClusterUninstaller) Run() error {
return len(tagClients) == 0 && loopError == nil, nil
},
)
if err != nil {
return err
}

o.Logger.Debug("search for untaggable resources")
if err := o.deleteUntaggedResources(awsSession); err != nil {
o.Logger.Debug(err)
return err
}
return nil
}

func splitSlash(name string, input string) (base string, suffix string, err error) {
Expand Down Expand Up @@ -612,6 +623,26 @@ func deleteEC2Instance(ec2Client *ec2.EC2, iamClient *iam.IAM, id string, logger
logger.Info("Deleted")
}
}
return nil
}

// This is a bit of hack. Some objects, like Instance Profiles, can not be tagged in AWS.
// We "normally" find those objects by their relation to other objects. We have found,
// however, that people regularly delete all of their instances and roles outside of
// openshift-install destroy cluster. This means that we are unable to find the Instance
// Profiles.
//
// This code is a place to find specific objects like this which might be dangling.
func (o *ClusterUninstaller) deleteUntaggedResources(awsSession *session.Session) error {
iamClient := iam.New(awsSession)
masterProfile := fmt.Sprintf("%s-master-profile", o.ClusterID)
if err := deleteIAMInstanceProfileByName(iamClient, &masterProfile, o.Logger); err != nil {
return err
}
workerProfile := fmt.Sprintf("%s-worker-profile", o.ClusterID)
if err := deleteIAMInstanceProfileByName(iamClient, &workerProfile, o.Logger); err != nil {
return err
}

return nil
}
Expand Down Expand Up @@ -1132,12 +1163,25 @@ func deleteIAM(session *session.Session, arn arn.ARN, logger logrus.FieldLogger)
}
}

func deleteIAMInstanceProfileByName(client *iam.IAM, name *string, logger logrus.FieldLogger) error {
_, err := client.DeleteInstanceProfile(&iam.DeleteInstanceProfileInput{
InstanceProfileName: name,
})
if err != nil {
if err.(awserr.Error).Code() == iam.ErrCodeNoSuchEntityException {
return nil
}
return err
}
logger.WithField("name", name).Info("Deleted")
Copy link
Member

@wking wking Mar 12, 2019

Choose a reason for hiding this comment

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

nit: I'd rather have the caller set this context, like we do here, etc. The caller can chose which of the information available to it should be logged. Functions can add additional logging context as new information become available, but they shouldn't write their arguments directly into the logging context.

return err
}

func deleteIAMInstanceProfile(client *iam.IAM, profileARN arn.ARN, logger logrus.FieldLogger) error {
resourceType, name, err := splitSlash("resource", profileARN.Resource)
if err != nil {
return err
}
logger = logger.WithField("name", name)

if resourceType != "instance-profile" {
return errors.Errorf("%s ARN passed to deleteIAMInstanceProfile: %s", resourceType, profileARN.String())
Expand All @@ -1162,17 +1206,13 @@ func deleteIAMInstanceProfile(client *iam.IAM, profileARN arn.ARN, logger logrus
if err != nil {
return errors.Wrapf(err, "dissociating %s", *role.RoleName)
}
logger.WithField("role", *role.RoleName).Info("Disassociated")
logger.WithField("name", name).WithField("role", *role.RoleName).Info("Disassociated")
}

_, err = client.DeleteInstanceProfile(&iam.DeleteInstanceProfileInput{
InstanceProfileName: profile.InstanceProfileName,
})
if err != nil {
if err := deleteIAMInstanceProfileByName(client, profile.InstanceProfileName, logger); err != nil {
return err
}

logger.Info("Deleted")
return nil
}

Expand Down