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

Fix registry mirror issues in packages #7115

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ OUTPUT_DIR := _output
OUTPUT_BIN_DIR := ${OUTPUT_DIR}/bin

KUSTOMIZE := $(TOOLS_BIN_DIR)/kustomize
KUSTOMIZE_VERSION := 4.2.0
KUSTOMIZE_VERSION := 4.5.6

KUSTOMIZE_OUTPUT_BIN_DIR="${OUTPUT_DIR}/kustomize-bin/"

Expand Down
1 change: 0 additions & 1 deletion config/default/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ resources:

bases:
- ../crd
- ../rbac
- ../manager
# [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
# crd/kustomization.yaml
Expand Down
43 changes: 37 additions & 6 deletions config/manifest/eksa-components.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6633,6 +6633,28 @@ metadata:
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
creationTimestamp: null
name: eksa-packages-role
namespace: eksa-packages
rules:
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- patch
- update
- apiGroups:
- packages.eks.amazonaws.com
resources:
- packagebundlecontrollers
verbs:
- delete
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: eksa-leader-election-role
namespace: eksa-system
Expand Down Expand Up @@ -6683,12 +6705,6 @@ rules:
verbs:
- patch
- update
- apiGroups:
- packages.eks.amazonaws.com
resources:
- packagebundlecontrollers
verbs:
- delete
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand All @@ -6711,6 +6727,7 @@ rules:
verbs:
- create
- delete
- get
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -7062,6 +7079,20 @@ rules:
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: eksa-packages-rolebinding
namespace: eksa-packages
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: eksa-packages-role
subjects:
- kind: ServiceAccount
name: eksa-controller-manager
namespace: eksa-system
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: eksa-leader-election-rolebinding
namespace: eksa-system
Expand Down
1 change: 1 addition & 0 deletions config/prod/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../default
- ../rbac_default
Copy link
Member

Choose a reason for hiding this comment

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

curious why are we adding this only on prod? don't we want all these permissions regardless of the "target"?


images:
- name: controller
Expand Down
20 changes: 19 additions & 1 deletion config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ rules:
verbs:
- create
- delete
- get
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -376,13 +377,14 @@ kind: Role
metadata:
creationTimestamp: null
name: manager-role
namespace: eksa-system
namespace: eksa-packages
rules:
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- patch
- update
- apiGroups:
Expand All @@ -391,3 +393,19 @@ rules:
- packagebundlecontrollers
verbs:
- delete

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
creationTimestamp: null
name: manager-role
namespace: eksa-system
rules:
- apiGroups:
- ""
resources:
- secrets
Copy link
Member

Choose a reason for hiding this comment

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

the cluster role has permissions to do these for secrets
can we remove it from the clusterole now that you have added this role?

verbs:
- patch
- update
17 changes: 16 additions & 1 deletion config/rbac/role_binding.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,19 @@ roleRef:
subjects:
- kind: ServiceAccount
name: controller-manager
namespace: system
namespace: system

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: manager-rolebinding
namespace: eksa-packages
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: manager-role
subjects:
- kind: ServiceAccount
name: controller-manager
namespace: system
11 changes: 11 additions & 0 deletions config/rbac_default/apply-namespace.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
namePrefix:
- path: metadata/namespace
kind: ServiceAccount
- path: subjects/name
kind: RoleBinding
- path: subjects/namespace
kind: RoleBinding
- path: subjects/name
kind: ClusterRoleBinding
- path: subjects/namespace
kind: ClusterRoleBinding
38 changes: 38 additions & 0 deletions config/rbac_default/kustomization.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
namePrefix: eksa-

bases:
- ../rbac

configurations:
- ./apply-namespace.yaml

transformers:
- |-
apiVersion: builtin
kind: NamespaceTransformer
metadata:
name: notImportantHere2
namespace: eksa-system
unsetOnly: true

patchesJson6902:
- patch: |-
- op: replace
path: /metadata/name
value: eksa-packages-rolebinding
target:
group: rbac.authorization.k8s.io
kind: RoleBinding
name: eksa-manager-rolebinding
namespace: eksa-packages
version: v1
- patch: |-
- op: replace
path: /metadata/name
value: eksa-packages-role
target:
group: rbac.authorization.k8s.io
kind: Role
name: eksa-manager-role
namespace: eksa-packages
version: v1
Comment on lines +19 to +38
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that every time we add a role for a different namespace we would need to add a couple entries here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we would need to patch the name of the role for other namespaces since they never get created if there is already another role with the same name in eksa-system.

10 changes: 8 additions & 2 deletions controllers/cluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
EnableFullLifecycle(ctx context.Context, log logr.Logger, clusterName, kubeConfig string, chart *v1alpha1.Image, registry *registrymirror.RegistryMirror, options ...curatedpackages.PackageControllerClientOpt) error
ReconcileDelete(context.Context, logr.Logger, curatedpackages.KubeDeleter, *anywherev1.Cluster) error
Reconcile(context.Context, logr.Logger, client.Client, *anywherev1.Cluster) error
UpdateSecrets(ctx context.Context, client client.Client, cluster *anywherev1.Cluster) error
}

type ProviderClusterReconcilerRegistry interface {
Expand Down Expand Up @@ -174,7 +175,7 @@
// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch;update
// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;delete
// +kubebuilder:rbac:groups="",namespace=eksa-system,resources=secrets,verbs=patch;update
// +kubebuilder:rbac:groups="",resources=namespaces,verbs=create;delete
// +kubebuilder:rbac:groups="",resources=namespaces,verbs=create;delete;get
// +kubebuilder:rbac:groups="",resources=nodes,verbs=list
// +kubebuilder:rbac:groups=addons.cluster.x-k8s.io,resources=clusterresourcesets,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=anywhere.eks.amazonaws.com,resources=clusters;gitopsconfigs;snowmachineconfigs;snowdatacenterconfigs;snowippools;vspheredatacenterconfigs;vspheremachineconfigs;dockerdatacenterconfigs;tinkerbellmachineconfigs;tinkerbelldatacenterconfigs;cloudstackdatacenterconfigs;cloudstackmachineconfigs;nutanixdatacenterconfigs;nutanixmachineconfigs;awsiamconfigs;oidcconfigs;awsiamconfigs;fluxconfigs,verbs=get;list;watch;update;patch
Expand All @@ -194,7 +195,8 @@
// +kubebuilder:rbac:groups=bmc.tinkerbell.org,resources=machines,verbs=list;watch
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awssnowclusters;awssnowmachinetemplates;awssnowippools;vsphereclusters;vspheremachinetemplates;dockerclusters;dockermachinetemplates;tinkerbellclusters;tinkerbellmachinetemplates;cloudstackclusters;cloudstackmachinetemplates;nutanixclusters;nutanixmachinetemplates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=packages.eks.amazonaws.com,resources=packages,verbs=create;delete;get;list;patch;update;watch
// +kubebuilder:rbac:groups=packages.eks.amazonaws.com,namespace=eksa-system,resources=packagebundlecontrollers,verbs=delete
// +kubebuilder:rbac:groups=packages.eks.amazonaws.com,namespace=eksa-packages,resources=packagebundlecontrollers,verbs=delete
// +kubebuilder:rbac:groups="",namespace=eksa-packages,resources=secrets,verbs=get;patch;update
// +kubebuilder:rbac:groups=anywhere.eks.amazonaws.com,resources=eksareleases,verbs=get;list;watch
// The eksareleases permissions are being moved to the ClusterRole due to client trying to list this resource from cache.
// When trying to list resources not already in cache, it starts an informer for that type using the scope of the cache.
Expand Down Expand Up @@ -386,6 +388,10 @@
}
}

if err := r.packagesClient.UpdateSecrets(ctx, r.client, cluster); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

could this be part of the packagesReconcile method?

Copy link
Member Author

Choose a reason for hiding this comment

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

So one of the issues I found was that the controller's reconcile method sometimes returns before it reaches packagesReconcile which is why I put it here to ensure the secret gets updated.

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. And what would be the problem on waiting until reconcile doesn't return early?

return controller.Result{}, err
}

Check warning on line 393 in controllers/cluster_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/cluster_controller.go#L392-L393

Added lines #L392 - L393 were not covered by tests

return controller.Result{}, nil
}

Expand Down
7 changes: 7 additions & 0 deletions controllers/cluster_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ func TestClusterReconcilerReconcileSelfManagedCluster(t *testing.T) {
registry := newRegistryMock(providerReconciler)
c := fake.NewClientBuilder().WithRuntimeObjects(selfManagedCluster, kcp).Build()
mockPkgs := mocks.NewMockPackagesClient(controller)
mockPkgs.EXPECT().UpdateSecrets(ctx, c, sameName(selfManagedCluster))
providerReconciler.EXPECT().Reconcile(ctx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(selfManagedCluster))
mhcReconciler.EXPECT().Reconcile(ctx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(selfManagedCluster)).Return(nil)

Expand Down Expand Up @@ -367,6 +368,7 @@ func TestClusterReconcilerReconcileConditions(t *testing.T) {
log := testr.New(t)
logCtx := ctrl.LoggerInto(ctx, log)

mockPkgs.EXPECT().UpdateSecrets(logCtx, testClient, sameName(config.Cluster))
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should think about adding a helper to group all expectations? most tests apparently don't care about the expectations, just about the result

iam.EXPECT().EnsureCASecret(logCtx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(config.Cluster)).Return(controller.Result{}, nil)
iam.EXPECT().Reconcile(logCtx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(config.Cluster)).Return(controller.Result{}, nil)
providerReconciler.EXPECT().Reconcile(logCtx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(config.Cluster)).Times(1)
Expand Down Expand Up @@ -625,6 +627,8 @@ func TestClusterReconcilerReconcileSelfManagedClusterConditions(t *testing.T) {
providerReconciler.EXPECT().Reconcile(gomock.Any(), gomock.Any(), gomock.Any()).Times(1)
mhcReconciler.EXPECT().Reconcile(logCtx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(config.Cluster)).Return(nil)

mockPkgs.EXPECT().UpdateSecrets(logCtx, testClient, sameName(config.Cluster))

r := controllers.NewClusterReconciler(testClient, registry, iam, clusterValidator, mockPkgs, mhcReconciler)

result, err := r.Reconcile(logCtx, clusterRequest(config.Cluster))
Expand Down Expand Up @@ -772,6 +776,7 @@ func TestClusterReconcilerReconcileGenerations(t *testing.T) {
mhcReconciler := mocks.NewMockMachineHealthCheckReconciler(mockCtrl)

if tt.wantReconciliation {
mockPkgs.EXPECT().UpdateSecrets(ctx, client, sameName(config.Cluster))
iam.EXPECT().EnsureCASecret(ctx, gomock.AssignableToTypeOf(logr.Logger{}), gomock.AssignableToTypeOf(config.Cluster)).Return(controller.Result{}, nil)
iam.EXPECT().Reconcile(ctx, gomock.AssignableToTypeOf(logr.Logger{}), gomock.AssignableToTypeOf(config.Cluster)).Return(controller.Result{}, nil)
providerReconciler.EXPECT().Reconcile(ctx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(config.Cluster)).Times(1)
Expand Down Expand Up @@ -1140,6 +1145,7 @@ func TestClusterReconcilerSkipDontInstallPackagesOnSelfManaged(t *testing.T) {

ctrl := gomock.NewController(t)
mockPkgs := mocks.NewMockPackagesClient(ctrl)
mockPkgs.EXPECT().UpdateSecrets(ctx, mockClient, sameName(cluster))
mockPkgs.EXPECT().ReconcileDelete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
mhcReconciler := mocks.NewMockMachineHealthCheckReconciler(ctrl)
mhcReconciler.EXPECT().Reconcile(ctx, gomock.Any(), sameName(cluster)).Return(nil)
Expand Down Expand Up @@ -1318,6 +1324,7 @@ func TestClusterReconcilerPackagesInstall(s *testing.T) {

mhcReconciler.EXPECT().Reconcile(logCtx, gomock.AssignableToTypeOf(logr.Logger{}), sameName(cluster)).Return(nil)

mockPkgs.EXPECT().UpdateSecrets(logCtx, fakeClient, sameName(cluster))
mockPkgs.EXPECT().
EnableFullLifecycle(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
Times(0)
Expand Down
3 changes: 3 additions & 0 deletions controllers/cluster_controller_test_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ func TestClusterReconcilerEnsureOwnerReferences(t *testing.T) {
validator.EXPECT().ValidateManagementClusterName(ctx, gomock.AssignableToTypeOf(logr.Logger{}), gomock.AssignableToTypeOf(cluster)).Return(nil)

pcc := newMockPackagesClient(t)
pcc.EXPECT().UpdateSecrets(ctx, cl, sameName(cluster))
pcc.EXPECT().Reconcile(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

mhc := newMockMachineHealthCheckReconciler(t)
Expand Down Expand Up @@ -312,6 +313,7 @@ func TestClusterReconcilerSetBundlesRef(t *testing.T) {
g.Expect(cl.Get(ctx, client.ObjectKey{Namespace: cluster.Spec.BundlesRef.Namespace, Name: cluster.Spec.BundlesRef.Name}, bundles)).To(Succeed())
g.Expect(cl.Get(ctx, client.ObjectKey{Namespace: constants.EksaSystemNamespace, Name: cluster.Name + "-kubeconfig"}, secret)).To(Succeed())
pcc := newMockPackagesClient(t)
pcc.EXPECT().UpdateSecrets(ctx, cl, sameName(cluster))
pcc.EXPECT().Reconcile(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

validator := newMockClusterValidator(t)
Expand Down Expand Up @@ -366,6 +368,7 @@ func TestClusterReconcilerSetDefaultEksaVersion(t *testing.T) {
mgmtCluster := &anywherev1.Cluster{}
g.Expect(cl.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: managementCluster.Name}, mgmtCluster)).To(Succeed())
pcc := newMockPackagesClient(t)
pcc.EXPECT().UpdateSecrets(ctx, cl, sameName(cluster))
pcc.EXPECT().Reconcile(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

validator := newMockClusterValidator(t)
Expand Down
14 changes: 14 additions & 0 deletions controllers/mocks/cluster_controller.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/api/v1alpha1/nodeupgrade_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type NodeUpgradeSpec struct {
Machine corev1.ObjectReference `json:"machine"`

// TODO(in-place): Determine if there's a way to get these dynamically instead of expecting it from the CRD.

KubernetesVersion string `json:"kubernetesVersion"`
EtcdVersion *string `json:"etcdVersion,omitempty"`
CoreDNSVersion *string `json:"coreDNSVersion,omitempty"`
Expand Down
5 changes: 5 additions & 0 deletions pkg/clustermanager/cluster_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,11 @@
return c.clusterClient.CreateNamespaceIfNotPresent(ctx, cluster.KubeconfigFile, constants.EksaSystemNamespace)
}

// CreatePackagesNamespace creates the eksa-packages namespace on the cluster if it doesn't already exist.
func (c *ClusterManager) CreatePackagesNamespace(ctx context.Context, cluster *types.Cluster) error {
Copy link
Member

Choose a reason for hiding this comment

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

curious, why is this needed now?
doesn't the "install packages" functionality (through helm) create this ns?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the namespace before installing packages or else we can't add role/rolebindings in that ns

return c.clusterClient.CreateNamespaceIfNotPresent(ctx, cluster.KubeconfigFile, constants.EksaPackagesName)

Check warning on line 1142 in pkg/clustermanager/cluster_manager.go

View check run for this annotation

Codecov / codecov/patch

pkg/clustermanager/cluster_manager.go#L1141-L1142

Added lines #L1141 - L1142 were not covered by tests
}

// CreateEKSAResources applies the eks-a cluster specs (cluster, datacenterconfig, machine configs, etc.), as well as the
// release bundle to the cluster. Before applying the spec, we pause eksa controller cluster and datacenter webhook validation
// so that the cluster spec can be created or updated in the cluster without webhook validation error.
Expand Down
4 changes: 2 additions & 2 deletions pkg/clustermanager/eksa_installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestEKSAInstallerInstallSuccessWithRealManifest(t *testing.T) {
tt := newInstallerTest(t)
tt.newSpec.VersionsBundles["1.19"].Eksa.Components.URI = "../../config/manifest/eksa-components.yaml"
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.AssignableToTypeOf(&appsv1.Deployment{}))
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.Any()).Times(37) // there are 37 objects in the manifest
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.Any()).Times(39) // there are 39 objects in the manifest
tt.client.EXPECT().WaitForDeployment(tt.ctx, tt.cluster, "30m0s", "Available", "eksa-controller-manager", "eksa-system")

tt.Expect(tt.installer.Install(tt.ctx, test.NewNullLogger(), tt.cluster, tt.newSpec)).To(Succeed())
Expand Down Expand Up @@ -146,7 +146,7 @@ func TestEKSAInstallerInstallSuccessWithNoTimeout(t *testing.T) {
tt := newInstallerTest(t, clustermanager.WithEKSAInstallerNoTimeouts())
tt.newSpec.VersionsBundles["1.19"].Eksa.Components.URI = "../../config/manifest/eksa-components.yaml"
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.AssignableToTypeOf(&appsv1.Deployment{}))
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.Any()).Times(37) // there are 37 objects in the manifest
tt.client.EXPECT().Apply(tt.ctx, tt.cluster.KubeconfigFile, gomock.Any()).Times(39) // there are 39 objects in the manifest
tt.client.EXPECT().WaitForDeployment(tt.ctx, tt.cluster, maxTime.String(), "Available", "eksa-controller-manager", "eksa-system")

tt.Expect(tt.installer.Install(tt.ctx, test.NewNullLogger(), tt.cluster, tt.newSpec)).To(Succeed())
Expand Down
Loading