Remove hard coded info about cert manager

[fabriziopandini]

As per #3364 (comment)

#3313 introduce some hard-code info about cert-manager in the code base here.

This code was slightly refactored in #3364, but in the long term we should get rid of this or find an alternative that doesn't require manual code changes at every cert-manager release bump

/kind cleanup

[vincepri]

/milestone Next

[wfernandes]

/area clusterctl

[wfernandes]

I feel like this issue and #3491 could be intertwined with each other.

[cpanato]

Can this be a flag in the clusterctl? or what is the idea other ideas?

[fabriziopandini]

I think that a possible approach is to derive hard-coded info (version, manifest-hash) from the embedded manifest, so when you upgrade the manifest there is no more need for manual code changes at every cert-manager release bump

[wfernandes]

As part of the clusterctl version check #3484, we added a version.yaml file that could be used to hold the information that was read from the embedded cert-manager manifest if required for future use.
Also the version.yaml file is not meant to be modified by the user so it would be a good place to store it if necessary.

[jichenjc]

@wfernandes we don't have version.yaml in release
https://github.com/kubernetes-sigs/cluster-api/releases

and this line assume if the file is not there, then that's ok and recreate it later
https://github.com/kubernetes-sigs/cluster-api/blob/master/cmd/clusterctl/cmd/version_checker.go#L188

but the code is hard code ,so should we put the version.yaml into release then download
from release and use it , is this the desired way to go?

[wfernandes]

@jichenjc The version checker is run after any clusterctl command is executed. So if the file doesn't exist in the beginning, it will be created after.

However, after thinking about it some more, it probably doesn't make sense to store this hash. Because the hash will probably be used only when ApplyUpgrade is called. So may be it just makes sense to compute the hash every time since I doubt clusterctl apply upgrade will be called frequently. That is, storing the hash probably doesn't save us very much.

I don't think putting the version.yaml file in the release is necessary because it is a file that is created by clusterctl and used to notify the users if they are using an older version of clusterctl.

Hope this helps.

[jichenjc]

@fabriziopandini @vincepri any comments on the info provided by @wfernandes above?
seems nothing we can do now?

[fabriziopandini]

I agree with @wfernandes that it does not make sense to store additional info in version .yaml, but instead, we should derive hard-coded info (version, manifest-hash) from the embedded manifest whenever necessary

[jichenjc]

so we need find some other places to store such info ? e.g hash.yaml ?

[fabriziopandini]

@jichenjc if I got right the original intent of the comment was to get rid of these following const in the code, and instead derive that info from the embedded manifest.

As per @wfernandes comment, the idea of storing that info somewhere else does not make really sense.

[jichenjc]

@fabriziopandini thanks, so derive that info from the embedded manifest. is the suggestion .
I don't know embedded manifest. means so I am working on learning how to use it ,if you happen to know, please help to point me , thanks~

[fabriziopandini]

embedded manifest is the manifest under cmd/clusterctl/config/assets which is the converted into bindata and shipped in the binary.
The generated bindata file is cmd/clusterctl/config/zz_generated.bindata.go, and an example of how this is used is

cluster-api/cmd/clusterctl/client/cluster/cert_manager.go

Lines 331 to 334 in 0154e3d

	yaml, err := manifests.Asset(embeddedCertManagerManifestPath)
	if err != nil {
	return nil, err
	}

[vincepri]

@jichenjc if I got right the original intent of the comment was to get rid of these following const in the code, and instead derive that info from the embedded manifest.

+1, we've also discussed that we should be able to remove the embedded manifests (bindata), and instead use the upstream cert-manager repository to retrieve the assets and install them. If there are any patches that need to be performed, those should be in the form of json-patch

[fabriziopandini]

If we are including in the scope removing the embedded manifests, I suggest to take some more time before starting to implement this issue, because the current concept of repository used for providers can't be easily adapted to cert-manager (something that is not a provider) and we need something similar to support air-gapped environments

[vincepri]

Let's focus this issue on removing the embedded constants, rather than restructuring how we manage cert-manager, what do you think?

[jichenjc]

@vincepri @fabriziopandini do you think create a configmap will solve the problem?
basically, define a config map at cmd/clusterctl/config/assets/cert-manager.yaml then
parse it during startup and replace hard code info with the configmap info ?

[fabriziopandini]

@jichenjc we basically need two informations, cert-manager version and the hash.
What about trying to compute those info from the embedded cert-manager manifest?

• The cert-manager version can be derived from the helm.sh/chart annotation that exists on the objects that exists in the manifest.
• The hash is a sha256.Sum256 of the manifest yaml
  

  cluster-api/cmd/clusterctl/client/cluster/cert_manager_test.go

  Line 50 in 0154e3d

  actualHash := fmt.Sprintf("%x", sha256.Sum256(yaml))

[fabriziopandini]

/close
given #3918 merged

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

/close
given #3918 merged

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.