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

Add proposal for packaging registry authentication model #208

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ewrenn8
Copy link
Contributor

@ewrenn8 ewrenn8 commented Aug 10, 2021

No description provided.


# Problem

Some package repository bundles and package bundles are hosted in registries
Copy link
Contributor

Choose a reason for hiding this comment

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

can we simplify this initial sentence, e.g. "Some assets are hosted in registries that require authentication".

dunno what the best word is, assets / artifacts / bundles / image-like-objects, but there's some sensibility to having an easy-reader as the lead-in for the rest of the contents.

Some package repository bundles and package bundles are hosted in registries
that require authentication. Users interacting with packaging APIs should have
minimal interaction with registry credentials for the sake of user experience,
so that: only users interacting with package repositories should have to provide
Copy link
Contributor

Choose a reason for hiding this comment

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

here this looks like a bulleted list got sqaushed?

  • only users interacting with package repositories should have to provide registry credentials
  • users creating PackageInstall CR should not have to think
    about registry creds
  • users interacting with CRs installed via a package should
    not have to think about registry creds

about registry creds users interacting with CRs installed via a package should
not have to think about registry creds

Why isn't node pre-auth enough? kapp-controller uses imgpkg inside its pod to
Copy link
Contributor

Choose a reason for hiding this comment

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

this paragraph feels funny in the intro, the question is almost a subheading for a different section. The parenthetical is confusing to me and I can't tell if i'm not following it due to lack of domain knowledge or if it's really hard to read? to attempt to paraphrase it: "some types of pre-auth would work, but here's an equality between two phrases and joe just can't figure out where the left-hand side of the equality starts, nor what the right hand is implying")

What if for the purposes of an introduction, this paragraph were just it's last sentence with a short lead in, like, "we need to handle registry authentication as a first-class feature without relying on node preauthorization, because node pre-auth may not be possible on some clusters and may not fully service all lifecycle phases of a package repository."

# Cases

### User wants to use package which fetches contents from private registry
* User has credentials which allow access private registry
Copy link
Contributor

Choose a reason for hiding this comment

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

missing one of those little joiner words, e.g. "... access to private registry"

* User has credentials which allow access private registry
* User adds PackageRepository CR for package to their cluster
* User creates PackageInstall CR to install package on their cluster
* Package is installed successfully
Copy link
Contributor

Choose a reason for hiding this comment

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

this last one is maybe unclear. -- why do i need to fetch contents from a private registry after the package is installed successfully? is that because it's then going to trigger an App install? if so let's make that slightly more explicit somehow. if I read this as "everything is done and it worked". then I can't tell why we would need credentials to fetch anything else

* install secretgen-controller alongside kapp-controller
* secretgen-controller would watch for placeholder secrets and populate them
with combined registry creds
* kapp-controller would take into account populated placeholder secrets when
Copy link
Contributor

Choose a reason for hiding this comment

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

would ... kapp controller uses populated placeholder secrets...

(generated based on Package CR) is fetching configuration contents which means
that the user does not have an opportunity to configure it.

Explicit configuration: not currently possible (TBD should we expose
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 a future goal?

secretgen.carvel.dev/image-pull-secret: ""
type: kubernetes.io/dockerconfigjson
data:
.dockerconfigjson: e30=
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth adding a comment here or a note below that base64.encode("{}") is the minimal value for this field -- empty string will not suffice.

# Future Details

## Future Enhancements
Future enhancements (aka more magic) that may make package author lives easier so that they do not need to include placeholder secret in their own configuration:
Copy link
Contributor

Choose a reason for hiding this comment

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

missing 's for "package author's lives"


# Future Details

## Future Enhancements
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels more like "other design options" than future enhancements?

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

Successfully merging this pull request may close these issues.

None yet

3 participants