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

update operator-sdk version in the go.mod #461

Open
iam-veeramalla opened this issue Oct 18, 2021 · 7 comments · May be fixed by #1301
Open

update operator-sdk version in the go.mod #461

iam-veeramalla opened this issue Oct 18, 2021 · 7 comments · May be fixed by #1301
Assignees
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@iam-veeramalla
Copy link
Collaborator

Describe the bug
operator-sdk version in the go.mod is very old. We need to update it to v1.11.0. Operator prints the sdk version using this dependency which will create a confusion to the users about the sdk version being used.

While it looks simple and straight forward, it is slightly complicated. The new version of operator-sdk has structural changes w.r.t to packages. Some of the packages like k8sutil which is used by the argocd-operator no longer exists in the new version of operator-sdk. It is an internal package in the new version.
Reference:
operator-framework/operator-sdk#3792

Expected behavior
Update the operator-sdk version

@iam-veeramalla iam-veeramalla modified the milestones: v.0.1.0, v0.1.0 Oct 18, 2021
@iam-veeramalla iam-veeramalla added bug Something isn't working good first issue Good for newcomers labels Oct 18, 2021
@jaideepr97
Copy link
Collaborator

@iam-veeramalla

could we bundle the argo-cd version change from v1.8.7 to v2.1.2 in this issue?

@iam-veeramalla
Copy link
Collaborator Author

That should be a different issue @jaideepr97 . Can you create one with the details ?

@jaideepr97
Copy link
Collaborator

@iam-veeramalla created #462
Thanks

@LaloLoop
Copy link

Hello! I would like to help out with this issue.

From what I read, I have been following the linked issue, and there seems to be a migration guide from pre 1.0.0 versions (0.18+ in this case), but it involves recreating the project and mapping the APIs again. That does not sound like the right way to me, and I feel like I should only find replacements for the now private packages. So far I've found the following.

For the k8sutils
The only usage I found was to retrieve the namespace being watched.

namespace, err := k8sutil.GetWatchNamespace()

My idea is to replace it with the same function but in the operator codebase, as it only reads from a predefined env variable and returns a string.

The Operator SDK version
Since the linked issue mentions

Your project should no longer depend directly on operator-sdk

I believe we should no longer print this out to the logs. I created a newly scaffolded project with the latest operator-sdk version and there's no trace of the operator-sdk libraries in the main.go file.

The tlsutil usage

Looking at the code, I found the usage of the type tlsutil.CertConfig, which is very similar to the CertManager's CertificateSpec. It's basically being used as a way to abstract a certificate definition and translate it into a secret inside the operator reconciliation process. Also, in issue #498 of the operator-sdk, they mention cert-manager as a more reliable source for cert issuance and management than they maintaining their own lib.

cfg := &tlsutil.CertConfig{
  CertName:     secret.Name,
  CertType:     tlsutil.ClientAndServingCert,
  CommonName:   secret.Name,
  Organization: []string{cr.ObjectMeta.Namespace},
}

The properties in use are basically the same provided by the latter. The possible mapping could be:

  • tlsutil.CertConfig.CertName => CertificateSpec.SecretName
  • tlsutil.CertConfig.CertType => Not required, explained below
  • tlsutil.CertConfig.CommonName => CertificateSpec.CommonName
  • tlsutil.CertConfig.Organization => CertificateSpec.Subject. Organizations

The field tlsutil.CertConfig.CertType would no longer be needed, because looking at the NewSignedCertificate function, although there's a switch statement, in the entire codebase, the only type ever assigned is tlsutil.ClientAndServingCert, thus, we could constraint it to only be that type; moreover, because it's not a configurable parameter.

I just looked at the test, but this is my plan so far. Please let me know what you think.

As a disclaimer, although I have written go code and some small services before with it, I'm still learning to become more and become more fluent/confortable with it.

Thanks in advance.

@Elyytscha
Copy link
Contributor

I added a PR for this, checks are passing, Feedback would be nice

@Elyytscha
Copy link
Contributor

@svghadi maybe we can work this out together too? :) would be willing to help with this, an initial pr i already submitted, but i think its not ready yet

@svghadi
Copy link
Collaborator

svghadi commented Jun 12, 2024

Sure @Elyytscha. Allow me some time to go through the issue and your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
5 participants