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

Allow-List of supported k8s kinds should be used only when applying labels #4489

Open
dgageot opened this issue Jul 17, 2020 · 9 comments
Open
Labels
kind/feature-request priority/p2 May take a couple of releases

Comments

@dgageot
Copy link
Contributor

dgageot commented Jul 17, 2020

The imageReplacer's role is to replace image names in k8s yaml by fully qualified names. It does that by visiting the k8s resources, looking for image fields and replacing image names that it has a fully qualified name for.

Since #3833, we have an Allow-List that specifies which resource kinds can be transformed and we lost the possibility to replace image fields in any random Custom Resource. This was mainly introduced to stop applying labels to resources that had a metadata field but didn't support metadata/labels.

I think we went to far with that Allow-List and that it should apply only when we set labels on resources. (Which by the way we might stop doing)

Another case that we have broken is that we collect namespaces only on resources whose kind is in the Allow-List.

@dgageot dgageot self-assigned this Jul 17, 2020
@briandealwis
Copy link
Member

Our current approach is very indiscriminate: for the allowed resources, we change every occurrence of image or label (e.g., #3219 (comment)).

There are also cases where we munged immutable fields (#3219, #3133).

@briandealwis
Copy link
Member

We can make the allowlist extensible by specifying G(V)Ks in the skaffold.yaml.

But perhaps rather than just listing G(V)Ks, what if we also allowed specifying a JSONPath-like syntax for identifying the specific fields to be rewritten? This is really only important for images for CRDs that may specify images by some other field name. (Assuming that we shouldn't be munging labels everywhere).

@dgageot
Copy link
Contributor Author

dgageot commented Jul 17, 2020

If we were to not use the allow-list for replacing image names:

  • do you think it would better than what we have now?
  • do you think it would be more dangerous?

@briandealwis
Copy link
Member

Our image transforms seem safe: they should be no-ops unless we hit a reference to one of our artifact references.

I'd just like to have some knobs for users to tweak in case we're wrong. Maybe a configurable blocklist would be sufficient?

@dgageot
Copy link
Contributor Author

dgageot commented Jul 17, 2020

For me, a possible knob is just changing image names.

@dgageot
Copy link
Contributor Author

dgageot commented Jul 17, 2020

wdyt?

@briandealwis
Copy link
Member

We have to consider the -l switch to add user labels too. This is also used by the IDEs.

@dgageot
Copy link
Contributor Author

dgageot commented Jul 18, 2020

For labels, we could keep the allow list until we find better. The idea is to stop using that list to apply image names and collect namespaces.

@briandealwis
Copy link
Member

If we can do a configurable group-kind block-list — so that users can unbreak themselves — I'm good with it.

@gsquared94 gsquared94 added kind/feature-request priority/p2 May take a couple of releases labels Aug 5, 2020
@nkubala nkubala added this to the Icebox [P2+] milestone Sep 1, 2020
@nkubala nkubala removed this from the Icebox [P2+] milestone May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request priority/p2 May take a couple of releases
Projects
None yet
Development

No branches or pull requests

4 participants