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

Implement /uuid API endpoint #38

Open
rvs opened this issue Aug 13, 2020 · 10 comments
Open

Implement /uuid API endpoint #38

rvs opened this issue Aug 13, 2020 · 10 comments

Comments

@rvs
Copy link
Contributor

rvs commented Aug 13, 2020

As per conversation in lf-edge/eve#1284 we need to update Adam ASAP

@giggsoff
Copy link
Collaborator

Hi, @rvs. Someone seems to have forgotten to run make proto. I cannot find any updates to implement UuidResponse here.

@rvs
Copy link
Contributor Author

rvs commented Aug 13, 2020

Good point @giggsoff -- I was expecting @cshari-zededa to do that, but if not -- can you please submit a PR?

@cshari-zededa
Copy link

Good point @giggsoff -- I was expecting @cshari-zededa to do that, but if not -- can you please submit a PR?

@rvs @giggsoff raised lf-edge/eve#1304 for the same

@giggsoff giggsoff mentioned this issue Aug 14, 2020
@deitch
Copy link
Contributor

deitch commented Aug 14, 2020

To be clear @rvs , is /uuid a v2-only endpoint? Based on what @giggsoff pointed out that it only appears in APIv2.md and not in APIv1.md, is it a v2 only endpoint?

@rvs
Copy link
Contributor Author

rvs commented Aug 14, 2020

Well, it is V2 in so much as that it was introduced when we declared V1 to be legacy. However, according to @cshari-zededa comment here we need to backport it into V1: lf-edge/eve#1284 (comment)

At least that's my read -- @cshari-zededa -- can you please confirm?

@cshari-zededa
Copy link

Well, it is V2 in so much as that it was introduced when we declared V1 to be legacy. However, according to @cshari-zededa comment here we need to backport it into V1: lf-edge/eve#1284 (comment)

At least that's my read -- @cshari-zededa -- can you please confirm?

@rvs @deitch @giggsoff @eriknordmark
Yes, the /uuid API endpoint requirement came up because, with attestation, V2 "/config" will start checking for integrity-token, which will break zedclient's logic of getting UUID through /config. V1 /config will not do the integrity-token check, so in that sense, V1 /config is equivalent to the new /uuid endpoint from zedclient's POV.

So we have the following options:

  1. Implement V2 /uuid, and don't implement V1 /uuid - With this, zedclient will need to have the logic of checking if "Force-V1" is set, and then use /config, else use /uuid
  2. Implement both V1 & V2 /uuid - With this, zedclient will always use /uuid, and will just drop using /config endpoint from its scope of operations.

@deitch
Copy link
Contributor

deitch commented Aug 16, 2020

I have no principled objection to adding /uuid as a v1 endpoint, but provided two conditions:

  1. We document it as part of v1
  2. We make /uuid, when in v1, use the v1 auth scheme (no AuthContainer, etc.)

The latter is what drove my question. @giggsoff started to implement it, but it had a different auth scheme, which didn't make sense to me, until I realized it was part of v2.

Expanding on your options to include the adam implementation part:

  1. "Implement V2 /uuid, and don't implement V1 /uuid" - in this case we don't implement /uuid in adam until v2 is implemented
  2. "Implement both V1 & V2 /uuid - With this, zedclient will always use /uuid, and will just drop using /config endpoint from its scope of operations" - also fine, but subject to the 2 conditions above: document it in APIv1.md; uses regular auth scheme

I think the first option is cleaner, as we are trying to get past v1 anyways, but whatever this group thinks is fine with me.

@eriknordmark
Copy link
Contributor

I think the first option is cleaner, as we are trying to get past v1 anyways, but whatever this group thinks is fine with me.

I think the first option is cleaner. And it means we should revisit having the client cert in both the authcontainer and the uuid payload @cshari-zededa

@rvs
Copy link
Contributor Author

rvs commented Aug 18, 2020

So... what's a consensus here? Are we going with "we don't implement /uuid in adam until v2 is implemented"? Wouldn't that require a stopgap to be put into EVE?

@deitch
Copy link
Contributor

deitch commented Aug 18, 2020

I've no objection to implementing one if it's truly v1 compliant. As soon as we start doing the authcontainer business, it's v2, so let's just figure it out and implement that, and defer uuid

Why would it require eve backfill support? Isn't uuid v2 only?

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

No branches or pull requests

5 participants