From ad3c747db0621cc54d85fca90cbb9f3d409ffe8e Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Mon, 12 Feb 2024 13:29:21 +0300 Subject: [PATCH 1/3] Version of composed resource could change Signed-off-by: Hasan Turken (cherry picked from commit 6bd71361186012e2629eb54d91711f714099b2a0) --- .../composite/composition_render.go | 17 ++++---- .../composite/composition_render_test.go | 39 +++++++++++++++++-- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/internal/controller/apiextensions/composite/composition_render.go b/internal/controller/apiextensions/composite/composition_render.go index d3bbce4ad..f43c5c80a 100644 --- a/internal/controller/apiextensions/composite/composition_render.go +++ b/internal/controller/apiextensions/composite/composition_render.go @@ -32,8 +32,8 @@ const ( errMarshalProtoStruct = "cannot marshal protobuf Struct to JSON" errSetControllerRef = "cannot set controller reference" - errFmtKindChanged = "cannot change the kind of a composed resource from %s to %s (possible composed resource template mismatch)" - errFmtNamePrefixLabel = "cannot find top-level composite resource name label %q in composite resource metadata" + errFmtKindOrGroupChanged = "cannot change the kind or group of a composed resource from %s to %s (possible composed resource template mismatch)" + errFmtNamePrefixLabel = "cannot find top-level composite resource name label %q in composite resource metadata" // TODO(negz): Include more detail such as field paths if they exist. // Perhaps require each patch type to have a String() method to help @@ -61,13 +61,16 @@ func RenderFromJSON(o resource.Object, data []byte) error { o.SetName(name) o.SetNamespace(namespace) - // This resource already had a GVK (probably because it already exists), but + // This resource already had a GK (probably because it already exists), but // when we rendered its template it changed. This shouldn't happen. Either - // someone changed the kind in the template or we're trying to use the wrong - // template (e.g. because the order of an array of anonymous templates + // someone changed the kind or group in the template, or we're trying to use the + // wrong template (e.g. because the order of an array of anonymous templates // changed). - if !gvk.Empty() && o.GetObjectKind().GroupVersionKind() != gvk { - return errors.Errorf(errFmtKindChanged, gvk, o.GetObjectKind().GroupVersionKind()) + // Please note, we don't check for version changes, as versions can change. For example, + // if a composed resource was created with a template that has a version of "v1alpha1", + // and then the template is updated to "v1beta1", the composed resource will still be valid. + if !gvk.Empty() && o.GetObjectKind().GroupVersionKind().GroupKind() != gvk.GroupKind() { + return errors.Errorf(errFmtKindOrGroupChanged, gvk, o.GetObjectKind().GroupVersionKind()) } return nil diff --git a/internal/controller/apiextensions/composite/composition_render_test.go b/internal/controller/apiextensions/composite/composition_render_test.go index 772ed4587..59697114d 100644 --- a/internal/controller/apiextensions/composite/composition_render_test.go +++ b/internal/controller/apiextensions/composite/composition_render_test.go @@ -62,8 +62,25 @@ func TestRenderFromJSON(t *testing.T) { err: errors.Wrap(errInvalidChar, errUnmarshalJSON), }, }, - "ExistingGVKChanged": { - reason: "We should return an error if unmarshalling the base template changed the composed resource's group, version, or kind", + "ExistingGroupChanged": { + reason: "We should return an error if unmarshalling the base template changed the composed resource's group.", + args: args{ + o: composed.New(composed.FromReference(corev1.ObjectReference{ + APIVersion: "example.org/v1", + Kind: "Potato", + })), + data: []byte(`{"apiVersion": "foo.io/v1", "kind": "Potato"}`), + }, + want: want{ + o: composed.New(composed.FromReference(corev1.ObjectReference{ + APIVersion: "foo.io/v1", + Kind: "Potato", + })), + err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "foo.io/v1, Kind=Potato"), + }, + }, + "ExistingKindChanged": { + reason: "We should return an error if unmarshalling the base template changed the composed resource's kind.", args: args{ o: composed.New(composed.FromReference(corev1.ObjectReference{ APIVersion: "example.org/v1", @@ -76,7 +93,23 @@ func TestRenderFromJSON(t *testing.T) { APIVersion: "example.org/v1", Kind: "Different", })), - err: errors.Errorf(errFmtKindChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"), + err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"), + }, + }, + "VersionCanChange": { + reason: "We should accept version changes in the base template.", + args: args{ + o: composed.New(composed.FromReference(corev1.ObjectReference{ + APIVersion: "example.org/v1alpha1", + Kind: "Potato", + })), + data: []byte(`{"apiVersion": "example.org/v1beta1", "kind": "Potato"}`), + }, + want: want{ + o: composed.New(composed.FromReference(corev1.ObjectReference{ + APIVersion: "example.org/v1beta1", + Kind: "Potato", + })), }, }, "NewComposedResource": { From bc322c3c288d6fc31e6cc8a509bddb89bb3d0422 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Mon, 12 Feb 2024 13:07:06 +0000 Subject: [PATCH 2/3] feat: drop aggregate-to-ns-* clusterroles Signed-off-by: Philippe Scorsolini (cherry picked from commit 82036c1d46d13c5bd132d89c98874ab871bb91b1) --- .../rbac-manager-managed-clusterroles.yaml | 69 ------------------- 1 file changed, 69 deletions(-) diff --git a/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml b/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml index 1354396fb..2ddd200c7 100644 --- a/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml +++ b/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml @@ -187,74 +187,5 @@ rules: - apiextensions.crossplane.io resources: ["*"] verbs: [get, list, watch] -{{- if .Values.rbacManager.managementPolicy }} ---- -# The below ClusterRoles are aggregated to the namespaced RBAC roles created by -# the Crossplane RBAC manager when it is running in --manage=All mode. -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: {{ template "crossplane.name" . }}:aggregate-to-ns-admin - labels: - rbac.crossplane.io/aggregate-to-ns-admin: "true" - rbac.crossplane.io/base-of-ns-admin: "true" - app: {{ template "crossplane.name" . }} - {{- include "crossplane.labels" . | indent 4 }} -rules: -# Crossplane namespace admins have access to view events. -- apiGroups: [""] - resources: [events] - verbs: [get, list, watch] -# Crossplane namespace admins may need to read or otherwise interact with -# resource claim connection secrets. -- apiGroups: [""] - resources: [secrets] - verbs: ["*"] -# Crossplane namespace admins have access to view the roles that they may be -# able to grant to other subjects. -- apiGroups: [rbac.authorization.k8s.io] - resources: [roles] - verbs: [get, list, watch] -# Crossplane namespace admins have access to grant the access they have to other -# subjects. -- apiGroups: [rbac.authorization.k8s.io] - resources: [rolebindings] - verbs: ["*"] ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: {{ template "crossplane.name" . }}:aggregate-to-ns-edit - labels: - rbac.crossplane.io/aggregate-to-ns-edit: "true" - rbac.crossplane.io/base-of-ns-edit: "true" - app: {{ template "crossplane.name" . }} - {{- include "crossplane.labels" . | indent 4 }} -rules: -# Crossplane namespace editors have access to view events. -- apiGroups: [""] - resources: [events] - verbs: [get, list, watch] -# Crossplane namespace editors may need to read or otherwise interact with -# resource claim connection secrets. -- apiGroups: [""] - resources: [secrets] - verbs: ["*"] ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: {{ template "crossplane.name" . }}:aggregate-to-ns-view - labels: - rbac.crossplane.io/aggregate-to-ns-view: "true" - rbac.crossplane.io/base-of-ns-view: "true" - app: {{ template "crossplane.name" . }} - {{- include "crossplane.labels" . | indent 4 }} -rules: -# Crossplane namespace viewers have access to view events. -- apiGroups: [""] - resources: [events] - verbs: [get, list, watch] -{{- end }} {{- end }} {{- end }} From 98b3da6631735f087d7e6f41d034ee223680f286 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Sat, 10 Feb 2024 09:48:08 +0000 Subject: [PATCH 3/3] fix(crank/xpkg): push properly retrieve upbound credentials Signed-off-by: Philippe Scorsolini (cherry picked from commit 4a7e9387187745ed554836ceb514f4ca6ea587ec) --- cmd/crank/xpkg/push.go | 15 ++++++++++++++- internal/xpkg/upbound/credhelper/credhelper.go | 4 ++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/cmd/crank/xpkg/push.go b/cmd/crank/xpkg/push.go index 0f7ea5fcd..eca0aee0d 100644 --- a/cmd/crank/xpkg/push.go +++ b/cmd/crank/xpkg/push.go @@ -36,6 +36,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/logging" "github.com/crossplane/crossplane/internal/xpkg" + "github.com/crossplane/crossplane/internal/xpkg/upbound" "github.com/crossplane/crossplane/internal/xpkg/upbound/credhelper" ) @@ -62,6 +63,9 @@ type pushCmd struct { // Flags. Keep sorted alphabetically. PackageFiles []string `short:"f" type:"existingfile" placeholder:"PATH" help:"A comma-separated list of xpkg files to push."` + // Common Upbound API configuration. + upbound.Flags `embed:""` + // Internal state. These aren't part of the user-exposed CLI structure. fs afero.Fs } @@ -91,6 +95,11 @@ func (c *pushCmd) AfterApply() error { // Run runs the push cmd. func (c *pushCmd) Run(logger logging.Logger) error { //nolint:gocyclo // This feels easier to read as-is. + upCtx, err := upbound.NewFromFlags(c.Flags, upbound.AllowMissingProfile()) + if err != nil { + return err + } + tag, err := name.NewTag(c.Package, name.WithDefaultRegistry(xpkg.DefaultRegistry)) if err != nil { return errors.Wrapf(err, errFmtNewTag, c.Package) @@ -112,7 +121,11 @@ func (c *pushCmd) Run(logger logging.Logger) error { //nolint:gocyclo // This fe } kc := authn.NewMultiKeychain( - authn.NewKeychainFromHelper(credhelper.New()), + authn.NewKeychainFromHelper(credhelper.New( + credhelper.WithLogger(logger), + credhelper.WithProfile(upCtx.ProfileName), + credhelper.WithDomain(upCtx.Domain.Hostname()), + )), authn.DefaultKeychain, ) diff --git a/internal/xpkg/upbound/credhelper/credhelper.go b/internal/xpkg/upbound/credhelper/credhelper.go index b5cc3133e..cf68fa35f 100644 --- a/internal/xpkg/upbound/credhelper/credhelper.go +++ b/internal/xpkg/upbound/credhelper/credhelper.go @@ -113,8 +113,10 @@ func (h *Helper) List() (map[string]string, error) { // Get gets credentials for the supplied server. func (h *Helper) Get(serverURL string) (string, string, error) { if !strings.Contains(serverURL, h.domain) { + h.log.Debug("Supplied server URL is not supported by this credentials helper", "serverURL", serverURL, "domain", h.domain) return "", "", errors.New(errUnsupportedDomain) } + h.log.Debug("Getting credentials for server", "serverURL", serverURL) if err := h.src.Initialize(); err != nil { return "", "", errors.Wrap(err, errInitializeSource) } @@ -124,11 +126,13 @@ func (h *Helper) Get(serverURL string) (string, string, error) { } var p config.Profile if h.profile == "" { + h.log.Debug("No profile specified, using default profile") _, p, err = conf.GetDefaultUpboundProfile() if err != nil { return "", "", errors.Wrap(err, errGetDefaultProfile) } } else { + h.log.Debug("Using specified profile", "profile", h.profile) p, err = conf.GetUpboundProfile(h.profile) if err != nil { return "", "", errors.Wrap(err, errGetProfile)