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

Update kubebuilder V4 #234

Merged
merged 13 commits into from
Dec 5, 2023
Merged

Update kubebuilder V4 #234

merged 13 commits into from
Dec 5, 2023

Conversation

MarcosDY
Copy link
Collaborator

No description provided.

@MarcosDY MarcosDY changed the title WIP: Update kubebuilder WIP: Update kubebuilder V4 Oct 20, 2023
@MarcosDY MarcosDY marked this pull request as ready for review October 20, 2023 20:30
@MarcosDY MarcosDY requested a review from azdagron as a code owner October 20, 2023 20:30
@MarcosDY MarcosDY changed the title WIP: Update kubebuilder V4 Update kubebuilder V4 Oct 20, 2023
azdagron
azdagron reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Did you manually verify things worked with this change? In particular, I believe this change as currently written will break the updating of the status subresource on the CRs...

api/v1alpha1/clusterfederatedtrustdomain_types.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably keep the same copyright on these files. We can use 2023 for new ones.

Makefile Outdated
@@ -121,10 +127,11 @@ lint-code: $(golangci_lint_bin)

##@ Build

# TODO: VERIFY BUILD KEEP WORKING WIHT NEW MAIN!!!!!!!!!!!!!!!
Copy link
Member

Choose a reason for hiding this comment

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

Did the build keep working 😄 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hahah forgot to remove the reminders haha

cmd/main.go Outdated
@@ -93,6 +92,21 @@ func parseConfig() (spirev1alpha1.ControllerManagerConfig, ctrl.Options, []*rege
"Command-line flags override configuration from this file.")
flag.StringVar(&spireAPISocketFlag, "spire-api-socket", "", "The path to the SPIRE API socket (deprecated; use the config file)")

// TODO: may we get metric, probeAdrr and leader?
Copy link
Member

Choose a reason for hiding this comment

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

Has this commented code been addressed elsewhere? the features provided by these configurables are still necessary, especially leader election....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we allow to set those values from Config file, so I think we can remove those comments,
what do you think?

cmd/main.go
if err = (&spirev1alpha1.ClusterFederatedTrustDomain{}).SetupWebhookWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "ClusterFederatedTrustDomain")
return err
if os.Getenv("ENABLE_WEBHOOKS") != "false" {
Copy link
Member

Choose a reason for hiding this comment

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

@kfox1111, you might be interested in this. Looks like a new kubebuilder convention?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hadn't seen that. Interesting. Thanks for the heads up.

config/crd/bases/spire.spiffe.io_clusterfederatedtrustdomains.yaml Outdated
@@ -88,9 +87,3 @@ spec:
storage: true
subresources:
status: {}
status:
Copy link
Member

Choose a reason for hiding this comment

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

I believe we still need these. For some reason kubebuilder isn't generating them but without them the status subresource does not update. See #223.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

code generator is no longer creating that status,
but when I run demo without status I got no error, and status is generated:

./cluster1 kubectl describe clusterspiffeid                                                                                                             ✔  16:52:03 
Name:         greeter-server
Namespace:
Labels:       <none>
Annotations:  <none>
API Version:  spire.spiffe.io/v1alpha1
Kind:         ClusterSPIFFEID
Metadata:
  Creation Timestamp:  2023-10-27T19:48:38Z
  Generation:          1
  Resource Version:    725
  UID:                 7043c2fa-90e8-40d8-aa0d-8bd15b0e91eb
Spec:
  Federates With:
    cluster2.demo
  Pod Selector:
    Match Labels:
      spire.spiffe.io/spiffeid:  greeter-server
  Spiffe ID Template:            spiffe://cluster1.demo/greeter-server
Status:
  Stats:
    Entries Masked:             0
    Entries To Set:             1
    Entry Failures:             0
    Namespaces Ignored:         4
    Namespaces Selected:        6
    Pod Entry Render Failures:  0
    Pods Selected:              1

not sure, may we add just in case?

Copy link
Member

Choose a reason for hiding this comment

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

I see. Was this on a clean cluster? I can check this myself to confirm. Maybe it depends on the version of k8s?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was in the test cluster, (demo one), but I remember getting the erorr you mentioned in aws... like 3 months ago..
so I think it is safe to keep adding this for now

demo/test.sh Outdated
@@ -37,7 +37,7 @@ cleanup() {
echo "Done."
}

trap cleanup EXIT
# trap cleanup EXIT
Copy link
Member

Choose a reason for hiding this comment

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

Was this commented out for testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeap

MarcosDY added 10 commits November 3, 2023 12:12
@MarcosDY
Upgrade kubebuilder
5f1d48a 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
clean
3a33dee 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
remove comment
5303322 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
resolve dockerfile issues
63eff54 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
Resolve issues in main
09120c7 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
Resolve missing loading images in test.sh
828de01 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
Update crds in demo
58267b1 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
Some of the PR comments
c8cda76 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
patch manually
136abb6 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@MarcosDY
Add permission
08c294c 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
azdagron
azdagron reviewed Dec 5, 2023
View reviewed changes
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Just some nits on the copyright year changes, other than that LGTM.

cmd/main.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

can we revert the copyright year change?

internal/controller/clusterfederatedtrustdomain_controller.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

internal/controller/clusterspiffeid_controller.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

internal/controller/clusterstaticentry_controller.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

internal/controller/common.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

internal/controller/pod_controller.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

internal/controller/suite_test.go Outdated
@@ -1,5 +1,5 @@
/*
Copyright 2021 SPIRE Authors.
Copyright 2023 SPIRE Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should revert this change.

@MarcosDY
PR changes
07afc0b 
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
@azdagron azdagron merged commit 1cf9f72 into spiffe:main Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfox1111 kfox1111 kfox1111 left review comments

@azdagron azdagron azdagron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarcosDY @kfox1111 @azdagron