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-27980: Support argocd reconciliation for routes #2197

Merged
merged 12 commits into from
Feb 14, 2025

Conversation

kovayur
Copy link
Contributor

@kovayur kovayur commented Feb 11, 2025

Description

Add support for the routes reconciliation on ArgoCD.
The feature can be now enabled by setting the value centralIngressEnabled to true in the gitops config.
If disabled (default), reconciliation should be performed in fleetshard-sync mode as before.
If enabled, the routes created by fleetshard-sync are deleted and the reconciliation will be performed by tenant-resources (ArgoCD).

Next steps:

  1. Enable the feature on dev environment to make E2E tests pass with the new setup.
  2. Enable the feature on the rest environments and effectively delete the routes reconciled by fleetshard-sync.
  3. Set centralIngressEnabled to true by default in tenant-resources
  4. Cleanup the fleetshard-sync source code by removing the switch and code which is responsible for the routes reconciliation.

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

@kovayur kovayur force-pushed the yury/ROX-27980-routes-argo branch from 1e9d35d to f69a0f9 Compare February 12, 2025 11:22
Comment on lines 1053 to 1056
if apiErrors.IsNotFound(err) {
centralTLSSecretFound = false // pragma: allowlist secret
}
return centralTLSSecretFound, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems there is something wrong here. If the secret doesn't exist, it will never call ensureSecretExists. Or am I missing something?

Suggested change
if apiErrors.IsNotFound(err) {
centralTLSSecretFound = false // pragma: allowlist secret
}
return centralTLSSecretFound, err
if apiErrors.IsNotFound(err) {
centralTLSSecretFound = false // pragma: allowlist secret
} else {
return false, err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of centralTLSSecretFound is to skip the further reconciliation instead of returning the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we'll return the error in any case, so the code was correct. I simplified it a bit.

@kovayur kovayur force-pushed the yury/ROX-27980-routes-argo branch from 0807965 to 13baa61 Compare February 13, 2025 17:43
func (r *CentralReconciler) ensureCentralCASecretExists(ctx context.Context, centralNamespace string) (centralTLSSecretFound bool, err error) {
centralTLSSecret, err := r.getSecret(centralNamespace, k8s.CentralTLSSecretName)
if err != nil {
return !apiErrors.IsNotFound(err), err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return !apiErrors.IsNotFound(err), err
return false, err

Copy link
Collaborator

@ludydoo ludydoo left a comment

Choose a reason for hiding this comment

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

Just this one last comment mentioned above, otherwise 🔥

@kovayur kovayur force-pushed the yury/ROX-27980-routes-argo branch from 96906c1 to 9b475f6 Compare February 14, 2025 10:54
@kovayur kovayur requested a review from ludydoo February 14, 2025 10:55
@openshift-ci openshift-ci bot added the lgtm label Feb 14, 2025
Copy link
Contributor

openshift-ci bot commented Feb 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kovayur, ludydoo

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

@kovayur kovayur merged commit 247ca20 into main Feb 14, 2025
15 checks passed
@kovayur kovayur deleted the yury/ROX-27980-routes-argo branch February 14, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants