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

✨ (go/v3,go/v4-alpha): instead of call script to install kustomize download it first #3187

Merged

Conversation

jberkhahn
Copy link
Contributor

@jberkhahn jberkhahn commented Jan 26, 2023

The security management software IBM runs flags this line as a vulnerability and kill -9's it whenever you try to execute it (by running make bundle-build or using any of the operator sdk commands that use that make task). As a result, IBMers currently can't make use of that functionality. This changes up the line a little to be more palatable to our security software.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jberkhahn / name: Jonathan Berkhahn (7779c40)

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 26, 2023
@jberkhahn
Copy link
Contributor Author

/hold
Would like to bring this up for discussion before merging, anyways.

As for the CLA, did that change somehow? I thought the IBM github org was a signatory to the CLA and my membership is public.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023
@bradtopol
Copy link

/retest

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 27, 2023
@jberkhahn jberkhahn force-pushed the change_kustomize_install branch 2 times, most recently from d2b583d to bb4884a Compare January 27, 2023 00:15
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 27, 2023
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

It seems like this would result in the experience for a user being unchanged but not being flagged by automated security alerts. I'm fine with this change going in.

Will wait to add any labels so the bot doesn't auto merge and allow for further discussion

@jberkhahn
Copy link
Contributor Author

todo: backport this to v3

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @jberkhahn,

Could you please add in the description the alerts/the output that you are facing which motivates you push this PR?

todo: backport this to v3

You do not backport. You need to change the makefile template for the go/v3 plugin and run make generate to ensure that testdata is updated as well.

@jberkhahn
Copy link
Contributor Author

The security management software IBM runs flags this line as a vulnerability and kill -9's it whenever you try to execute it (by running make bundle-build or using any of the operator sdk commands that use that make task). As a result, IBMers currently can't make use of that functionality. We discussed this a bit internally and at the operator sdk meeting, and we think this is the most light-weight solution. I was going to attend the next kubebuilder meeting and discuss this, but I guess I just missed one so it's not for 2 weeks?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jan 30, 2023

Hi @jberkhahn,

Regards #3187 (comment). The meeting is biweekly. The next one will be on Feb 9. But we do not need to bring that to the meeting.

I was just interested to know what errors were faced.
Why downloading the shell (instead of executing it) can solve the problem?

Regarding the proposed change, I do not have any strong opinion about it. Could you please update the go/v3 plugin (same boilerplate) and make generate?

Signed-off-by: jberkhahn <jaberkha@us.ibm.com>
@jberkhahn
Copy link
Contributor Author

Ok, updated this to include v3

Copy link
Contributor

@Kavinjsir Kavinjsir 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 sake of consistency, do we also need to update here:
https://github.com/kubernetes-sigs/kubebuilder/blob/v3.9.0/Makefile#L90

golangci-lint:
	@[ -f $(GOLANGCI_LINT) ] || { \
	set -e ;\
	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) v1.50.1 ;\
	}

@camilamacedo86
Copy link
Member

Hi @Kavinjsir

Regards #3187 (review)

golangci-lint:
@[ -f $(GOLANGCI_LINT) ] || {
set -e ;
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) v1.50.1 ;
}

We do not need to update this one because it is ONLY used by those that contributes with Kubebuilder. therefore, it will not affected the end-users.

@camilamacedo86 camilamacedo86 changed the title change kustomize install templating to avoid tripping automated security alerts ✨ (go/v3,go/v4-alpha): instead of call script to install kustomize download it first Feb 1, 2023
@camilamacedo86
Copy link
Member

@jberkhahn I updated the title accordingly.
However, would very great we be able to have in the description of this PR a better info regards the error faced and why the change is required so that others can better understand its motivations by reading that. Could you please provide it?

@jberkhahn
Copy link
Contributor Author

updated the description

@jberkhahn
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2023
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2023
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, jberkhahn, varshaprasad96

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:
  • OWNERS [camilamacedo86,varshaprasad96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jberkhahn
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit f802fe7 into kubernetes-sigs:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants