Fix crash in copyVolumeDataPodFunc when location type is not GCS

[e-sumin]

Change Overview

copyVolumeDataPodFunc is crashing when location type is not GCS

Pull request type

Please check the type of change your PR introduces:

• 🚧 Work in Progress
• 🌈 Refactoring (no functional changes, no api changes)
• 🐹 Trivial/Minor
• 🐛 Bugfix
• 🌻 Feature
• 🗺️ Documentation
• 🤖 Test

• 💪 Manual
• ⚡ Unit test
• 💚 E2E

[github-actions]

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

[viveksinghggits]

should we use the parent context instead of creating new one.

[denisvmedia]

If the parent context dies, the clean up call will be aborted as well. Thus, I think there's no issue here. But maybe it makes sense to put a note.

[e-sumin]

Exactly, I'll add a comment

[denisvmedia]

I think the real cause of the failure is that the remover is unexpectedly nil (see: https://github.com/kanisterio/kanister/blob/2babd1ad48c4f408f614c50ded06a2f3eb2a13b0/pkg/function/utils.go#LL102C1-L113C2). I strongly believe (however, my beliefs might be wrong) that returning nil for the object when there is no error (thus return nil, nil) is wrong in most of the cases (and here as well).

If we check

// WriteCredsToPod creates a file with Google credentials if the given profile points to a GCS location
func WriteCredsToPod(ctx context.Context, writer kube.PodFileWriter, profile *param.Profile) (kube.PodFileRemover, error) {
	if profile.Location.Type == crv1alpha1.LocationTypeGCS {
		remover, err := writer.Write(ctx, consts.GoogleCloudCredsFilePath, bytes.NewBufferString(profile.Credential.KeyPair.Secret))
		if err != nil {
			return nil, errors.Wrapf(err, "Unable to write Google credentials to the pod.")
		}

		return remover, nil
	}
	return nil, nil
}

I'd instead implement a NopRemover that would do nothing, and return it instead of nil. This will prevent further mistakes like the one solved in this PR.

[denisvmedia]

Check my PR comment: #2072 (comment)

[e-sumin]

Ok, I agree with NopRemover. I thought about this but decided that probably it's an overkill. But since this came not only to my mind, it probably makes sense.

[denisvmedia]

Suggested change

	var _ kube.PodFileRemover = (*nopRemover)(nil)
	func (nr *nopRemover) Remove(ctx context.Context) error {
	return nil
	}
	func (nr *nopRemover) Path() string {
	return ""
	}
	var _ kube.PodFileRemover = (nopRemover)(nil)
	func (nr nopRemover) Remove(ctx context.Context) error {
	return nil
	}
	func (nr nopRemover) Path() string {
	return ""
	}

[denisvmedia]

Suggested change

	return &nopRemover{}, nil
	return nopRemover{}, nil

[e-sumin]

done