OADP-5079 Add support for legacy aws plugin #1565

sseago · 2024-10-17T20:34:13Z

Why the changes were made

Certain s3 providers don't support the AWS SDK v2 updates made in velero-plugin-for-aws v1.9+ (OADP 1.4+). This PR provides the ability to use the v1.8 plugin in a newer OADP environment.

How to test the changes made

In the DPA DefaultPlugins section, use legacy-aws instead of aws. S3 providers that work with the regular aws plugin should still work here. S3 providers that error out in various ways with the regular plugin should also work.

sseago · 2024-10-17T20:34:44Z

PR is in Draft state now because this won't work until we have quay builds for the legacy plugin

docs/developer/PROW_CI.md

mateusoliveira43 · 2024-10-18T12:32:33Z

pkg/common/common.go

@@ -75,6 +76,7 @@ const (
 // Plugin names


This is confusing for me. Plugin names are aws, gcp, etc, for me. This is only used as the container name of each plugin, right?

The container name is the only actual "name" string that's used in the velero side of the configuration -- as the container name. aws/gcp/etc. only exist as values for the velero default plugin list. So from a velero point of view, this is the plugin name.

pkg/credentials/credentials.go

mateusoliveira43 · 2024-10-18T12:35:48Z

pkg/credentials/credentials.go

@@ -25,6 +25,7 @@ type DefaultPluginFields struct {
 	PluginImage        string
 	PluginSecretKey    string
 	PluginName         string


would change this to ContainerName to avoid confusion

mateusoliveira43 · 2024-10-18T12:39:13Z

controllers/bsl.go

@@ -398,9 +399,9 @@ func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.Backup
 	return nil
 }

-func pluginExistsInVeleroCR(configuredPlugins []oadpv1alpha1.DefaultPlugin, expectedPlugin oadpv1alpha1.DefaultPlugin) bool {
+func pluginExistsInVeleroCR(configuredPlugins []oadpv1alpha1.DefaultPlugin, expectedProvider string) bool {


This is a duplication of the function changed in controllers/vsl.go. I would delete function in that file and use this one

This function name is wrong, no? From what I see in code, DPA was called velero at some point in time. It should be pluginExistsInDPA or something like this, right?

I was trying to avoid making major functionality changes since we want to backport to 1.4. The only change here is we're passing a regular string rather than a string that's typecast to expectedPlugin -- until this change, the plugin name and the provider were always the same. Now they're not, since both legacy-aws and aws have the provider aws.

mateusoliveira43 · 2024-10-18T12:44:01Z

controllers/bsl.go

@@ -412,7 +413,7 @@ func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupS
 		return nil
 	}
 	// check for existence of provider plugin and warn if the plugin is absent
-	if !pluginExistsInVeleroCR(r.dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPlugin(bslSpec.Provider)) {
+	if !pluginExistsInVeleroCR(r.dpa.Spec.Configuration.Velero.DefaultPlugins, bslSpec.Provider) {
 		r.Log.Info(fmt.Sprintf("%s backupstoragelocation is configured but velero plugin for %s is not present", bslSpec.Provider, bslSpec.Provider))


@stillalearner changed this in VSL from a warning log and warning event to error validation. I think it makes sense to do the same for BSLs

Sachin, so there is validation here #1559 (comment)

The goal of this PR is not to change how BSLs work but to react to adding a new plugin where plugin name and provider don't match anymore. If we want to change validation rules, that should be a separate PR.

@mateusoliveira43 @sseago Sure, Thanks ! Will create a separate PR for this validation . Noted.

mateusoliveira43 · 2024-10-18T12:49:41Z

controllers/validator.go

@@ -155,5 +165,10 @@ func (r *DPAReconciler) ValidateVeleroPlugins(log logr.Logger) (bool, error) {
 			}
 		}
 	}
+
+	if foundAWSPlugin && foundLegacyAWSPlugin {


I would change code here to something like

if slices.Contains(dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPluginAWS) && slices.Contains(dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPluginLegacyAWS) {

I think it is easier to read

Readability is subjective here. I would find that change harder to read -- but we're already iterating over the slice once above. Adding this would mean 2 more iterations.

controllers/validator.go

config/manager/manager.yaml

README.md

sseago · 2024-10-22T18:09:18Z

/retest

kaovilai · 2024-10-22T20:26:09Z

TODO: Add legacy-aws to e2e

kaovilai · 2024-10-22T23:47:21Z

/test 4.16-e2e-test-aws

kaovilai · 2024-10-23T02:40:17Z

/test 4.16-e2e-test-aws

mateusoliveira43 · 2024-10-23T12:31:58Z

controllers/validator.go

@@ -104,10 +104,20 @@ func (r *DPAReconciler) ValidateVeleroPlugins(log logr.Logger) (bool, error) {
 		}
 	}

+	foundAWSPlugin := false
+	foundLegacyAWSPlugin := false
 	for _, plugin := range dpa.Spec.Configuration.Velero.DefaultPlugins {
 		pluginSpecificMap, ok := credentials.PluginSpecificFields[plugin]
 		pluginNeedsCheck, foundInBSLorVSL := providerNeedsDefaultCreds[string(plugin)]


Do we also need to change here because of line 94?

@mateusoliveira43 You are correct again. I'll fix this.

mateusoliveira43

I ran E2E tests in my IBM cluster, by changing $PROVIDER here

oadp-operator/tests/e2e/scripts/ibmcloud_settings.sh

Line 19 in 20d8c61

"openshift", "$PROVIDER"

to legacy-aws and they all passed ✅ (did not run virt tests)

I also ran some commands like grep -Inr "DefaultPlugins" . --exclude-dir=tests --exclude=\*_test.go, and I think all necessary code changes were made, except for this question #1565 (comment)

weshayutin · 2024-10-23T17:25:37Z

I ran E2E tests in my IBM cluster, by changing $PROVIDER here

oadp-operator/tests/e2e/scripts/ibmcloud_settings.sh

Line 19 in 20d8c61

"openshift", "$PROVIDER"

to legacy-aws and they all passed ✅ (did not run virt tests)
I also ran some commands like grep -Inr "DefaultPlugins" . --exclude-dir=tests --exclude=\*_test.go, and I think all necessary code changes were made, except for this question #1565 (comment)

FYI @mateusoliveira43 If this worked for you and have completed the code review I would expect to see you vote as well sir. Thank you

weshayutin · 2024-10-23T17:26:06Z

Testing will be covered here: https://issues.redhat.com/browse/OADP-5106

openshift-ci · 2024-10-23T17:30:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, shubham-pampattiwar, sseago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mateusoliveira43,shubham-pampattiwar,sseago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mateusoliveira43 · 2024-10-23T19:26:07Z

/lgtm

openshift-ci-robot · 2024-10-23T19:33:10Z

/retest-required

Remaining retests: 0 against base HEAD 20d8c61 and 2 for PR HEAD 2e096a5 in total

kaovilai · 2024-10-23T19:41:04Z

/override ci/prow/4.17-e2e-test-kubevirt-aws

openshift-ci · 2024-10-23T19:41:21Z

@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.17-e2e-test-kubevirt-aws

In response to this:

/override ci/prow/4.17-e2e-test-kubevirt-aws

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-sigs/prow repository.

sseago · 2024-10-23T19:41:36Z

/override ci/prow/4.17-e2e-test-kubevirt-aws

openshift-ci · 2024-10-23T19:41:53Z

@sseago: Overrode contexts on behalf of sseago: ci/prow/4.17-e2e-test-kubevirt-aws

In response to this:

/override ci/prow/4.17-e2e-test-kubevirt-aws

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-sigs/prow repository.

openshift-ci · 2024-10-23T19:44:27Z

@sseago: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.18-dev-preview-e2e-test-aws	`2e096a5`	link	false	`/test 4.18-dev-preview-e2e-test-aws`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

sseago marked this pull request as draft October 17, 2024 20:34

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2024

openshift-ci bot requested review from kaovilai and shubham-pampattiwar October 17, 2024 20:35

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2024

mateusoliveira43 reviewed Oct 18, 2024

View reviewed changes

docs/developer/PROW_CI.md Outdated Show resolved Hide resolved

mateusoliveira43 reviewed Oct 18, 2024

View reviewed changes

pkg/credentials/credentials.go Outdated Show resolved Hide resolved

mateusoliveira43 reviewed Oct 18, 2024

View reviewed changes

pkg/credentials/credentials.go Outdated Show resolved Hide resolved

mateusoliveira43 reviewed Oct 18, 2024

View reviewed changes

controllers/validator.go Outdated Show resolved Hide resolved

sseago force-pushed the legacy-aws branch from 1b31651 to e220f9f Compare October 18, 2024 13:56

rayfordj reviewed Oct 18, 2024

View reviewed changes

config/manager/manager.yaml Show resolved Hide resolved

sseago marked this pull request as ready for review October 22, 2024 14:25

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024

openshift-ci bot requested review from mpryc and mrnold October 22, 2024 14:26

mateusoliveira43 reviewed Oct 22, 2024

View reviewed changes

README.md Show resolved Hide resolved

sseago force-pushed the legacy-aws branch from e220f9f to 5c9ea1d Compare October 22, 2024 14:44

sseago force-pushed the legacy-aws branch from 5c9ea1d to 0b40558 Compare October 22, 2024 20:24

kaovilai mentioned this pull request Oct 22, 2024

Add legacy-aws to e2e #1566

Closed

kaovilai previously approved these changes Oct 22, 2024

View reviewed changes

mateusoliveira43 reviewed Oct 23, 2024

View reviewed changes

Add support for legacy aws plugin

2e096a5

sseago dismissed kaovilai’s stale review via 2e096a5 October 23, 2024 15:35

sseago force-pushed the legacy-aws branch from 0b40558 to 2e096a5 Compare October 23, 2024 15:35

shubham-pampattiwar approved these changes Oct 23, 2024

View reviewed changes

mateusoliveira43 approved these changes Oct 23, 2024

View reviewed changes

kaovilai mentioned this pull request Oct 23, 2024

OADP-5106: Add legacy-aws default plugin reconcile e2e test. #1568

Merged

openshift-ci bot assigned mateusoliveira43 Oct 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2024

openshift-merge-bot bot merged commit 0d5ba7b into openshift:master Oct 23, 2024
21 of 22 checks passed

sseago added a commit to sseago/oadp-operator that referenced this pull request Oct 23, 2024

Add support for legacy aws plugin (openshift#1565)

9c4e9a9

openshift-merge-bot bot pushed a commit that referenced this pull request Oct 24, 2024

Add support for legacy aws plugin (#1565) (#1570)

11ab518

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OADP-5079 Add support for legacy aws plugin #1565

OADP-5079 Add support for legacy aws plugin #1565

sseago commented Oct 17, 2024

sseago commented Oct 17, 2024

mateusoliveira43 Oct 18, 2024 •

edited

Loading

sseago Oct 18, 2024

mateusoliveira43 Oct 18, 2024

mateusoliveira43 Oct 18, 2024

sseago Oct 18, 2024

mateusoliveira43 Oct 18, 2024

sseago Oct 18, 2024

stillalearner Oct 18, 2024

mateusoliveira43 Oct 18, 2024

sseago Oct 18, 2024

sseago commented Oct 22, 2024

kaovilai commented Oct 22, 2024

kaovilai commented Oct 22, 2024

kaovilai commented Oct 23, 2024

mateusoliveira43 Oct 23, 2024

sseago Oct 23, 2024

mateusoliveira43 left a comment

weshayutin commented Oct 23, 2024

weshayutin commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

mateusoliveira43 commented Oct 23, 2024

openshift-ci-robot commented Oct 23, 2024

kaovilai commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

sseago commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

OADP-5079 Add support for legacy aws plugin #1565

OADP-5079 Add support for legacy aws plugin #1565

Conversation

sseago commented Oct 17, 2024

Why the changes were made

How to test the changes made

sseago commented Oct 17, 2024

mateusoliveira43 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseago commented Oct 22, 2024

kaovilai commented Oct 22, 2024

kaovilai commented Oct 22, 2024

kaovilai commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateusoliveira43 left a comment

Choose a reason for hiding this comment

weshayutin commented Oct 23, 2024

weshayutin commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

mateusoliveira43 commented Oct 23, 2024

openshift-ci-robot commented Oct 23, 2024

kaovilai commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

sseago commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

mateusoliveira43 Oct 18, 2024 •

edited

Loading