Skip to content

NO-JIRA: Remove inject-proxy annotations for aws-ebs, aws-efs node daemonsets #308

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

Jump to bottom
Merged
merged 1 commit into from
Feb 20, 2025
Merged

NO-JIRA: Remove inject-proxy annotations for aws-ebs, aws-efs node daemonsets #308

merged 1 commit into from
Feb 20, 2025
+0 −5

Conversation

stephenfin
Copy link
Contributor

There is no proxy hook configured for either driver making this a no-op. Remove it.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 23, 2024
@openshift-ci-robot
Copy link

@stephenfin: This pull request explicitly references no jira issue.

In response to this:

There is no proxy hook configured for either driver making this a no-op. Remove it.

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 openshift-eng/jira-lifecycle-plugin repository.

@stephenfin stephenfin mentioned this pull request Oct 23, 2024
Merged
@openshift-ci openshift-ci bot requested review from gnufied and tsmetana October 23, 2024 12:50
@stephenfin
Copy link
Contributor Author

stephenfin commented Oct 23, 2024 •
edited
Loading

@jsafrane @gnufied I've only removed the config.openshift.io/inject-proxy: csi-driver annotation here. However, I'm wondering if I should also remove the config.openshift.io/inject-proxy-cabundle: csi-driver annotation and the associated hook from both drivers. Based on feedback in #304, we're saying that the drivers don't need access to the cloud on the node and there is therefore no need for a proxy hook. If this is the case, what is the reason for injecting CA bundles into the containers?

csi-operator/pkg/driver/aws-ebs/aws_ebs.go

Lines 314 to 325 in e71aab2

// withCABundleDaemonSetHook projects custom CA bundle ConfigMap into the CSI driver container
func withCABundleDaemonSetHook(c *clients.Clients) (csidrivernodeservicecontroller.DaemonSetHookFunc, []factory.Informer) {
hook := csidrivernodeservicecontroller.WithCABundleDaemonSetHook(
c.GuestNamespace,
trustedCAConfigMap,
c.GetConfigMapInformer(c.GuestNamespace),
)
informers := []factory.Informer{
c.GetConfigMapInformer(c.GuestNamespace).Informer(),
}
return hook, informers
}

which resolves to the below that explicitly references use for proxies:

https://github.com/openshift/library-go/blob/abb8c75b88dcba14fb3b27f6bf47388d3fc71ca2/pkg/operator/csi/csidrivernodeservicecontroller/helpers.go#L50-L60

Should I also be dropping the config.openshift.io/inject-proxy-cabundle: csi-driver annotation and the withCABundleDaemonSetHook hook?

@stephenfin
Copy link
Contributor Author

/retest-required

2 similar comments
@stephenfin
Copy link
Contributor Author

/retest-required

@stephenfin
Copy link
Contributor Author

/retest-required

@jsafrane
Copy link
Contributor

jsafrane commented Dec 9, 2024

/lgtm
/approve

/label px-approved
/label docs-approved
I leave it to QE to validate the AWS EBS and AWS EFS daemonsets do not currently talk to the cloud API.

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Dec 9, 2024
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 9, 2024
@Phaow
Copy link
Contributor

Phaow commented Dec 10, 2024

/label qe-approved
The presubmit jobs aws-efs-operator-e2e-extended and e2e-aws-csi-extended already use the proxy configured cluster to execute the e2e tests, the results looks good. It seems azure file needs the proxy hook talk to the cloud API while both aws disk and file does not need.

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 10, 2024
@stephenfin
aws-ebs, aws-efs: Remove inject-proxy annotation

Verified

This commit was signed with the committer’s verified signature.
bryan-cox Bryan Cox
GPG key ID: 117B7793559C9636
Verified
Learn about vigilant mode
Loading
Loading status checks…
693e4fe 
There is no proxy hook configured for either driver making this a no-op.
Remove it.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin stephenfin force-pushed the remove-aws-proxy-annotations branch from 9813dde to 693e4fe Compare January 16, 2025 12:29
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 16, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, stephenfin

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:

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

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 16, 2025 •
edited
Loading

@stephenfin: The following tests 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/e2e-azurestack-csi 9813dde link false /test e2e-azurestack-csi
ci/prow/hypershift-e2e-openstack-csi-cinder 9813dde link true /test hypershift-e2e-openstack-csi-cinder
ci/prow/okd-scos-e2e-aws-ovn 693e4fe link false /test okd-scos-e2e-aws-ovn

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.

@jsafrane
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025
@stephenfin
Copy link
Contributor Author

/retest-required

Both failures are clearly unrelated to the change.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 8a67b26 and 2 for PR HEAD 693e4fe in total

@openshift-merge-bot openshift-merge-bot bot merged commit 6ab7a9a into openshift:main Feb 20, 2025
22 of 23 checks passed
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-aws-efs-csi-driver-operator
This PR has been included in build ose-aws-efs-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: azure-file-csi-driver-operator
This PR has been included in build ose-azure-file-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: csi-driver-manila-operator
This PR has been included in build csi-driver-manila-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-azure-disk-csi-driver-operator
This PR has been included in build ose-azure-disk-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-aws-ebs-csi-driver-operator
This PR has been included in build ose-aws-ebs-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-openstack-cinder-csi-driver-operator
This PR has been included in build ose-openstack-cinder-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-smb-csi-driver-operator
This PR has been included in build ose-smb-csi-driver-operator-container-v4.19.0-202502200306.p0.g6ab7a9a.assembly.stream.el9.
All builds following this will include this PR.

@stephenfin stephenfin deleted the remove-aws-proxy-annotations branch February 25, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied

@tsmetana tsmetana

Assignees

@jsafrane jsafrane

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stephenfin @openshift-ci-robot @jsafrane @Phaow @openshift-bot