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

feat: Make Argo UI aware of OpenShift objects#4601 #5096

Closed
wants to merge 1 commit into from

Conversation

iam-veeramalla
Copy link
Member

@iam-veeramalla iam-veeramalla commented Dec 21, 2020

Signed-off-by: iam-veeramalla abhishek.veeramalla@gmail.com

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • My build is green (troubleshooting builds).

This PR will make Argo UI aware of OpenShift objects. It provides details such as the route url and services attached to route. This feature is currently available for ingress.

Fixes #4601 #7052

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@iam-veeramalla
Copy link
Member Author

@alexmt can you review this ?

@jessesuen
Copy link
Member

I think @jgwest and/or @wtam2018 would be good ones to review this

@wtam2018
Copy link
Contributor

@iam-veeramalla do you mind to create an issue to describe the enhancement? It would be nice to include some screen shots to illustrate the enhancement.

@iam-veeramalla
Copy link
Member Author

@iam-veeramalla do you mind to create an issue to describe the enhancement? It would be nice to include some screen shots to illustrate the enhancement.

Sure @wtam2018

@iam-veeramalla
Copy link
Member Author

iam-veeramalla commented Dec 22, 2020

@iam-veeramalla do you mind to create an issue to describe the enhancement? It would be nice to include some screen shots to illustrate the enhancement.

Sure @wtam2018

Argo renders the Ingress url for Ingress resource deployed in k8s and shows it in the ingress object under network map, This will help users to access their applications(pods) directly from the Argo UI. This feature is not available for openshift routes as Argo does not identify/render/provide any route details. Also, by the nature of openshift, it will convert any ingress resource into routes. So, Argo does not render the url, health details of k8s ingress or openshift routes deployed in openshift. So, the network map is technically not openshift friendly.

This enhancement will allow users to access the openshift route url from Argo UI. This will make user experience better as they don't have to describe route using oc cli and frame the route url(typically host+path) to access their applications. This feature is already available for ingress and istio virtual service in k8s platform. This feature is added as per the requirement in issue #4601

Screenshot 2020-12-22 at 4 29 21 PM

Another screenshot to explain how this enhancement will provide
Screenshot 2020-12-22 at 4 50 05 PM

Openshift route object identified by Argo does not even provide the health details. I will create an issue for that and add it as a follow up enhancement to this.

Please let me know, If you need more details.

@wtam2018 @jgwest

@wtam2018
Copy link
Contributor

Fixes #4601

@wtam2018
Copy link
Contributor

I'll take a look.

Copy link
Contributor

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

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

#4601 also propose to support DeploymentConfig . Should we address it? We could do it in a different PR.

@@ -170,6 +191,45 @@ func populateIngressInfo(un *unstructured.Unstructured, res *ResourceInfo) {
res.NetworkingInfo = &v1alpha1.ResourceNetworkingInfo{TargetRefs: targets, Ingress: ingress, ExternalURLs: urls}
}

func populateRouteInfo(un *unstructured.Unstructured, res *ResourceInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this function to populateOpenShiftRouteInfo.

@@ -170,6 +191,45 @@ func populateIngressInfo(un *unstructured.Unstructured, res *ResourceInfo) {
res.NetworkingInfo = &v1alpha1.ResourceNetworkingInfo{TargetRefs: targets, Ingress: ingress, ExternalURLs: urls}
}

func populateRouteInfo(un *unstructured.Unstructured, res *ResourceInfo) {
route := getRoute(un)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is more like getOpenShiftIngress()?

Namespace: un.GetNamespace(),
Name: fmt.Sprintf("%s", backend["name"]),
}] = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is only one to backend service per route but there can be a list of alternateBackends. Please see https://docs.openshift.com/container-platform/3.6/architecture/networking/routes.html#alternateBackends


if backend, ok, err := unstructured.NestedMap(un.Object, "spec", "to"); ok && err == nil {
targetsMap[v1alpha1.ResourceRef{
Kind: kube.ServiceKind,
Copy link
Contributor

@wtam2018 wtam2018 Dec 24, 2020

Choose a reason for hiding this comment

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

Currently, Kind can only be Service but should we not hardcode here? Perhaps, use backend["kind"] instead?

res := make([]v1.LoadBalancerIngress, 0)
for _, item := range route {
if lbIngress, ok := item.(map[string]interface{}); ok {
if hostname := lbIngress["routerCanonicalHostname"]; hostname != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

urls = append(urls, url)
}

res.NetworkingInfo = &v1alpha1.ResourceNetworkingInfo{TargetRefs: targets, Ingress: route, ExternalURLs: urls}
Copy link
Contributor

@wtam2018 wtam2018 Dec 24, 2020

Choose a reason for hiding this comment

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

I think we need to handle edge cases when we are unable to extract backend services, ingress, and urls.

@dlmorais-pbh
Copy link

#4601 also propose to support DeploymentConfig . Should we address it? We could do it in a different PR.

One thing I know that Argo supports for Deployment, but not for DeploymentConfig, is the ability to trigger a rollout by using the "three dots menu" on Deployment object.

And there is also the icon at object's box.

@mbecca
Copy link

mbecca commented Aug 10, 2021

any update for this PR?

@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #5096 (619de1c) into master (45b3e48) will increase coverage by 0.08%.
The diff coverage is 82.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5096      +/-   ##
==========================================
+ Coverage   40.82%   40.90%   +0.08%     
==========================================
  Files         133      133              
  Lines       18154    18189      +35     
==========================================
+ Hits         7411     7440      +29     
- Misses       9672     9675       +3     
- Partials     1071     1074       +3     
Impacted Files Coverage Δ
controller/cache/info.go 65.76% <82.85%> (+3.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45b3e48...619de1c. Read the comment docs.

@iam-veeramalla
Copy link
Member Author

any update for this PR?

I started to work back on this. I will probably get a PR for this by end of the week :)

@pasha-codefresh
Copy link
Member

@iam-veeramalla could you please update on this? Do you need some help?

@crenshaw-dev
Copy link
Member

Closing for now, @iam-veeramalla feel free to reopen when ready. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Argo UI aware of OpenShift objects, such as Route and DeploymentConfig
7 participants