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.

Sign up for GitHub

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

Jump to bottom

KIND: Fix output path for api conversation generation + Fix webhook ca bundle injection for flow-collector CRD #284

Merged
merged 2 commits into from
Mar 3, 2023
Merged

KIND: Fix output path for api conversation generation + Fix webhook ca bundle injection for flow-collector CRD #284

merged 2 commits into from
Mar 3, 2023

Conversation

astoycos
Copy link
Contributor

@astoycos astoycos commented Mar 2, 2023 •
edited

This PR fixes two bugs:

There are still some webhook issues when running on KIND... specifically I spin things up with

  1. IMG="quay.io/bpfd/network-observability-operator:test" make image-build image-push
  2. make local-deploy
  3. IMG="quay.io/bpfd/network-observability-operator:test" make deploy-kind
  4. make deploy-sample-cr

I run into

==> Deploy sample CR
kubectl apply -f ./config/samples/flows_v1beta1_flowcollector.yaml || true
Error from server: error when creating "./config/samples/flows_v1beta1_flowcollector.yaml": conversion webhook for flows.netobserv.io/v1beta1, Kind=FlowCollector failed: Post "https://netobserv-webhook-service.netobserv.svc:443/convert?timeout=30s": x509: certificate signed by unknown authority

operator Logs:

2023/03/02 06:33:07 http: TLS handshake error from 10.244.0.1:3302: remote error: tls: bad certificate

This PR fixes the aforementioned bug by ensuring we inject the webhook ca-bundle into the flow-collector CRD definition.

The makefile logic to detect weather or not the cloned code is in gopath is flawed....specifically

# Set --output-base for conversion-gen if we are not within GOPATH
ifneq ($(abspath $(ROOT_DIR)),$(shell go env GOPATH)/src/network-observability-operator)
	CONVERSION_GEN_OUTPUT_BASE := --output-base=$(ROOT_DIR)
else
	export GOPATH := $(shell go env GOPATH)
endif

Assumes that you specifically cloned the code to $GOPATH/src/network-observability-operator which probably isn’t the case for everyone

for example on my setup the repo lives at /home/astoycos/go/src/github.com/netobserv/network-observability-operator so although it’s in my GOPATH we still set --output-base here when we don’t need to. Ultimately I was seeing zz_generated.conversion.go get put in the wrong spot because of this

The fix just makes sure that the $GOPATH is in wherever the code lives (i.e $ROOT_DIR)

@msherif1234
Copy link
Contributor

msherif1234 commented Mar 2, 2023 •
edited

@astoycos pls make sure u update to main latest all webhook issues should have been addressed as well as kind issues, specifically the tls error u shared olm doesn’t accept enabling cert mgr I fixed that in #274

Fix output path detection for generated conversion
0bb411f 
Fix the logic to detect if the cloned repo is in the GOPATH
by removing a the hardcoded path `$(GOPATH)/src/network-observability-operator`
and instead just check if the $(ROOT_DIR) is in the $(GOPATH) or not with
string comparisons.

Also update the conversions with the new controller-gen version.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos astoycos force-pushed the fix-kind branch 3 times, most recently from 14938cd to 00fcdc8 Compare March 2, 2023 18:21
@astoycos astoycos changed the title WIP: Fixup kind setup, version updates, and webhook issues Fixup kind setup, version updates, and webhook issues Mar 2, 2023
@astoycos
Copy link
Contributor Author

astoycos commented Mar 2, 2023

/assign @msherif1234

This should be good to go now!

msherif1234
msherif1234 reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@msherif1234 msherif1234 left a comment

Choose a reason for hiding this comment

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

overall looks good thanks for fixing KinD env, just left a couple of nits

Makefile Show resolved Hide resolved
config/kubernetes/manager_webhook_patch.yaml Outdated
@@ -21,3 +21,11 @@ spec:
secret:
defaultMode: 420
secretName: webhook-server-cert
---
# The following patch adds a directive for certmanager to inject CA into the CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't better/cleaner to have this in its own patch file and let kustomization patch it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it's just a preference thing I can do that if you'd like, I wish I could just point to ../crd/patches/cainjection_in_flowcollectors.yaml but it won't let me point to that file from the kubernetes/ directory

Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to keep the original sdk yamls as close as the original auto generated ones so any future interactions with sdk-operator will be smooth

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do that later if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good I can do it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!
/lgtm

@msherif1234 msherif1234 added no-qe This PR doesn't necessitate QE approval no-doc This PR doesn't require documentation change on the NetObserv operator labels Mar 2, 2023
@msherif1234
Copy link
Contributor

also there is no longer version updates so pls adjust the title and PR description just indicating for kind we need to have cert-mage CA or something similar

@astoycos astoycos changed the title Fixup kind setup, version updates, and webhook issues Fixup - Conversation generation and kind webhook issues Mar 2, 2023
Add CA bundle to CRD for Kind deployment
517b499 
Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@msherif1234 msherif1234 changed the title Fixup - Conversation generation and kind webhook issues KIND: Fixup - Conversation generation and kind webhook issues Mar 2, 2023
@openshift-ci openshift-ci bot added the lgtm label Mar 2, 2023
@astoycos astoycos changed the title KIND: Fixup - Conversation generation and kind webhook issues KIND: Fixup output path for conversation generation and webhook ca bundle injection for flow-collector CRD Mar 2, 2023
@astoycos astoycos changed the title KIND: Fixup output path for conversation generation and webhook ca bundle injection for flow-collector CRD KIND: Fixup output path for api conversation generation and webhook ca bundle injection for flow-collector CRD Mar 2, 2023
@astoycos astoycos changed the title KIND: Fixup output path for api conversation generation and webhook ca bundle injection for flow-collector CRD KIND: Fix output path for api conversation generation + Fix webhook ca bundle injection for flow-collector CRD Mar 2, 2023
@jotak
Copy link
Member

jotak commented Mar 3, 2023

tested and lgtm, thanks @astoycos !
/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

@openshift-ci openshift-ci bot added the approved label Mar 3, 2023
@openshift-merge-robot openshift-merge-robot merged commit 176f7d9 into netobserv:main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msherif1234 msherif1234 msherif1234 left review comments

Assignees

@msherif1234 msherif1234

Labels
approved lgtm no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@astoycos @msherif1234 @jotak @openshift-merge-robot