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

🐛 Correctly sets DNS imageRepository when custom repositories are used #2832

Merged

Conversation

gab-satchi
Copy link
Member

@gab-satchi gab-satchi commented Mar 31, 2020

Which issue(s) this PR fixes
Fixes #2810

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2020
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

Overall this is a good step, although I wonder if we can find / build a regular expression and validate each input?

Maybe @randomvariable knows where can get a good regex to parse OCI images? Then we can use the regexp package to divide the incoming string in groups and return what we need

util/image/image.go Outdated Show resolved Hide resolved
util/image/image.go Outdated Show resolved Hide resolved
util/image/image.go Outdated Show resolved Hide resolved
@vincepri
Copy link
Member

In alternative, if we can't find a good regex with groups, we can use the reference repository we're using today just for validation / normalization, then parse the strings manually where required

@gab-satchi gab-satchi force-pushed the 2810-imageRepo-subpath branch from cd063d6 to 24b30f3 Compare March 31, 2020 19:05
@gab-satchi
Copy link
Member Author

Agree with the potential of panics. That was going through my head when working on this as well. My regex-fu isn't that good but I think the reference library uses them as well. So might just be able to get it from there.

@randomvariable
Copy link
Member

randomvariable commented Mar 31, 2020

The reference library has a bunch of regexes at https://github.com/docker/distribution/blob/master/reference/regexp.go

@vincepri
Copy link
Member

For this iteration, I'd suggest to build these higher level functions on top of the reference library, but make them work like we need.

@gab-satchi gab-satchi force-pushed the 2810-imageRepo-subpath branch from 24b30f3 to 8dc514a Compare April 2, 2020 13:21
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2020
@gab-satchi
Copy link
Member Author

@vincepri updated to just use the constant in the file that's used to search for the deployment.

@vincepri
Copy link
Member

vincepri commented Apr 2, 2020

code changes lgtm, should we retitle the PR and tackle the image library separately?

@gab-satchi gab-satchi force-pushed the 2810-imageRepo-subpath branch from 8dc514a to fa18c56 Compare April 2, 2020 15:07
@gab-satchi
Copy link
Member Author

/retitle Correctly sets DNS imageRepository when custom repositories are used

@k8s-ci-robot k8s-ci-robot changed the title 🐛 Adds image util to parse repo, image and tags from container images Correctly sets DNS imageRepository when custom repositories are used Apr 2, 2020
@ncdc ncdc changed the title Correctly sets DNS imageRepository when custom repositories are used 🐛 Correctly sets DNS imageRepository when custom repositories are used Apr 2, 2020
@ncdc
Copy link
Contributor

ncdc commented Apr 2, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2020
@vincepri
Copy link
Member

vincepri commented Apr 2, 2020

/milestone v0.3.4

@k8s-ci-robot k8s-ci-robot added this to the v0.3.4 milestone Apr 2, 2020
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit a9c54ec into kubernetes-sigs:master Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KCP: CoreDNS upgrade with imageRepository containing subpaths doesn't work
5 participants