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

make third party repo image names configurable #14815

Open
wants to merge 2 commits into
base: master
from

Conversation

6 participants
@anandkumarpatel
Copy link
Contributor

commented Jun 13, 2019

Some image names have been hardcoded in helm templates.
This PR makes all the image names configurable via helm values.

I have defaulted them to original values in their respective values.yaml to ensure backward compatibility.

why?
In my use-case, all our images have to come from our image repository and our image repository has strict naming conventions for our images. This change will allow me to change image names without having to fork and change image names in the source.

@googlebot googlebot added the cla: yes label Jun 13, 2019

@istio-testing istio-testing requested review from costinm and howardjohn Jun 13, 2019

@istio-testing

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

Hi @anandkumarpatel. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@anandkumarpatel anandkumarpatel force-pushed the anandkumarpatel:fix/make-kubectl-image-a-param branch 2 times, most recently from a7d9e9b to 77927fc Jun 13, 2019

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

/assign @sdake

@howardjohn

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@anandkumarpatel

We are moving work on istio installer into our "next gen" installer at https://github.com/istio/installer. The new installer will have first class support for Kustomize, which is perfect for your use case of changing the image name. If you use that you can just create an overlay to override the image name.

However I don't think we should make this change in the Helm templates. I know we have some images with it, but we consider it a mistake that we haven't reverted due to backwards compatibility concerns.

I completely understand that your use case requires changing them, but we can't add an option for everything or it gets too complicated. I would highly recommend you look into using https://github.com/istio/installer which is going to be alpha released with Istio 1.2 (coming very soon)

@sdake

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@howardjohn we do have a policy for the 1.2 installer that third party component's names can be customized in the template (in the unlikely event the upstream changes result in an image name change, which we have actually experienced requiring a z stream). Images that come from docker.io/istio, which we control, however, are hardcoded intentionally.

@anandkumarpatel thank you for your work and the PR. This isn't the first PR we have seen that does this work, and it probably won't be the last. As @howardjohn explained in #14815 (comment) we plan to address this in the next-gen installer (and if the stars align, deprecate this installer in the 1.3 release). As a result of the deprecation decision by the technical oversight committee and in agreement with the environments workgroup leads (@sdake and @costinm), we have effectively frozen this directory for master unless a change is required to make the installer function properly. No new features are permitted. I do respect your requirements and we are happy to make this type of functionality available in the next generation installer.

Please read https://discuss.istio.io/t/maintainers-please-read-installer-next-plans-for-1-2/2446 for more details.

Cheers
-steve

@sdake
Copy link
Member

left a comment

As per policy, would be willing to accept a PR that customizes the third party image names in the 1.2 repository. Were the Istio components themselves not hardcoded? Taking a quick look at pilot, it looks like it supports customization. Looks like the policy was applied in backward fashion... See: https://github.com/istio/istio/blob/master/install/kubernetes/helm/istio/charts/pilot/templates/deployment.yaml#L50

@costinm can you comment?

@anandkumarpatel anandkumarpatel force-pushed the anandkumarpatel:fix/make-kubectl-image-a-param branch from 77927fc to f5186dd Jun 18, 2019

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@sdake updated 3rd party images. Would you accept a PR for others as you suggested with https://github.com/istio/istio/blob/master/install/kubernetes/helm/istio/charts/pilot/templates/deployment.yaml#L50 ?

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Friendly Monday morning poke :)
@sdake @costinm

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Few concerns - and one recommendation if you really need this.

The main problem we have with installer (and possibly Istio) is that we have far too many options for anyone to fully understand. And almost none of the options are tested - including this PR. We do require tests for code changes - but apparently install and config which can have a far bigger impact is forgotten.
And the fact we have so many documented options is also used as an excuse ( nobody can test that many options and combinations ).

I understand you use case - and that of most other option added. And we do want to support them,
however not by adding every single word from a k8s template to values.yaml.

First - please make the changes in the istio/install repository for the new installer. After that if needed we can also backport to old installer ( in case we'll still ship it in 1.3).

Second - consider using the kustomize step of the new installer, if possible. We now support a combination of template and kustomize, by kustomizing after the template rendering. If you use a CI/CD system with 'helm template' - it's just an extra command to apply kustomize. This has first class support for overriding images and tags, and it's pretty clean.

If you can't use kustomize ( for example use helm tiller ) - it is possible to have customisable fields in the templates that are not exposed in values.yaml. Like with Kustomize - many things can be changed by advanced users without exposing it to typical users. To do that - just use the default value in the helm template.

The goal is to have a very small 'values.yaml' for most users, with actual tests for the few values we need there - and use kustomize for advanced uses.

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@costinm I understand your concerns. I made the PR for the installer istio/installer#253.

We can't use kustomize right now since we use helm with tiller in our pipeline.
If I changed this to use | defaults instead of putting it in values.yaml would it likely be merged here?

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@sdake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@costinm re this specific PR, this fixes a very specific problem we have with the installation - that is that third-party hub AND images can change upstream. As per policy in several reviews, we have decided (I thought) that third party customization was fine, but customizing istio image names was not fine. I know its inconsistent, however, we control istio image names, and I gave the green light for this work already based upon these decisions that have been agreed upon in past reviews.

Regarding testing, that isn't a problem easily solveable nor should we dump that on @anandkumarpatel just because the rest of the project has not properly sorted out a testing strategy for our installation...

I can approve this specific PR but would like you to agree.

Cheers
-steve

@sdake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@sdake sdake changed the title make all image names configurable make third party repo image names configurable Jul 1, 2019

@sdake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Can you also take care of the grafana third party hub/image? ATM community members are blocked caching of upstream images, and we need the hub to be independent of the image name. It would be nice if this was one pr to backport and not two (if we choose to backport to a z stream).

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I did think a bit more about this - and the implications on operator and kustomize work we are doing.
This doc in particular, on how this would work for people who rely on kustomizations:
https://github.com/kubernetes-sigs/kustomize/blob/master/docs/fields.md#images
Also keeping in mind that values.yaml is getting transformed into a properly schemed configuration, and will be used by operator as 'mesh config'.

How about adopting the same style with kustomize: we allow 'tag' and 'name' to be customized, with 'name' defined as the combination of hub and image name ?

Ideally we would add an "images" section, following the same syntax as kustomize - but I don't know if we can do this in helm.
For people using Kustomize, they would use something like:

images:
- name: istio-pilot
  newName: my-registry/my-pilot
  newTag: v1

Can helm template use this as an input ?

On testing: I am not suggesting full end-to-end tests for this, but some basic "generate the config with a specific setting with helm template, grep the output to verify it has the expected result". I believe there are few other ways to 'unit test' helm templates ( @Stono mentioned one he is using ). This can also serve as an example, like any good test.

In particular I am looking for confirmation that when a feature like this is added - image selection - we can review the usability and how it looks in both helm and kustomize - and for that we need at least some examples included in the PR. And a simple test is also an example.

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

To clarify: I would expect configuring the image name with Kustomize will use the kustomize style, and the test for changing the images will kustomize will use a Kustomize file.
Given that we plan the 'easy' installation to be based on kustomize and 'kubectl -k ..' (and operator in future) we can't ignore this.

@sdake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I did think a bit more about this - and the implications on operator and kustomize work we are doing.
This doc in particular, on how this would work for people who rely on kustomizations:
https://github.com/kubernetes-sigs/kustomize/blob/master/docs/fields.md#images
Also keeping in mind that values.yaml is getting transformed into a properly schemed configuration, and will be used by operator as 'mesh config'.

How about adopting the same style with kustomize: we allow 'tag' and 'name' to be customized, with 'name' defined as the combination of hub and image name ?

Ideally we would add an "images" section, following the same syntax as kustomize - but I don't know if we can do this in helm.
For people using Kustomize, they would use something like:

images:
- name: istio-pilot
  newName: my-registry/my-pilot
  newTag: v1

Can helm template use this as an input ?

I think it may be possible, although @morvencao may have more insight into how to do the transformation. If it were in the subchart values.yaml, it would work straight away. Not sure if we can import kustomize blobs directly into values files, but it may be possible with a tool (that inserts global.).

Any simpler ways @morvencao ?

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

BTW - my comment applies primarily on the istio/installer use of this feature - this PR in particular is consistent with what we are doing for the other images, so no problem with it ( and since we don't test changing the pilot image - I'm not going to ask for it ).

/lgtm
/approve
(but you have a conflict )

@sdake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

thanks @costinm! Also when rectifying the merge conflict, can you address grafana in this same fashion as the rest of this PR?

Cheers
-steve

@costinm

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

We should move the discussion about this to a feature request in installer...
The 'feature' is to have a supported, documented and tested way to customize images and tags, for all 3 installation flows ( kustomize, helm tiller, operator)

@istio-testing

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

@anandkumarpatel: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-unit-tests.sh f5186dd link /test istio-unit-tests-master
prow/istio-lint.sh f5186dd link /test istio-lint-master
prow/shellcheck.sh f5186dd link /test istio-shellcheck-master
prow/racetest.sh f5186dd link /test istio-racetest-master
prow/codecov.sh f5186dd link /test istio-codecov-master
prow/integ-framework-local-presubmit-tests.sh f5186dd link /test integ-framework-local-presubmit-tests-master
prow/integ-galley-local-presubmit-tests.sh f5186dd link /test integ-galley-local-presubmit-tests-master
prow/integ-istioctl-local-presubmit-tests.sh f5186dd link /test integ-istioctl-local-presubmit-tests-master
prow/integ-mixer-local-presubmit-tests.sh f5186dd link /test integ-mixer-local-presubmit-tests-master
prow/integ-pilot-local-presubmit-tests.sh f5186dd link /test integ-pilot-local-presubmit-tests-master
prow/integ-security-local-presubmit-tests.sh f5186dd link /test integ-security-local-presubmit-tests-master
prow/integ-framework-k8s-presubmit-tests.sh f5186dd link /test integ-framework-k8s-presubmit-tests-master
prow/integ-galley-k8s-presubmit-tests.sh f5186dd link /test integ-galley-k8s-presubmit-tests-master
prow/integ-mixer-k8s-presubmit-tests.sh f5186dd link /test integ-mixer-k8s-presubmit-tests-master
prow/integ-istioctl-k8s-presubmit-tests.sh f5186dd link /test integ-istioctl-k8s-presubmit-tests-master
prow/integ-pilot-k8s-presubmit-tests.sh f5186dd link /test integ-pilot-k8s-presubmit-tests-master
prow/integ-security-k8s-presubmit-tests.sh f5186dd link /test integ-security-k8s-presubmit-tests-master
prow/integ-telemetry-k8s-presubmit-tests.sh f5186dd link /test integ-telemetry-k8s-presubmit-tests-master
prow/integ-new-installer-k8s-presubmit-tests.sh f5186dd link /test integ-new-install-k8s-presubmit-tests-master
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh f5186dd link /test istio-pilot-e2e-envoyv2-v1alpha3-master
prow/e2e-mixer-no_auth.sh f5186dd link /test e2e-mixer-no_auth-master
prow/e2e-dashboard.sh f5186dd link /test e2e-dashboard-master
prow/e2e-bookInfoTests-trustdomain.sh f5186dd link /test e2e-bookInfoTests-trustdomain-master
prow/e2e-simpleTests.sh f5186dd link /test e2e-simpleTests-master
prow/istio-pilot-multicluster-e2e.sh f5186dd link /test istio-pilot-multicluster-e2e-master
prow/e2e_pilotv2_auth_sds.sh f5186dd link /test istio_auth_sds_e2e-master
prow/release-test.sh f5186dd link /test release-test-master
prow/integ-telemetry-local-presubmit-tests.sh f5186dd link /test integ-telemetry-local-presubmit-tests-master
prow/e2e-bookInfoTests-v1alpha3.sh f5186dd link /test e2e-bookInfoTests-envoyv2-v1alpha3-master

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@stale

This comment has been minimized.

Copy link

commented Jul 15, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Jul 15, 2019

@istio-testing istio-testing removed the lgtm label Jul 16, 2019

@istio-testing

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

New changes are detected. LGTM label has been removed.

@stale stale bot removed the stale label Jul 16, 2019

@istio-testing

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anandkumarpatel, costinm

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

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@costinm updated with master again. LMK if there if you want anything else in this PR

@anandkumarpatel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Also @sdake grafana chart abstracts this out in a different way.

grafana:
  image:
    repository: grafana/grafana
    tag: 6.1.6

Would you like me to split the grafana.image.repository key into grafana.image.hub and grafana.image.image to match the others? (this can cause some backward-incompatible issue if people are setting this via cli with --set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.