Skip to content

chore(refactor): Operator-sdk upgrade#561

Merged
akhilerm merged 33 commits intoopenebs-archive:masterfrom
RealHarshThakur:osdk-upgrade
Jul 7, 2021
Merged

chore(refactor): Operator-sdk upgrade#561
akhilerm merged 33 commits intoopenebs-archive:masterfrom
RealHarshThakur:osdk-upgrade

Conversation

@RealHarshThakur
Copy link
Copy Markdown
Contributor

@RealHarshThakur RealHarshThakur commented Apr 4, 2021

Pull Request template

Why is this PR required? What issue does it fix?:
Operator-sdk is used to scaffold the project and beyond that, it will help when creating webhooks, etc

What this PR does?:
Upgrades operator-sdk to latest version

Does this PR require any upgrade changes?:

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Pending tasks:

  • Migrate project structure
  • Fix import types
  • Upgrade client-go
  • Fix ndm operator's main.go (it has outdated operator-sdk dependencies)
  • Fix unit tests
  • Fix integration tests which are failing due to lack of preupgrade checks

References:

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@RealHarshThakur RealHarshThakur changed the title chore(upgrade): Operator-sdk upgrade chore(refactor): Operator-sdk upgrade Apr 4, 2021
Comment thread Dockerfile Outdated
Comment thread PROJECT Outdated
Comment thread apis/blockdevice/v1alpha1/groupversion_info.go Outdated
Comment thread .dockerignore Outdated
Comment thread apis/blockdevice/v1alpha1/blockdevice_types.go Outdated
Comment thread apis/blockdeviceclaim/v1alpha1/groupversion_info.go Outdated
Comment thread build/boilerplate.go.txt
Comment thread cmd/manager/main.go Outdated
@RealHarshThakur
Copy link
Copy Markdown
Contributor Author

@akhilerm Mentioned some of these concerns in Slack too.
I am not sure about some of the changes:

Boilerplate, dockerfiles, etc were generated by operator-sdk which I'll clean up and structure in Makefile when operator-sdk is to be used.

Comment thread apis/blockdevice/v1alpha1/crds.go
Comment thread config/certmanager/certificate.yaml Outdated
Comment thread cmd/ndm_daemonset/controller/controller.go
Comment thread controllers/blockdevice/blockdevice_controller.go
Comment thread deploy/crds/blockdevice.openebs.io_blockdevices.yaml Outdated
Comment thread go.mod Outdated
Comment thread main.go Outdated
Comment thread pkg/crds/build.go Outdated
Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

Some comments:

  1. What is the use of config/ directory
  2. Are the controllers meant to be at the root of the project?

Copy link
Copy Markdown
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

For the packaging of the latest operator-sdk the main.go, apis/ and controller/ are at the root which is ideal for a single controller. But in this repo we have multiple operators so can we move them back into cmd/ and pkg/ structure instead of strictly following the operator-sdk pattern.

Comment thread config/crd/patches/webhook_in_blockdeviceclaims.yaml Outdated
Comment thread config/default/kustomization.yaml Outdated
@RealHarshThakur
Copy link
Copy Markdown
Contributor Author

RealHarshThakur commented Apr 7, 2021

@shubham14bajpai I think it's possible to have multiple controllers with same layout because main.go creates a manager to run all of them. We can remove the kustomize and others thing we don't use but I am not sure if we should move away from project structure as it will make upgrading the operator-sdk version difficult as mentioned in their docs. It will also hinder using some of the features https://v1-5-x.sdk.operatorframework.io/docs/faqs/#can-i-customize-the-projects-generated-with-sdk-tool

@akhilerm
Copy link
Copy Markdown
Contributor

akhilerm commented Apr 7, 2021

@shubham14bajpai I think it's possible to have multiple controllers with same layout because main.go creates a manager to run all of them. We can remove the kustomize and others thing we don't use but I am not sure if we should move away from project structure as it will make upgrading the operator-sdk version difficult as mentioned in their docs. It will also hinder using some of the features https://v1-5-x.sdk.operatorframework.io/docs/faqs/#can-i-customize-the-projects-generated-with-sdk-tool

How is it done in jiva operator @shubham14bajpai ?

@shubham14bajpai
Copy link
Copy Markdown
Contributor

@shubham14bajpai I think it's possible to have multiple controllers with same layout because main.go creates a manager to run all of them. We can remove the kustomize and others thing we don't use but I am not sure if we should move away from project structure as it will make upgrading the operator-sdk version difficult as mentioned in their docs. It will also hinder using some of the features https://v1-5-x.sdk.operatorframework.io/docs/faqs/#can-i-customize-the-projects-generated-with-sdk-tool

How is it done in jiva operator @shubham14bajpai ?

In case of jiva-operator we wanted to be less dependent on the operator-sdk binary as that itself has breaking changes if we upgrade after a long time. As the operator-sdk generates the code using the standard k8s kubebuilder etc. I restructured the packages in line with other repos and added targets to the makefile using standard k8s patterns as I did for the crd generation PR. This helps us have more control over smaller parts of the code.

@RealHarshThakur
Copy link
Copy Markdown
Contributor Author

RealHarshThakur commented Apr 7, 2021

Currently, kubebuilder and operator-sdk are following the same package structure. Also, I think there are some good features we have to start utilizing from Operator-sdk like OLM, score card testing and small features like this which might be valuable at some point. After their 1.0 release, there are no major breaking changes. Having said that, if we're sure we don't need those features , then I think we can stop relying on operator-sdk altogether but IMHO, I think we should stick to Operator-SDK and if we ever face any issue, we can easily fall back on controller-gen, etc .

@shubham14bajpai
Copy link
Copy Markdown
Contributor

Yes it makes sense if these features are used. In case of jiva-operator it was a simple reconciler and these features are not required.

@RealHarshThakur RealHarshThakur marked this pull request as draft April 16, 2021 15:30
Comment thread go.mod Outdated
google.golang.org/protobuf v1.24.0
k8s.io/api v0.19.2
k8s.io/apiextensions-apiserver v0.19.2
k8s.io/apimachinery v0.19.2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical OSS Vulnerability:  

pkg:golang/golang.org/x/net@0.0.0-20180724234803-3673e40ba225

5 Critical, 0 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/k8s.io/apimachinery@0.19.2

CRITICAL Vulnerabilities (5)

    CVE-2018-17143

    [CVE-2018-17143] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-17 in Go mishandles <template><tBody><isindex/action=0>, leading to a "panic: runtime error" in inBodyIM in parse.go during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17848

    [CVE-2018-17848] Data Handling

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <math><template><mn><b></template>, leading to a "panic: runtime error" (index out of range) in (*insertionModeStack).pop in node.go, called from inHeadIM, during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17847

    [CVE-2018-17847] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <svg><template><desc><t><svg></template>, leading to a "panic: runtime error" (index out of range) in (*nodeStack).pop in node.go, called from (*parser).clearActiveFormattingElements, during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17142

    [CVE-2018-17142] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-17 in Go mishandles <math><template><mo><template>, leading to a "panic: runtime error" in parseCurrentToken in parse.go during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17846

    [CVE-2018-17846] Resource Management Errors

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <table><math><select><mi><select></table>, leading to an infinite loop during an html.Parse call because inSelectIM and inSelectInTableIM do not comply with a specification.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


(at-me in a reply with help or ignore)

Comment thread go.mod Outdated
k8s.io/client-go v12.0.0+incompatible
golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4
google.golang.org/grpc v1.27.0
google.golang.org/protobuf v1.24.0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severe OSS Vulnerability:  

pkg:golang/golang.org/x/crypto@0.0.0-20190308221718-c2843e01d9a2

0 Critical, 1 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/google.golang.org/protobuf@1.24.0

SEVERE Vulnerabilities (1)

    [CVE-2019-11840] Use of Insufficiently Random Values

    An issue was discovered in supplementary Go cryptography libraries, aka golang-googlecode-go-crypto, before 2019-03-20. A flaw was found in the amd64 implementation of golang.org/x/crypto/salsa20 and golang.org/x/crypto/salsa20/salsa. If more than 256 GiB of keystream is generated, or if the counter otherwise grows greater than 32 bits, the amd64 implementation will first generate incorrect output, and then cycle back to previously generated keystream. Repeated keystream bytes can lead to loss of confidentiality in encryption applications, or to predictability in CSPRNG applications.

    CVSS Score: 5.9

    CVSS Vector: CVSS:3.0/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:N/A:N


(at-me in a reply with help or ignore)

@akhilerm akhilerm requested a review from shubham14bajpai July 6, 2021 09:18
akhilerm added 9 commits July 6, 2021 17:51
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Comment thread go.mod
github.com/onsi/ginkgo v1.14.1
github.com/onsi/gomega v1.10.2
github.com/prometheus/client_golang v1.7.1
github.com/spf13/cobra v1.1.1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical OSS Vulnerability:  

pkg:golang/golang.org/x/net@0.0.0-20180724234803-3673e40ba225

5 Critical, 0 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/github.com/spf13/cobra@1.1.1

CRITICAL Vulnerabilities (5)

    CVE-2018-17143

    [CVE-2018-17143] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-17 in Go mishandles <template><tBody><isindex/action=0>, leading to a "panic: runtime error" in inBodyIM in parse.go during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17848

    [CVE-2018-17848] Data Handling

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <math><template><mn><b></template>, leading to a "panic: runtime error" (index out of range) in (*insertionModeStack).pop in node.go, called from inHeadIM, during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17847

    [CVE-2018-17847] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <svg><template><desc><t><svg></template>, leading to a "panic: runtime error" (index out of range) in (*nodeStack).pop in node.go, called from (*parser).clearActiveFormattingElements, during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17142

    [CVE-2018-17142] Improper Input Validation

    The html package (aka x/net/html) through 2018-09-17 in Go mishandles <math><template><mo><template>, leading to a "panic: runtime error" in parseCurrentToken in parse.go during an html.Parse call.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


    CVE-2018-17846

    [CVE-2018-17846] Resource Management Errors

    The html package (aka x/net/html) through 2018-09-25 in Go mishandles <table><math><select><mi><select></table>, leading to an infinite loop during an html.Parse call because inSelectIM and inSelectInTableIM do not comply with a specification.

    CVSS Score: 7.5

    CVSS Vector: CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H


(at-me in a reply with help or ignore)

Comment thread go.mod
github.com/onsi/ginkgo v1.14.1
github.com/onsi/gomega v1.10.2
github.com/prometheus/client_golang v1.7.1
github.com/spf13/cobra v1.1.1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severe OSS Vulnerability:  

pkg:golang/golang.org/x/crypto@0.0.0-20181029021203-45a5f77698d3

0 Critical, 1 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/github.com/spf13/cobra@1.1.1

SEVERE Vulnerabilities (1)

    [CVE-2019-11840] Use of Insufficiently Random Values

    An issue was discovered in supplementary Go cryptography libraries, aka golang-googlecode-go-crypto, before 2019-03-20. A flaw was found in the amd64 implementation of golang.org/x/crypto/salsa20 and golang.org/x/crypto/salsa20/salsa. If more than 256 GiB of keystream is generated, or if the counter otherwise grows greater than 32 bits, the amd64 implementation will first generate incorrect output, and then cycle back to previously generated keystream. Repeated keystream bytes can lead to loss of confidentiality in encryption applications, or to predictability in CSPRNG applications.

    CVSS Score: 5.9

    CVSS Vector: CVSS:3.0/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:N/A:N


(at-me in a reply with help or ignore)

Comment thread go.mod
google.golang.org/grpc v1.27.1
google.golang.org/protobuf v1.25.0
k8s.io/api v0.20.2
k8s.io/apiextensions-apiserver v0.20.1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severe OSS Vulnerability:  

pkg:golang/golang.org/x/crypto@0.0.0-20181029021203-45a5f77698d3

0 Critical, 1 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/k8s.io/apiextensions-apiserver@0.20.1

SEVERE Vulnerabilities (1)

    [CVE-2019-11840] Use of Insufficiently Random Values

    An issue was discovered in supplementary Go cryptography libraries, aka golang-googlecode-go-crypto, before 2019-03-20. A flaw was found in the amd64 implementation of golang.org/x/crypto/salsa20 and golang.org/x/crypto/salsa20/salsa. If more than 256 GiB of keystream is generated, or if the counter otherwise grows greater than 32 bits, the amd64 implementation will first generate incorrect output, and then cycle back to previously generated keystream. Repeated keystream bytes can lead to loss of confidentiality in encryption applications, or to predictability in CSPRNG applications.

    CVSS Score: 5.9

    CVSS Vector: CVSS:3.0/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:N/A:N


(at-me in a reply with help or ignore)

Comment thread go.mod
k8s.io/client-go v0.20.2
k8s.io/klog v1.0.0
sigs.k8s.io/controller-runtime v0.5.2
sigs.k8s.io/controller-runtime v0.8.2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severe OSS Vulnerability:  

pkg:golang/golang.org/x/crypto@0.0.0-20181029021203-45a5f77698d3

0 Critical, 1 Severe, 0 Moderate and 0 Unknown vulnerabilities have been found in a transitive dependency of pkg:golang/sigs.k8s.io/controller-runtime@0.8.2

SEVERE Vulnerabilities (1)

    [CVE-2019-11840] Use of Insufficiently Random Values

    An issue was discovered in supplementary Go cryptography libraries, aka golang-googlecode-go-crypto, before 2019-03-20. A flaw was found in the amd64 implementation of golang.org/x/crypto/salsa20 and golang.org/x/crypto/salsa20/salsa. If more than 256 GiB of keystream is generated, or if the counter otherwise grows greater than 32 bits, the amd64 implementation will first generate incorrect output, and then cycle back to previously generated keystream. Repeated keystream bytes can lead to loss of confidentiality in encryption applications, or to predictability in CSPRNG applications.

    CVSS Score: 5.9

    CVSS Vector: CVSS:3.0/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:N/A:N


(at-me in a reply with help or ignore)

Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
@RealHarshThakur
Copy link
Copy Markdown
Contributor Author

@akhilerm LGTM , I think there could be small things we could be missing as PR has gotten so huge but we can fix them later.

Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
@akhilerm
Copy link
Copy Markdown
Contributor

akhilerm commented Jul 7, 2021

@akhilerm LGTM , I think there could be small things we could be missing as PR has gotten so huge but we can fix them later.

@RealHarshThakur can you make a list of those things, so that we can refer back to this PR, incase we need to update it later.

shubham14bajpai
shubham14bajpai previously approved these changes Jul 7, 2021
Copy link
Copy Markdown
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
@RealHarshThakur
Copy link
Copy Markdown
Contributor Author

@akhilerm I meant I don't see anything wrong immediately. There could be no issues too :D

Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
@akhilerm akhilerm requested a review from shubham14bajpai July 7, 2021 07:34
Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@akhilerm akhilerm merged commit d6c57f0 into openebs-archive:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants