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

Cc 7145 hcp link status api #20330

Merged
merged 16 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions ui/packages/consul-ui/app/services/repository/hcp-link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

import RepositoryService from 'consul-ui/services/repository';
import dataSource from 'consul-ui/decorators/data-source';

export default class HcpLinkService extends RepositoryService {
@dataSource('/:partition/:ns/:dc/hcp-link')
async fetch({ partition, ns, dc }, { uri }, request) {
let result;
try {
result = (
await request`
GET /api/hcp/v2/link/global
`
)((headers, body) => {
return {
meta: {
version: 2,
uri: uri,
},
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just "magic" ™️ to make the repo service happy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes 😅

body: {
isLinked: (body.status['consul.io/hcp/link']['conditions'] || []).some(
(condition) => condition.type === 'linked' && condition.state === 'STATE_TRUE'
Comment on lines +25 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Was going to ask why we have the array methods here, but the API response does have conditions to be an array, I wonder if we'd ever see multiple contradicting conditions?

I hope not - feels like something we should handle in the backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it is how the backend responds as of now. I've just decided to check that condition is linked, just in case we will have some other stuff added in future.

But I totally agree, that as of now we don't assume to have something else to be returned from the endpoint

),
},
headers,
};
});
} catch (e) {
// set linked to false if the global link is not found
if (e.statusCode === 404) {
result = Promise.resolve({ isLinked: false });
Comment on lines +33 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the comment!

} else {
result = Promise.resolve(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

So if we get an error (auth related or anything really), null would probably act as an isLinked: false as in code if we tried to do a

{{#if hcpLlink.isLinked}}
  Say cluster is linked
{{else}}
  <DisplayBanner />
{{/if}}

Thinking aloud, is that what we'd want to do?

  • If we fail to fetch this result because our ACL token doesn't have enough permissions - this would return notLinked - which is okay - since we have code ensuring we only display banner if user has permission to link
  • If we fail for other reasons - I think we should probably check that we don't have a definitive result to the is this cluster linked in the hcp-link-statusservice, which is what you were saying with repo return null for the result object (to make it possible to understand when some other system/auth error raised) I think

So I think this is fine/great! However we hook this up to the hcp-link-status, we'll probably have to handle that there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, the idea was to add some state, where we could see that hcpLink could not actually be observed as linked or not. there are other ways to do it, for sure, like to set {isLinked: null} or so.

So I open to discuss it

}
}
return result;
}
}
38 changes: 20 additions & 18 deletions ui/packages/consul-ui/app/templates/application.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,18 @@

{{#if (not-eq route.currentName 'oauth-provider-debug')}}

{{! redirect if we aren't on a URL with dc information }}
{{! redirect if we aren't on a URL with dc information }}
{{#if (eq route.currentName 'index')}}
{{! until we get to the dc route we don't know any permissions }}
{{! as we don't know the dc, any inital permission based }}
{{! redirects are in the dc.show route}}
{{! until we get to the dc route we don't know any permissions }}
{{! as we don't know the dc, any inital permission based }}
{{! redirects are in the dc.show route}}

{{! 2022-04-15: Temporarily reverting the services page to the default }}
{{! 2022-04-15: Temporarily reverting the services page to the default }}
{{did-insert
(route-action 'replaceWith' 'dc.services.index' (hash dc=(env 'CONSUL_DATACENTER_LOCAL')))
}}
{{else}}
{{! If we are notfound, guess the params we need }}
{{! If we are notfound, guess the params we need }}
{{#if (eq route.currentName 'notfound')}}
<DataSource
@src={{uri '/*/*/*/notfound/${path}' (hash path=route.params.notfound)}}
Expand All @@ -70,11 +70,11 @@
''
)
(if (can 'use nspaces') (or route.params.nspace notfound.nspace token.Namespace '') '')
as |partition nspace|
as |partition nspace|
}}

{{! Make sure we have enough to show the app chrome}}
{{! Don't show anything until we have a list of DCs }}
{{! Make sure we have enough to show the app chrome}}
{{! Don't show anything until we have a list of DCs }}
<DataSource @src={{uri '/*/*/*/datacenters'}} as |dcs|>
{{! Once we have a list of DCs make sure the DC we are asking for exists }}
{{! If not use the DC that the UI is running in }}
Expand All @@ -85,20 +85,21 @@
(hash Name=(env 'CONSUL_DATACENTER_LOCAL'))
)
dcs.data
as |dc dcs|
as |dc dcs|
}}
{{#if (and (gt dc.Name.length 0) dcs)}}

{{! figure out our current DC and convert it to a model }}
{{! figure out our current DC and convert it to a model }}
<DataSource
@src={{uri
'/${partition}/*/${dc}/datacenter-cache/${name}'
(hash dc=dc.Name partition=partition name=dc.Name)
}}
'/${partition}/*/${dc}/datacenter-cache/${name}'
(hash dc=dc.Name partition=partition name=dc.Name)
}}
as |dc|
>
{{#if dc.data}}
<HashicorpConsul
<DataSource @src={{uri '/${partition}/*/${dc}/hcp-link' (hash dc=dc.Name partition=partition name=dc.Name) }} @open={{true}} as |hcpLink|>
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit curious about this - Does this mean we'll fetch this upon application load? And this data will be accessible as the hcpLink variable in this template?

Would we still need/want this if we're only requesting this inside of the hcpLinkStatus service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. when application loading, we calling the service. but we have to call it on login as well
due to <HashicorpConsul> component has the tag structure and we need the data in sidenav and in main tags, I've put the call to application and make it open. it means that on login we will have it reloaded, same as on route change.

It is questionable though, when we need to reload it. let's say if we are linking in HCP, and does not reload page in core, I think we still should get the information if it is linked

actually it depends on the cost of calling the service, which in our case seems to be not too bad, because it uses local consul... what do you think?

<HashicorpConsul
id='wrapper'
@dcs={{dcs}}
@dc={{dc.data}}
Expand All @@ -110,10 +111,10 @@
>

{{#if error}}
{{! If we got an error from anything, show an error page }}
{{! If we got an error from anything, show an error page }}
<AppError @error={{error}} @login={{consul.login.open}} />
{{else}}
{{! Otherwise show the rest of the app}}
{{! Otherwise show the rest of the app}}
<Outlet
@name='application'
@model={{hash app=consul user=(hash token=token) dc=dc.data dcs=dcs}}
Expand All @@ -127,6 +128,7 @@
{{/if}}

</HashicorpConsul>
</DataSource>
{{/if}}
</DataSource>
{{/if}}
Expand All @@ -135,7 +137,7 @@
{{/let}}
{{/if}}
{{else}}
{{! Routes with no main navigation }}
{{! Routes with no main navigation }}
<Outlet @name='application' @model={{hash user=(hash token=token)}} as |o|>
{{outlet}}
</Outlet>
Expand Down
16 changes: 16 additions & 0 deletions ui/packages/consul-ui/mock-api/api/hcp/v2/link/global
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"status": {
"consul.io/hcp/link": {
"conditions": [
{
"message": "Successfully linked to cluster 'organization/f53e5646-6529-4698-ae29-d74f8bd22a01/project/6994bb7a-5561-4d5c-8bb0-cf40177e5b77/hashicorp.consul.global-network-manager.cluster/mkam-vm'",
"reason": "SUCCESS",
"state": "STATE_TRUE",
"type": "linked"
}
],
"observedGeneration":"01HMA2VPHVKNF6QR8TD07KDN5K",
"updatedAt":"2024-01-16T21:29:25.923140Z"
}
}
}
Loading