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

Watch routes and ingress, ensure we're only using one #1169

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Sep 18, 2024

  • If routes are installed in the cluster, add a watch for them
  • If we are using routes, ensure that we do not have an ingress.
  • Remove manually set certificates on routes and use the cluster default

CP4AIOPS-6311

Comment on lines +76 to +79
route.Spec.TLS.Certificate = ""
route.Spec.TLS.Key = ""
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if people would want to manually set certificates on routes, but this would remove them and force the use of the cluster certificates.

Copy link
Member

Choose a reason for hiding this comment

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

Can you comment as such right on the code?

Copy link
Member

Choose a reason for hiding this comment

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

What happens if customer provides a custom tls cert secret in the IMInstall CR? Will custom cert still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if customer provides a custom tls cert secret in the IMInstall CR? Will custom cert still work?

No, this would force using the certificates on the cluster.
Wouldn't they install that certificate on the cluster?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this works. Probably. I am ok with the fix

Comment on lines +284 to +286
} else {
logger.Info(fmt.Sprintf("Skipping watch for Routes! %s", err))
}
Copy link
Member

@Fryguy Fryguy Sep 19, 2024

Choose a reason for hiding this comment

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

Should this conditional also be on the other 2 if clauses if they fail?

Copy link
Member 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 those would fail, but I added some anyway.

Copy link
Member

@kschanrtp kschanrtp left a comment

Choose a reason for hiding this comment

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

LGTM

Allow changes to our Ingress and Route to trigger a reconcile.
Routes are not available outside of OpenShift and the controller setup
will fail if we try to watch resources that are not registered to the
cluster.  So, we first need to see if we get an error if we try to list
routes.

CP4AIOPS-6311
Sometimes when we are initially created we go down the ingress path rather
than the routes.  Then certificates are created and we switch to routes
instead.  Unfortunately nothing was cleaning up the ingress (and potentially
associated route httpd-xxxxx if we are running in OpenShift.)  This ensures
that if we are using routes, we clean up any ingress that we may have created
in the past.

CP4AIOPS-6311
In the past we had set certificates on routes, since ManageIQ#1161 we want to use
the cluster default certificates.  So, if the route existed before that
was merged, we need to remove the certificate and key fields.

CP4AIOPS-6311
@Fryguy Fryguy merged commit fc227fa into ManageIQ:master Sep 20, 2024
2 checks passed
@bdunne bdunne deleted the watch_routes_ingress branch September 23, 2024 13:41
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.

3 participants