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.

Sign up for GitHub

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

Jump to bottom

Fix/registry oauth2 #13296

Merged
merged 13 commits into from Sep 8, 2021
Merged

Fix/registry oauth2 #13296

merged 13 commits into from Sep 8, 2021

Conversation

ycliuhw
Copy link
Member

@ycliuhw ycliuhw commented Sep 2, 2021 •
edited

This PR introduces OCI Registry HTTP API V2 authorization via OAuth for

  • pulling jujud-operator, juju-db and charm-base images from k8s controllers, application operators, and sidecar pods;
  • fetching available controller versions to upgrade for upgrade-controller command;

The tested registries are(the rest of registries will be implemented and tested in following PRs):

Registry Public Private
azurecr.io
docker.io ✅ ✅
ecr.aws
gcr.io
ghcr.io ✅
registry.gitlab.com ✅
quay.io

Checklist

  • Requires a pylibjuju change
  • Added integration tests for the PR
  • Added or updated doc.go related to packages changed
  • Comments answer the question of why design decisions were made

QA steps

# private registries:

# prepare credentials like below (you can grab these credentials from `~/.docker/config.json` once `docker login` succeeded)
$ cat dockerhub.json
{
    // "auth": "xxxxx==",
    "repository": "jujuqa",
    "username": "jujuqa",
    "password": "xxx",
}

$ cat ghcr.json
{
    "auth": "xxxx",
    "repository": "ghcr.io/jujuqa",
    "serveraddress": "ghcr.io",
}

$ cat gitlab.json
{
    "repository": "registry.gitlab.com/jujuqa",
    "serveraddress": "registry.gitlab.com",
    "username": "jujuqa",
    "password": "xxxx",
}

$ juju bootstrap microk8s k1 --config caas-image-repo="'$(cat dockerhub.json)'" --debug

$ juju upgrade-controller --agent-stream=develop

# public registry

$ juju bootstrap microk8s k1 --config caas-image-repo="ghcr.io/jujuqa"

$ juju upgrade-controller --agent-stream=develop

Documentation changes

Yes

Bug reference

https://bugs.launchpad.net/juju/+bug/1935830
https://bugs.launchpad.net/juju/+bug/1940820
https://bugs.launchpad.net/juju/+bug/1935953

@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 2, 2021

OCI registry API supports v2 bearer token oauth

hpidcock
hpidcock reviewed Sep 3, 2021
View reviewed changes
Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Looks promising

docker/registry/acr.go Outdated Show resolved Hide resolved
docker/registry/gcr.go Outdated Show resolved Hide resolved
docker/registry/github.go Outdated Show resolved Hide resolved
docker/registry/gitlab.go Outdated Show resolved Hide resolved
docker/registry/quay_io.go Outdated Show resolved Hide resolved
docker/registry/transports.go Outdated Show resolved Hide resolved
docker/registry/transports.go Outdated Show resolved Hide resolved
docker/registry/transports.go Outdated Show resolved Hide resolved
docker/registry/registry.go Outdated Show resolved Hide resolved
@ycliuhw ycliuhw marked this pull request as ready for review September 7, 2021 06:27
wallyworld
wallyworld approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

One change I think should be that DecideBaseURL is an internal detail of initialising each repo, not exposed as a separate function.

controller/config.go Outdated Show resolved Hide resolved
docker/auth.go Show resolved Hide resolved
docker/registry/acr.go Show resolved Hide resolved
docker/registry/acr.go Outdated Show resolved Hide resolved
docker/registry/base.go Show resolved Hide resolved
docker/registry/dockerhub.go
return c.repoDetails.ServerAddress == "" || strings.Contains(c.repoDetails.ServerAddress, "docker.io")
}

func (c *dockerhub) DecideBaseURL() error {
Copy link
Member

Choose a reason for hiding this comment

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

DecideBaseURL seems like it should be called something like Initialise or something.
Actually, the functionality here seems to pertain to the repoDetails value - should this be an internal method used to set up things whenever the repoDetails is set, ie should this be called from newDockerHub and not exposed as a separate method?

Copy link
Member Author

Choose a reason for hiding this comment

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

DecideBaseURL is exported for generating mocks and it's one of the methods called in initClient.
So DecideBaseURL is just one of the steps of Initialise.
I will move the provider integration code to internal sub-package in the following PR.

@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 8, 2021

!!build!!

@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 8, 2021

$$merge$$

3 similar comments
@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 8, 2021

$$merge$$

@wallyworld
Copy link
Member

$$merge$$

@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 8, 2021

$$merge$$

@jujubot jujubot merged commit 2dc0d60 into juju:2.9 Sep 8, 2021
@achilleasa achilleasa mentioned this pull request Sep 13, 2021
Merged
jujubot added a commit that referenced this pull request Sep 13, 2021
@jujubot
Merge pull request #13329 from achilleasa/dev-merge-2.9
a0f25a5 
#13329

This PR forward ports 2.9 into develop. The following PRs are included in this port:
 - Merge pull request #13324 from achilleasa/2.9-add-ovs-integration-test
 - Merge pull request #13328 from manadart/2.9-assess-series-upgrade
 - Merge pull request #13327 from wallyworld/azure-tests-fix
 - Merge pull request #13325 from SimonRichardson/raft-worker-errors
 - Merge pull request #13274 from SimonRichardson/lxd-network-devices-config-host-name
 - Merge pull request #13323 from jujubot/increment-to-2.9.15
 - Merge pull request #13321 from wallyworld/more-secret-metadata
 - Merge pull request #13319 from manadart/2.9-bridge-policy
 - Merge pull request #13320 from SimonRichardson/revert-lxd-changes
 - Merge pull request #13194 from juanmanuel-tirado/patch-1
 - Merge pull request #13318 from hpidcock/fix-1942948
 - Merge pull request #13314 from simondeziel/snap-ack
 - Merge pull request #13297 from achilleasa/2.9-allow-empty-openvswitch-blocks-in-netplan-config
 - Merge pull request #13317 from jujubot/increment-to-2.9.14
 - Merge pull request #13316 from hpidcock/fix-1942948
 - Merge pull request #13296 from ycliuhw/fix/registry-oauth2
 - Merge pull request #13311 from wallyworld/unitagent-missing-charm
 - Merge pull request #13315 from wallyworld/lxd-not-found-fix
 - Merge pull request #13221 from juanmanuel-tirado/status_watch_flag
 - Merge pull request #13312 from wallyworld/remove-txnwatcher-started
 - Merge pull request #13309 from kot0dama/fix-instrospection-posix-shell-2.9

The following files had merge conflicts that had to be resolved (please double-check the changes in last commit):
- caas/kubernetes/provider/bootstrap_test.go
- feature/flags.go
- scripts/win-installer/setup.iss
- snap/snapcraft.yaml
- state/pool.go
- version/version.go
- worker/uniter/relation/state_test.go
@ycliuhw
Copy link
Member Author

ycliuhw commented Sep 21, 2021

integrate more registry support, for example gcr, azure, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hpidcock hpidcock hpidcock left review comments

@wallyworld wallyworld wallyworld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ycliuhw @wallyworld @hpidcock @jujubot