Skip to content
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.

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

ROX 23555: Remove egress-proxy #1768

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Conversation

ebensh
Copy link
Collaborator

@ebensh ebensh commented Apr 16, 2024

Description

Removes the egress-proxy, as it has been replaced by NetworkPolicies.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources
  • (If applicable) Changes to the dp-terraform Helm values have been reflected in the addon on integration environment

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

Copy link
Contributor

openshift-ci bot commented Apr 16, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from a7c3763 to 855b9cf Compare April 17, 2024 14:48
@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from 3d78716 to 446da01 Compare April 18, 2024 09:48
@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from faefc90 to dbff8dc Compare April 18, 2024 14:09
@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from dbff8dc to 5ca1258 Compare April 18, 2024 14:11
@ebensh ebensh requested a review from kylape April 18, 2024 19:56
@ebensh ebensh marked this pull request as ready for review April 18, 2024 19:56
Copy link
Contributor

@vladbologa vladbologa left a comment

Choose a reason for hiding this comment

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

According to https://issues.redhat.com/browse/ROX-13199 the egress-proxy will not be removed from existing tenants. We should check that this is really the case.

@vladbologa vladbologa dismissed their stale review April 22, 2024 17:38

Don't want to block this PR. The egress-proxy deployments will not be removed for existing tenants, but they will not be used anymore. We could also remove them with a script if it's too much effort to fix ROX-13199.

@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from 5ca1258 to 494c920 Compare April 29, 2024 12:14
@ebensh ebensh force-pushed the evan/rox-23555-remove-egress-proxy branch from 494c920 to f8086e8 Compare May 10, 2024 09:36
apiVersion: v1
kind: Service
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to change this from Service to NetworkPolicy because the tenant resource garbage collection mechanism no longer manages objects of type Service (see the change to zzz_managed_resources.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this will be a problem, but we do need to be careful - this isn't conditional on setting the secureTenantNetwork flag. If we disabled that flag, and this resource exists, all network traffic will be blocked for all tenants.

Can we use a non-NetworkPolicy resource? A ConfigMap or Secret?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if you noticed that this is a dummy file used in tests only.

Due to the way tenant resource deletion works, we can only use types that are mentioned in zzz_managed_resources.go. The types in that file are auto-generated by parsing everything in fleetshard/pkg/central/charts/data/tenant-resources.

These types currently are VerticalPodAutoscaler and NetworkPolicy. If you use any other type, the objects will not get deleted and some tests will fail (e.g. TestChartResourcesAreAddedAndRemoved).

The tests themselves are not conditional on secureTenantNetwork. But if we will no longer have NetworkPolicies deployed as tenant resources, this dummy file will need to be fixed, and dependent tests too.

@vladbologa vladbologa requested a review from ludydoo June 17, 2024 20:57
@@ -35,7 +35,6 @@ type Config struct {
ServiceAccountTokenFile string `env:"FLEET_MANAGER_TOKEN_FILE"`
CreateAuthProvider bool `env:"CREATE_AUTH_PROVIDER" envDefault:"false"`
MetricsAddress string `env:"FLEETSHARD_METRICS_ADDRESS" envDefault:":8080"`
EgressProxyImage string `env:"EGRESS_PROXY_IMAGE"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will deleting this remove the ENV flag and break if we try to set it? Or will the unknown flag be silently ignored?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the order in which these are loaded is in reverse.

.Values.fleetshardSync.egressProxy.image in the fleetshard-sync template creates the env var. The value in the env var is then loaded here.

They were both removed, but the helm value is still set (for now) in the gitops config. I think helm will silently ignore that value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ACK; sounds good.

apiVersion: v1
kind: Service
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this will be a problem, but we do need to be careful - this isn't conditional on setting the secureTenantNetwork flag. If we disabled that flag, and this resource exists, all network traffic will be blocked for all tenants.

Can we use a non-NetworkPolicy resource? A ConfigMap or Secret?

Copy link
Contributor

openshift-ci bot commented Jun 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ebensh, mclasmeier

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

@vladbologa vladbologa merged commit 2c327e6 into main Jun 21, 2024
6 checks passed
@vladbologa vladbologa deleted the evan/rox-23555-remove-egress-proxy branch June 21, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants