FF-160/download docker app by mike-sul · Pull Request #9 · foundriesio/aktualizr-lite

mike-sul · 2020-05-25T16:23:39Z

No description provided.

mike-sul · 2020-05-25T18:13:38Z

@doanac I enabled clang tidying again by introducing our own testing docker container based on ubuntu.bionic.
I am going to add some negative tests for the introduced functionality.

doanac

I've done a first pass. I still need to read a bit more, but this should get things going.

One thing - that would be nice: can we separate the ninja format changes from the actual code changes. Maybe - just merge ninja-format problems now?

doanac · 2020-05-26T02:53:00Z

 if ! docker image inspect $container 2>/dev/null 1>/dev/null ; then
 	echo "Container not found, doing one time docker build"
-	docker build -t $container -f ./aktualizr/docker/Dockerfile.debian.testing .
+	docker build -t $container -f ./docker/Dockerfile.ubuntu.bionic .


should we go ahead and jump to 20.04, or would that risk some incompatibilities with aktualizr?

let's see what the aktualizr team will do about it if anything, I would stay with whatever they will come up with because if we just switch to something higher we might end-up with some incompatibilities as you stated. I consider it as a temporal solution.

doanac · 2020-05-26T02:54:15Z

+RUN NOCONFIGURE=1 ./autogen.sh
+RUN ./configure CFLAGS='-Wno-error=missing-prototypes' --with-libarchive --disable-gtk-doc --disable-gtk-doc-html --disable-gtk-doc-pdf --disable-man --with-builtin-grub2-mkconfig --with-curl --without-soup --prefix=/usr
+RUN make VERBOSE=1 -j4
+RUN make install


I've always been curious why ostree is pinned to a specific source revision while the other dependencies are not. You have any insights on that? Just curious.

It has to do with the hack around testing the ostree based package manager (LD_PRELOAD and those two overridden methods ), the hack doesn't work with any ostree version.

doanac · 2020-05-26T03:32:10Z

+    auto endpoint_pos = registry_conf.auth_creds_endpoint.find(treehub_endpoint);
+    if (endpoint_pos != std::string::npos) {
+      registry_conf.auth_creds_endpoint.replace(endpoint_pos, registry_creds_endpoint.length(),
+                                                registry_creds_endpoint);


I guess the else branch here helps with testing?

n/m - i see how you handle this below now.

I am wondering if we should have this config defined. It pins us to hub.foundries.io. However - docker makes this discoverable. eg:

$ curl -v https://hub.foundries.io/v2/msul-dev01/shellhttpd/manifests/latest ... < HTTP/1.1 401 Unauthorized ... < Www-Authenticate: Bearer realm="https://hub.foundries.io/token-auth/",service="registry",scope="repository:msul-dev01/shellhttpd:pull"

doanac · 2020-05-26T03:37:04Z

+  if (split_pos == std::string::npos) {
+    throw std::invalid_argument("Invalid App URI: '@' not found in " + uri);
+  }
+


should we do some assertion that uri starts with hub.foundries.io?

Can't it be different than hub.foundries.io, e.g. test mocks, tests, on prem deployment. I suppose it can be verified/checked if it's inline with what is defined in the config (registry_base_url).

this all fits in with my other comment above about just letting docker's api tell us what to authenticate with.

Maybe I just check if the App URI hostname matches the registry_base_url's hostname here, and check if the registry_base_url is equal to https://hub.foundries.io at the configuration time.
Another option is to get rid off base_url, auth_token_endpoint, repo_base_url configuration variables at all and extracts registry hostname from App URI, although '/token-auth/' and /v2/ has to be hardcoded then. BTW, /token-auth/ is hardcoded in the docker compose download implementation, too.

I think getting rid of them is fine for now. "v2" can be hard-coded. I guess I did "auth-token" incorrectly in my docker-compose patch. The "Www-Authenticate:" header should tell the client where to go. For docker, I noticed my logic would be broke as the do:

Www-Authenticate: Bearer realm="https://auth.docker.io/token",service="registry.docker.io"...

doanac · 2020-05-26T14:56:17Z

+  return "authorization: basic " + encoded_auth_secret;
+}
+
+std::string RegistryClient::getBearerAuthHeader(const std::string& repo) const {


this might be too painful for now and we should just keep it like this with a TODO. However, docker provides a way to find credentials. In theory, on LmP, you look at /usr/lib/docker/config.json and then $HOME/.docker/config.json for auths and credHelpers. If "auths" you just use the value provided. If "credsHelpers", you call that program and use its stdout.

Maybe we assert hub.foundries.io for now though and because its hub.fio, we know how to handle this w/o using the credsHelper.

The credHelpers relies on aktaulizr-get. What's the point to go from aktualizr to the docker stuff which in turn comes back to aktualizr if 'we' are aktualizr and go straight to the auth service.
It's not hub.foundries.io, it's https://ota-lite.foundries.io:8443/hub-creds/.

what i was trying to say here is that: we should really make this generic and not tied to hub.foundries.io. maybe eventually though since its a bit more work. in the case of hub.foundries.io, we could skip looking up the cred helper since we know how to do that.

what's nice about the docker-compose implementation is that it could download the archive from any docker registry with any credentials docker supports.

but since this is tied to hub.foundries.io, I think we can skip the configuration data and just hard-code to hub.foundries.io and raise an error if its not a hub.foundries.io app. In the future we could add logic to support other docker registries.

I see what you mean. Still, we can change our registry's hostname overtime or deploy additional one with the same way of getting creds to it (via https://ota-lite.foundries.io:8443/hub-creds).

doanac · 2020-05-26T15:12:49Z

  }

-  bool fetch(const std::string &app_uri) {
+  bool download(const std::string& app_uri);


I'm not against breaking these out. However, it confused my eyes when I first saw this. Maybe you should create a change before this one that breaks out all the methods of ComposeApps to be like this. Then when you do this change it won't look so different from everything else.

I just moved it (download) to the private methods

Signed-off-by: Mike Sul <[email protected]>

mike-sul · 2020-05-27T13:13:56Z

@doanac I removed the registry's configuration params. I didn't implement the auth request discovery as it requires changing in the aktualizr's HTTP client or developing our own, not sure if it's justifiable at the current moment provided that we don't support other than our Registry.

doanac · 2020-05-27T14:47:27Z


+// TODO: restructure composeappmanager.* so it contains just the manager implemenation
+// ComposeApp and Docker::* stuff should be moved into separate source files
+


I agree. I think a docker.cc file for the implementation would be nice. This is only going to grow in complexity.

mike-sul · 2020-05-27T14:50:17Z

Checked this branch on RPi and QEMU, it looks like it works as expected.

doanac · 2020-05-27T15:03:25Z

    docker_prune = val != "0" && val != "false";
  }
+
+  // Docker Registry Configuration (TODO: to be removed)


Can we just remove this stuff and hard-code it in getBasicAuthHeader() where we already have the big TODO's? I'm hoping this will never leak into someone's sota.toml. Or do you need someway to intercept things for unit tests?

What about the on-premises deployment? In this case, the base URL will be different than 'https://ota-lite.foundries.io:8443'.

ugh - your right. l guess we should keep the logic that discovers it - lets drop the config value method though.

What is the config value method?
I guess you mean that we just deduce the auth base url from ostree_server url and append hub-creds to it?

correct. we deduce from ostree_server

Removing the ability to configure the auth URL and just deducting it from the ostree/treehub URL is fine.
But, removing this param auth_creds_endpoint and moving the deduction logic in getBasicAuthHeader is not fine. First of all, we need to pass at least one param to RegistryClient anyway (either the treehub URL or already deducted auth URL), secondly if we do deduction in getBasicAuthHeader then we will do it for each request while this URL is static and should be evaluated once at startup.

doanac · 2020-05-27T15:10:00Z

i think this looks pretty good now. Here's how i think we should proceed to merge:

lets do a new PR to foundriesio/aktualizr to pull in the Docker fix. ie to do a [fiofromlist] patch with advancedtelematic/aktualizr@85776e8
lets do a new PR to aktualizr-lite that:
a) pulls in the new foundriesio/aktualizr
b) fixes whitespace
update this PR or do a new PR with this stuff minus all the other noise.
i think breaking out the new docker stuff into docker.[h/cc] files would be great.

let me know if you need help with 1.

mike-sul · 2020-05-27T15:21:37Z

3. update this PR or do a new PR with this stuff minus all the other noise.

I opt for a new PR, this one has already too many changes.

mike-sul · 2020-05-27T15:23:18Z

I suppose we need to make corresponding changes in lmp-device-register to configure aklite/sota.toml with compose_apps package manager.

doanac · 2020-05-27T15:26:05Z

I suppose we need to make corresponding changes in lmp-device-register to configure aklite/sota.toml with compose_apps package manager.

eventually. i think we are a few weeks away from that. we need to first get everyone's personal factories using this (and we can update the fioctl devices config updates command to move each device) first. once everyone is using it and happy we can make this the new default.

we'll also have to update the partner-setup repo to define things using docker-compose rather than .dockerapp

mike-sul · 2020-05-27T15:29:38Z

eventually. I think we are a few weeks away from that.

Why not create a branch and PR right now, so we can test the end-to-end flow as it would happen in real-life starting from bitbaking.

doanac · 2020-05-27T15:33:09Z

that's fine.

Signed-off-by: Mike Sul <[email protected]>

mike-sul · 2020-05-27T18:54:32Z

I rebased this branch on foundriesio/aktualizr#38 and removed the commit that introduces customer docker container for testing.

doanac · 2020-05-28T03:39:38Z

looks good.

mike-sul requested a review from doanac May 25, 2020 16:23

mike-sul force-pushed the FF-160/download-docker-app branch from 304ef30 to 5e091cd Compare May 25, 2020 18:04

doanac reviewed May 26, 2020

View reviewed changes

Switch to Left Pointer Alignment

d869158

Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the FF-160/download-docker-app branch from f89e4fa to 98d3a3c Compare May 26, 2020 19:17

doanac reviewed May 27, 2020

View reviewed changes

mike-sul added 5 commits May 27, 2020 21:43

Compose App download initial implementation

ff70cf8

Signed-off-by: Mike Sul <[email protected]>

Compose App download negative tests

84bac6c

Signed-off-by: Mike Sul <[email protected]>

Remove Compose App archive after extraction

b00ae82

Signed-off-by: Mike Sul <[email protected]>

Get rid of Registry's conf variables

909fa70

Signed-off-by: Mike Sul <[email protected]>

Check available storage space before App download

728c0d3

Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the FF-160/download-docker-app branch from 761dbe8 to 728c0d3 Compare May 27, 2020 18:51

mike-sul mentioned this pull request May 27, 2020

Configure aklite with compose apps foundriesio/lmp-device-register#17

Merged

doanac marked this pull request as ready for review May 28, 2020 03:39

mike-sul merged commit c3bc433 into master May 28, 2020

mike-sul deleted the FF-160/download-docker-app branch May 28, 2020 07:57


		// TODO: restructure composeappmanager.* so it contains just the manager implemenation
		// ComposeApp and Docker::* stuff should be moved into separate source files

Conversation

mike-sul commented May 25, 2020

Uh oh!

mike-sul commented May 25, 2020

Uh oh!

doanac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-sul commented May 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-sul commented May 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doanac commented May 27, 2020

Uh oh!

mike-sul commented May 27, 2020

Uh oh!

mike-sul commented May 27, 2020

Uh oh!

doanac commented May 27, 2020

Uh oh!

mike-sul commented May 27, 2020