feat: add multi-level-repo support to arbitrary repositories beyond just GCR and AR #6915
Conversation
khrisrichardson
changed the title
khrisrichardson
changed the title
pkg/skaffold/docker/default_repo.go
Outdated
| func registrySupportsMultiLevelRepos(repo string) bool { | ||
| return strings.Contains(repo, "gcr.io") || strings.Contains(repo, "-docker.pkg.dev") | ||
| func registrySupportsMultiLevelRepos(repo string, multiLevelRepo *bool) bool { | ||
| return (multiLevelRepo == nil && (strings.Contains(repo, "gcr.io") || strings.Contains(repo, "-docker.pkg.dev"))) || (multiLevelRepo != nil && *multiLevelRepo) |
There was a problem hiding this comment.
I would keep this simpler,
- Always give preference to
gcr.ioanddocker.pkg.devby keeping returning true ifstrings.Contains(repo, "gcr.io") || strings.Contains(repo, "-docker.pkg.dev") - Return *multiLevelRepo if not nil.
This way, users are protected against multiLevelRepo set to false by mistake for gcr.io and docker.pkg
In
There was a problem hiding this comment.
The skaffold team is considering a revamp of skaffold config. One of the points discussed during this revamp was, does it makes sense to move default-repo and registry config to skaffold.yaml instead of global config.
Do you have any thots on this? Do you think moving this to skaffold.yaml will make the config more sharable?
There was a problem hiding this comment.
Only speaking from my experience, but here are a few uses cases to consider.
-
Developer for Open-Source Project
While this isn't nearly as prevalent as I would like, I would find it especially useful if maintainers of open-source projects included a skaffold.yaml in place of a Makefile to actuate the building and deployment of their software.
In this particular use case, it would make sense for there to be a default repository in the skaffold.yaml file to set a project wide standard. In all likelihood, however, this default repository wouldn't be writable by contributors. Were the notion of setting the default repository removed from the global config, some natural friction would surface as contributors of the project attempted to build with their private registries, which would require updating the skaffold.yaml to affect the results and remembering to undo the edit before committing any code changes.
In this particular use case, the term "default" becomes overloaded since the default for the project conflicts with the default for the contributor.
Providing a way to overlay the skaffold.yaml ala JSON or strategic merge patches similarly to kustomize without compromising the integrity of the project config could be one avenue to facilitate this use case. Again in all likelihood, a contributor could conceivably wish to set their private registry as the default across a multitude of projects, which is why the global config still has some resonance IMHO.
-
Developer for Private Enterprise
It's entirely possible that a developer for a private enterprise does all their development and testing against the production account using progressive rollout techniques. More power to them. A singular default repository works great in this scenario since there's only the production repository to contend with.
The messy reality is that developers and operators may have to contend with a smattering of repositories, possibly across multiple cloud providers and/or cloud provider accounts. In this scenario a singular default repository becomes problematic, but so long as it remains possible to target a specific repository per Kubernetes context, then things should work out alright. The huge caveat there is the assumption that consumers of skaffold are necessarily deploying to Kubernetes, which is the target audience but still worthy of consideration.
-
Operator/Consultant for Private Enterprise
Not all software is created equal. I find myself using skaffold to build both long running daemons and CI tools (KRM functions). If images with a multitude of purposes are built from the same skaffold configuration, then it may be necessary to target a multitude of repositories from the same file. Although the
requiresdirective provides a means to separate areas of concern into distinct skaffold configurations, IMHO that should be optional not required.For me personally, I like to write the CI tools so that they work in a variety of contexts (home, work, etc.) Having the flexibility to write to a multitude of repositories is key, and when building CI tools the notion of a Kubernetes context is less relevant. I happen to use the GitLab Kubernetes executor both at home and work, so maintaining the portability of images is of the utmost importance to me (thus this PR). I could imagine a consultant for a number of companies wishing to maintain this same level of portability.
Baking the default repository into the skaffold.yaml may conflict with the desire of portability when there's no notion of Kubernetes context at hand to steer the logic.
As an aside, one thing I pondered when writing this PR was whether there shouldn't be a separate data structure in ~/.skaffold/config listing the variety of repositories and their attributes. In other words, I would ideally only need to identify that a particular repository has multi-level support once even though I may use that repository in a number of kubeContexts.
There was a problem hiding this comment.
Thanks @khrisrichardson for all the thoughts. These are all valid points.
I think with the current config, the multi-level config goes well.
That said, setting expectations here.
This change won't be released for a long time. We might end up re-configuring the skaffold.yaml as part of our major config revamp and release this later in Jan or Feb.
Its upto you if you want us to commit this change on main now and then go through a refactor or,
Wait for us to design/start config revamp project and then make this feature addition.
There was a problem hiding this comment.
Let me know what you prefer.
Codecov Report
@@ Coverage Diff @@
## main #6915 +/- ##
==========================================
- Coverage 70.48% 68.82% -1.67%
==========================================
Files 515 551 +36
Lines 23150 25319 +2169
==========================================
+ Hits 16317 17425 +1108
- Misses 5776 6717 +941
- Partials 1057 1177 +120
Continue to review full report at Codecov.
|
|
FWIW I see similar failures when integration testing again the main branch. May I have some clarity on next steps? |
hmm. could be a flake. |
Fixes: #nnn
Related: Relevant tracking issues, for context
Merge before/after: N/A
Description
While the data model is a little clunky, as it would be preferable for multi-level support to be an attribute of a repository, given the one-to-one relationship of default repository to global or kube context, however, this adds configurable multi-level repository support per context.
The current behavior is that only GCR and AR support multi-level repositories, with an exemption granted if the name of the image being built has the same prefix as the default repository for the current context. Otherwise forward slashes and periods in image names are substituted with underscores.
GitLab container image registry is another repository that supports, and requires one to adhere to, multi-level image naming conventions. Permission is denied when one violates the organization/repository/nested/sub-directory naming conventions. The problem is that self-hosted registry domain names are not subject to string comparison like GCR and AR. It's easy enough to workaround this by prefixing the image name in skaffold.yaml with the default repository, but then the skaffold configuration is no longer portable. Hence this PR.
Note: The version of node was updated in the Dockerfile to support the latest version of postcss.
User facing changes (remove if N/A)
The user is now eligible to set for either the global or kube context multi-level support of the repository to which they intend to push images. The default behavior is equivalent to the behavior prior to this PR.
Follow-up Work (remove if N/A)
I was uncertain whether this feature should only be supported via config or also as a command line flag, and so there were no changes made to
flags.go.