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

ARO-4518 pass custom manifests(MIWI) to hive cluster deployment as secret #3841

Merged
merged 2 commits into from
Sep 17, 2024
Merged

ARO-4518 pass custom manifests(MIWI) to hive cluster deployment as secret #3841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43bbf18
ARO-4518 pass custom manifests to hive cluster deployment as secret
rajdeepc2792 Sep 13, 2024
6b916b5
ARO-4518 add unit test cases
rajdeepc2792 Sep 16, 2024
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
11 changes: 10 additions & 1 deletion pkg/cluster/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
securityclient "github.com/openshift/client-go/security/clientset/versioned"
mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
extensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
kruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -275,10 +276,18 @@ func (m *manager) runHiveInstaller(ctx context.Context) error {
return err
}

var customManifests map[string]kruntime.Object
if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
customManifests, err = m.generateWorkloadIdentityResources()
if err != nil {
return err
}
}

// Run installer. For M5/M6 we will persist the graph inside the installer
// code since it's easier, but in the future, this data should be collected
// from Hive's outputs where needed.
return m.hiveClusterManager.Install(ctx, m.subscriptionDoc, m.doc, version)
return m.hiveClusterManager.Install(ctx, m.subscriptionDoc, m.doc, version, customManifests)
}

func setFieldCreatedByHive(createdByHive bool) database.OpenShiftClusterDocumentMutator {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cluster/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ func TestRunHiveInstallerSetsCreatedByHiveFieldToTrueInClusterDoc(t *testing.T)
defer controller.Finish()

hiveClusterManagerMock := mock_hive.NewMockClusterManager(controller)
hiveClusterManagerMock.EXPECT().Install(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
hiveClusterManagerMock.EXPECT().Install(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())

m := &manager{
doc: dequeuedDoc,
Expand Down
36 changes: 22 additions & 14 deletions pkg/hive/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,13 @@ func makeEnvSecret(name string) corev1.EnvVar {
}
}

func (c *clusterManager) Install(ctx context.Context, sub *api.SubscriptionDocument, doc *api.OpenShiftClusterDocument, version *api.OpenShiftVersion) error {
sppSecret, err := servicePrincipalSecretForInstall(doc.OpenShiftCluster, sub, c.env.IsLocalDevelopmentMode())
func (c *clusterManager) Install(ctx context.Context, sub *api.SubscriptionDocument, doc *api.OpenShiftClusterDocument, version *api.OpenShiftVersion, customManifests map[string]kruntime.Object) error {
azureCredentialSecret, err := azureCredentialSecretForInstall(doc.OpenShiftCluster, sub, c.env.IsLocalDevelopmentMode())
cadenmarchese marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}

manifestsSecret, err := clusterManifestsSecret(doc.OpenShiftCluster.Properties.HiveProfile.Namespace, customManifests)
if err != nil {
return err
}
Expand All @@ -76,7 +81,8 @@ func (c *clusterManager) Install(ctx context.Context, sub *api.SubscriptionDocum
}

resources := []kruntime.Object{
sppSecret,
azureCredentialSecret,
manifestsSecret,
envSecret(doc.OpenShiftCluster.Properties.HiveProfile.Namespace, c.env.IsLocalDevelopmentMode()),
psSecret,
installConfigCM(doc.OpenShiftCluster.Properties.HiveProfile.Namespace, doc.OpenShiftCluster.Location),
Expand All @@ -96,25 +102,24 @@ func (c *clusterManager) Install(ctx context.Context, sub *api.SubscriptionDocum
return nil
}

func servicePrincipalSecretForInstall(oc *api.OpenShiftCluster, sub *api.SubscriptionDocument, isDevelopment bool) (*corev1.Secret, error) {
clusterSPBytes, err := clusterSPToBytes(sub, oc)
func azureCredentialSecretForInstall(oc *api.OpenShiftCluster, sub *api.SubscriptionDocument, isDevelopment bool) (*corev1.Secret, error) {
enc, err := json.Marshal(oc)
if err != nil {
return nil, err
}

enc, err := json.Marshal(oc)
encSub, err := json.Marshal(sub.Subscription)
if err != nil {
return nil, err
}

encSub, err := json.Marshal(sub.Subscription)
azureCredentialSecret, err := clusterAzureSecret(oc.Properties.HiveProfile.Namespace, oc, sub)
if err != nil {
return nil, err
}

sppSecret := clusterServicePrincipalSecret(oc.Properties.HiveProfile.Namespace, clusterSPBytes)
sppSecret.Data["99_aro.json"] = enc
sppSecret.Data["99_sub.json"] = encSub
azureCredentialSecret.Data["99_aro.json"] = enc
azureCredentialSecret.Data["99_sub.json"] = encSub

if isDevelopment {
// In development mode, load in the proxy certificates so that clusters
Expand Down Expand Up @@ -146,12 +151,12 @@ func servicePrincipalSecretForInstall(oc *api.OpenShiftCluster, sub *api.Subscri
return nil, err
}

sppSecret.Data["proxy.crt"] = proxyCert
sppSecret.Data["proxy-client.crt"] = proxyClientCert
sppSecret.Data["proxy-client.key"] = proxyClientKey
azureCredentialSecret.Data["proxy.crt"] = proxyCert
azureCredentialSecret.Data["proxy-client.crt"] = proxyClientCert
azureCredentialSecret.Data["proxy-client.key"] = proxyClientKey
}

return sppSecret, nil
return azureCredentialSecret, nil
}

func (c *clusterManager) clusterDeploymentForInstall(doc *api.OpenShiftClusterDocument, version *api.OpenShiftVersion, isDevelopment bool) *hivev1.ClusterDeployment {
Expand Down Expand Up @@ -227,6 +232,9 @@ func (c *clusterManager) clusterDeploymentForInstall(doc *api.OpenShiftClusterDo
Name: installConfigName,
},
InstallerEnv: envVars,
ManifestsSecretRef: &corev1.LocalObjectReference{
Name: clusterManifestsSecretName,
},
},
PreserveOnDelete: true,
ManageDNS: false,
Expand Down
3 changes: 2 additions & 1 deletion pkg/hive/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
corev1 "k8s.io/api/core/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/util/retry"
Expand All @@ -37,7 +38,7 @@ type ClusterManager interface {
Delete(ctx context.Context, doc *api.OpenShiftClusterDocument) error
// Install creates a ClusterDocument and related secrets for a new cluster
// so that it can be provisioned by Hive.
Install(ctx context.Context, sub *api.SubscriptionDocument, doc *api.OpenShiftClusterDocument, version *api.OpenShiftVersion) error
Install(ctx context.Context, sub *api.SubscriptionDocument, doc *api.OpenShiftClusterDocument, version *api.OpenShiftVersion, customManifests map[string]kruntime.Object) error
IsClusterDeploymentReady(ctx context.Context, doc *api.OpenShiftClusterDocument) (bool, error)
IsClusterInstallationComplete(ctx context.Context, doc *api.OpenShiftClusterDocument) (bool, error)
GetClusterDeployment(ctx context.Context, doc *api.OpenShiftClusterDocument) (*hivev1.ClusterDeployment, error)
Expand Down
55 changes: 44 additions & 11 deletions pkg/hive/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kruntime "k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/yaml"

"github.com/Azure/ARO-RP/pkg/api"
utillog "github.com/Azure/ARO-RP/pkg/util/log"
Expand All @@ -24,6 +25,7 @@ const (
ClusterDeploymentName = "cluster"
aroServiceKubeconfigSecretName = "aro-service-kubeconfig-secret"
clusterServicePrincipalSecretName = "cluster-service-principal-secret"
clusterManifestsSecretName = "cluster-manifests-secret"
)

var (
Expand Down Expand Up @@ -56,10 +58,6 @@ var (

func (hr *clusterManager) resources(sub *api.SubscriptionDocument, doc *api.OpenShiftClusterDocument) ([]kruntime.Object, error) {
namespace := doc.OpenShiftCluster.Properties.HiveProfile.Namespace
clusterSP, err := clusterSPToBytes(sub, doc.OpenShiftCluster)
if err != nil {
return nil, err
}

cd := adoptedClusterDeployment(
namespace,
Expand All @@ -69,7 +67,7 @@ func (hr *clusterManager) resources(sub *api.SubscriptionDocument, doc *api.Open
doc.OpenShiftCluster.Location,
doc.OpenShiftCluster.Properties.NetworkProfile.APIServerPrivateEndpointIP,
)
err = utillog.EnrichHiveWithCorrelationData(cd, doc.CorrelationData)
err := utillog.EnrichHiveWithCorrelationData(cd, doc.CorrelationData)
if err != nil {
return nil, err
}
Expand All @@ -78,9 +76,14 @@ func (hr *clusterManager) resources(sub *api.SubscriptionDocument, doc *api.Open
return nil, err
}

azureCredentialSecret, err := clusterAzureSecret(namespace, doc.OpenShiftCluster, sub)
if err != nil {
return nil, err
}

return []kruntime.Object{
aroServiceKubeconfigSecret(namespace, doc.OpenShiftCluster.Properties.AROServiceKubeconfig),
clusterServicePrincipalSecret(namespace, clusterSP),
azureCredentialSecret,
cd,
}, nil
}
Expand All @@ -98,17 +101,47 @@ func aroServiceKubeconfigSecret(namespace string, kubeConfig []byte) *corev1.Sec
}
}

func clusterServicePrincipalSecret(namespace string, secret []byte) *corev1.Secret {
return &corev1.Secret{
func clusterAzureSecret(namespace string, oc *api.OpenShiftCluster, sub *api.SubscriptionDocument) (*corev1.Secret, error) {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: clusterServicePrincipalSecretName,
Namespace: namespace,
},
Data: map[string][]byte{
"osServicePrincipal.json": secret,
},
Data: map[string][]byte{},
Type: corev1.SecretTypeOpaque,
}

// Add osServicePrincipal.json only when cluster is not managed identity
if !oc.UsesWorkloadIdentity() {
clusterSPBytes, err := clusterSPToBytes(sub, oc)
if err != nil {
return nil, err
}
secret.Data["osServicePrincipal.json"] = clusterSPBytes
}

return secret, nil
}

func clusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit - if this makes sense to you, I suggest a rename of this function to avoid confusion with the clusterManifestsSecret var:

Suggested change
func clusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {
func generateClusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of changing the function name, changed the var name:- https://github.com/Azure/ARO-RP/pull/3841/files#diff-f83dd6c00cfc7b417ce96e6f7bfbe58ce118101ea90630f21d658abf3e96b129R60

generateClusterManifestsSecret name is similar to https://github.com/Azure/ARO-RP/pull/3841/files#diff-71a1dd610bf07a9e4ece9f782af2bad11e6f2d0e03df442019088f8c47265497R281 where the actual generation is taking place and keeping it similar to similar type of secret creations in the file.

secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: clusterManifestsSecretName,
Namespace: namespace,
},
StringData: map[string]string{},
Type: corev1.SecretTypeOpaque,
}

for key, manifest := range customManifests {
b, err := yaml.Marshal(manifest)
if err != nil {
return nil, err
}

secret.StringData[key] = string(b)
}
return secret, nil
}

func envSecret(namespace string, isDevelopment bool) *corev1.Secret {
Expand Down
94 changes: 94 additions & 0 deletions pkg/hive/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ import (
"testing"

"github.com/go-test/deep"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kruntime "k8s.io/apimachinery/pkg/runtime"

"github.com/Azure/ARO-RP/pkg/api"
)

func TestInstallConfigMap(t *testing.T) {
Expand All @@ -18,3 +23,92 @@ func TestInstallConfigMap(t *testing.T) {
t.Error(err)
}
}

func TestClusterManifestsSecret(t *testing.T) {
var expected = map[string]string{"custom.yaml": "apiVersion: v1\nkind: Secret\nmetadata:\n creationTimestamp: null\n name: demo-credentials\n namespace: default\nstringData:\n demo1: value1\n demo2: value2\ntype: Opaque\n"}
customManifests := map[string]kruntime.Object{
"custom.yaml": &corev1.Secret{
TypeMeta: metav1.TypeMeta{
APIVersion: corev1.SchemeGroupVersion.Identifier(),
Kind: "Secret",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "demo-credentials",
},
Type: corev1.SecretTypeOpaque,
StringData: map[string]string{
"demo1": "value1",
"demo2": "value2",
},
},
}

r, _ := clusterManifestsSecret("testNamespace", customManifests)

for _, err := range deep.Equal(r.StringData, expected) {
t.Error(err)
}
if r.Namespace != "testNamespace" {
t.Errorf("Incorrect Secret namespace, expected: testNamespace, found %s", r.Namespace)
}
}

func TestClusterAzureSecret(t *testing.T) {
for _, tt := range []struct {
name string
oc *api.OpenShiftCluster
wantSP bool
}{
{
name: "Successfully return secret - MIWI Cluster",
oc: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{},
},
},
wantSP: false,
},
{
name: "Successfully return secret - Non-MIWI Cluster",
oc: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
ServicePrincipalProfile: &api.ServicePrincipalProfile{
ClientID: "clientid",
ClientSecret: api.SecureString("clientsecret"),
},
},
},
wantSP: true,
},
{
name: "Failed returning secret - Non-MIWI Cluster, No Credentials",
oc: &api.OpenShiftCluster{
Properties: api.OpenShiftClusterProperties{
ServicePrincipalProfile: &api.ServicePrincipalProfile{},
},
},
wantSP: true,
},
} {
t.Run(tt.name, func(t *testing.T) {
subDoc := api.SubscriptionDocument{
Subscription: &api.Subscription{
Properties: &api.SubscriptionProperties{
TenantID: "tenantid",
},
},
ID: "fakeID",
}
r, _ := clusterAzureSecret("testNamespace", tt.oc, &subDoc)

_, ok := r.Data["osServicePrincipal.json"]
if tt.wantSP != ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You can use assert pkg.

ARO-RP/pkg/cluster/networkprofile_test.go

Lines 294 to 295 in 5370de9

assert.Equal(t, tt.expectedErr, err, "Unexpected error exception")
assert.Equal(t, tt.expectedMTU, tt.m.doc.OpenShiftCluster.Properties.NetworkProfile.MTUSize, "MTUSize was not updated as expected exception")

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I'm personally in favor of using the assert pkg here, I don't think we have a consistent pattern for these assertions across our codebase's test cases, so I'm fine either way with changing this or letting it merge as-is.

t.Errorf("Wanted %t got %t", tt.wantSP, ok)
}
if r.Namespace != "testNamespace" {
t.Errorf("Incorrect Secret namespace, expected: testNamespace, found %s", r.Namespace)
}
})
}
}
9 changes: 5 additions & 4 deletions pkg/util/mocks/hive/hive.go
View file
Open in desktop

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

Loading