Skip to content

Remove clusterID from installconfig and move it to cluster #783

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

Closed
rajatchopra wants to merge 1 commit into from
Closed

Remove clusterID from installconfig and move it to cluster #783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/asset/cluster/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import (
)

// Metadata converts an install configuration to AWS metadata.
func Metadata(config *types.InstallConfig) *aws.Metadata {
func Metadata(clusterID string, config *types.InstallConfig) *aws.Metadata {
return &aws.Metadata{
Region: config.Platform.AWS.Region,
Identifier: []map[string]string{
{
"openshiftClusterID": config.ClusterID,
"openshiftClusterID": clusterID,
},
{
fmt.Sprintf("kubernetes.io/cluster/%s", config.ObjectMeta.Name): "owned",
Expand Down
9 changes: 6 additions & 3 deletions pkg/asset/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func (c *Cluster) Name() string {
// the cluster.
func (c *Cluster) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
&TerraformVariables{},
&password.KubeadminPassword{},
Expand All @@ -55,10 +56,11 @@ func (c *Cluster) Dependencies() []asset.Asset {

// Generate launches the cluster and generates the terraform state file on disk.
func (c *Cluster) Generate(parents asset.Parents) (err error) {
clusterID := &installconfig.ClusterID{}
installConfig := &installconfig.InstallConfig{}
terraformVariables := &TerraformVariables{}
kubeadminPassword := &password.KubeadminPassword{}
parents.Get(installConfig, terraformVariables, kubeadminPassword)
parents.Get(clusterID, installConfig, terraformVariables, kubeadminPassword)

if installConfig.Config.Platform.None != nil {
return errors.New("cluster cannot be created with platform set to 'none'")
Expand All @@ -78,6 +80,7 @@ func (c *Cluster) Generate(parents asset.Parents) (err error) {

metadata := &types.ClusterMetadata{
ClusterName: installConfig.Config.ObjectMeta.Name,
ClusterID: clusterID.ClusterID,
}

defer func() {
Expand All @@ -103,11 +106,11 @@ func (c *Cluster) Generate(parents asset.Parents) (err error) {

switch {
case installConfig.Config.Platform.AWS != nil:
metadata.ClusterPlatformMetadata.AWS = aws.Metadata(installConfig.Config)
metadata.ClusterPlatformMetadata.AWS = aws.Metadata(clusterID.ClusterID, installConfig.Config)
case installConfig.Config.Platform.Libvirt != nil:
metadata.ClusterPlatformMetadata.Libvirt = libvirt.Metadata(installConfig.Config)
case installConfig.Config.Platform.OpenStack != nil:
metadata.ClusterPlatformMetadata.OpenStack = openstack.Metadata(installConfig.Config)
metadata.ClusterPlatformMetadata.OpenStack = openstack.Metadata(clusterID.ClusterID, installConfig.Config)
default:
return fmt.Errorf("no known platform")
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/asset/cluster/openstack/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import (
)

// Metadata converts an install configuration to OpenStack metadata.
func Metadata(config *types.InstallConfig) *openstack.Metadata {
func Metadata(clusterID string, config *types.InstallConfig) *openstack.Metadata {
return &openstack.Metadata{
Region: config.Platform.OpenStack.Region,
Cloud: config.Platform.OpenStack.Cloud,
Identifier: map[string]string{
"openshiftClusterID": config.ClusterID,
"openshiftClusterID": clusterID,
},
}
}
6 changes: 4 additions & 2 deletions pkg/asset/cluster/tfvars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func (t *TerraformVariables) Name() string {
// Dependencies returns the dependency of the TerraformVariable
func (t *TerraformVariables) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
&bootstrap.Bootstrap{},
&machine.Master{},
Expand All @@ -41,16 +42,17 @@ func (t *TerraformVariables) Dependencies() []asset.Asset {

// Generate generates the terraform.tfvars file.
func (t *TerraformVariables) Generate(parents asset.Parents) error {
clusterID := &installconfig.ClusterID{}
installConfig := &installconfig.InstallConfig{}
bootstrap := &bootstrap.Bootstrap{}
master := &machine.Master{}
parents.Get(installConfig, bootstrap, master)
parents.Get(clusterID, installConfig, bootstrap, master)

bootstrapIgn := string(bootstrap.Files()[0].Data)

masterIgn := string(master.Files()[0].Data)

data, err := tfvars.TFVars(installConfig.Config, bootstrapIgn, masterIgn)
data, err := tfvars.TFVars(clusterID.ClusterID, installConfig.Config, bootstrapIgn, masterIgn)
if err != nil {
return errors.Wrap(err, "failed to get Tfvars")
}
Expand Down
11 changes: 6 additions & 5 deletions pkg/asset/installconfig/clusterid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,25 @@ import (
"github.com/openshift/installer/pkg/asset"
)

type clusterID struct {
// ClusterID is the unique ID of the cluster, immutable during the cluster's life
type ClusterID struct {
Copy link
Contributor

@abhinavdahiya abhinavdahiya Dec 5, 2018

Choose a reason for hiding this comment

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

why not just type ClusterID string ?

Copy link
Author

@rajatchopra rajatchopra Dec 14, 2018

Choose a reason for hiding this comment

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

Just for uniformity of initialization, here for example by replacing installconfig.ClusterID{} with new(installconfig.ClusterID)
Also looks kludgy to dereference and assign 'self' in a class function here when it appears like *a = uuid.New()

The sizeof() should also not be different, but drop one hint and I can change it, I am fine either way.

Copy link
Contributor

@staebler staebler Dec 14, 2018

Choose a reason for hiding this comment

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

It makes sense to me to have ClusterID be a struct. The type is not the cluster ID itself: The type is the asset that generates the cluster ID.

Copy link
Contributor

@staebler staebler Dec 14, 2018

Choose a reason for hiding this comment

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

There is precedent for this in all the other assets. For comparison, installconfig.baseDomain could be a string instead of a struct, too.

ClusterID string
}

var _ asset.Asset = (*clusterID)(nil)
var _ asset.Asset = (*ClusterID)(nil)

// Dependencies returns no dependencies.
func (a *clusterID) Dependencies() []asset.Asset {
func (a *ClusterID) Dependencies() []asset.Asset {
return []asset.Asset{}
}

// Generate generates a new UUID
func (a *clusterID) Generate(asset.Parents) error {
func (a *ClusterID) Generate(asset.Parents) error {
a.ClusterID = uuid.New()
return nil
}

// Name returns the human-friendly name of the asset.
func (a *clusterID) Name() string {
func (a *ClusterID) Name() string {
return "Cluster ID"
}
4 changes: 0 additions & 4 deletions pkg/asset/installconfig/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ var _ asset.WritableAsset = (*InstallConfig)(nil)
// InstallConfig asset.
func (a *InstallConfig) Dependencies() []asset.Asset {
return []asset.Asset{
&clusterID{},
&sshPublicKey{},
&baseDomain{},
&clusterName{},
Expand All @@ -52,14 +51,12 @@ func (a *InstallConfig) Dependencies() []asset.Asset {

// Generate generates the install-config.yaml file.
func (a *InstallConfig) Generate(parents asset.Parents) error {
clusterID := &clusterID{}
sshPublicKey := &sshPublicKey{}
baseDomain := &baseDomain{}
clusterName := &clusterName{}
pullSecret := &pullSecret{}
platform := &platform{}
parents.Get(
clusterID,
sshPublicKey,
baseDomain,
clusterName,
Expand All @@ -71,7 +68,6 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
ObjectMeta: metav1.ObjectMeta{
Name: clusterName.ClusterName,
},
ClusterID: clusterID.ClusterID,
SSHKey: sshPublicKey.Key,
BaseDomain: baseDomain.BaseDomain,
Networking: types.Networking{
Expand Down
4 changes: 2 additions & 2 deletions pkg/asset/machines/aws/machines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
)

// Machines returns a list of machines for a machinepool.
func Machines(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error) {
func Machines(clusterID string, config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error) {
if configPlatform := config.Platform.Name(); configPlatform != aws.Name {
return nil, fmt.Errorf("non-AWS configuration: %q", configPlatform)
}
Expand All @@ -37,7 +37,7 @@ func Machines(config *types.InstallConfig, pool *types.MachinePool, role, userDa
var machines []clusterapi.Machine
for idx := int64(0); idx < total; idx++ {
azIndex := int(idx) % len(azs)
provider, err := provider(config.ClusterID, clustername, platform, mpool, azIndex, role, userDataSecret)
provider, err := provider(clusterID, clustername, platform, mpool, azIndex, role, userDataSecret)
if err != nil {
return nil, errors.Wrap(err, "failed to create provider")
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/asset/machines/aws/machinesets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

// MachineSets returns a list of machinesets for a machinepool.
func MachineSets(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error) {
func MachineSets(clusterID string, config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error) {
if configPlatform := config.Platform.Name(); configPlatform != aws.Name {
return nil, fmt.Errorf("non-AWS configuration: %q", configPlatform)
}
Expand All @@ -38,7 +38,7 @@ func MachineSets(config *types.InstallConfig, pool *types.MachinePool, role, use
replicas++
}

provider, err := provider(config.ClusterID, clustername, platform, mpool, idx, role, userDataSecret)
provider, err := provider(clusterID, clustername, platform, mpool, idx, role, userDataSecret)
if err != nil {
return nil, errors.Wrap(err, "failed to create provider")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/machines/libvirt/machines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

// Machines returns a list of machines for a machinepool.
func Machines(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error) {
func Machines(clusterID string, config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error) {
if configPlatform := config.Platform.Name(); configPlatform != libvirt.Name {
return nil, fmt.Errorf("non-Libvirt configuration: %q", configPlatform)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/machines/libvirt/machinesets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

// MachineSets returns a list of machinesets for a machinepool.
func MachineSets(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error) {
func MachineSets(clusterID string, config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error) {
if configPlatform := config.Platform.Name(); configPlatform != libvirt.Name {
return nil, fmt.Errorf("non-Libvirt configuration: %q", configPlatform)
}
Expand Down
10 changes: 6 additions & 4 deletions pkg/asset/machines/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,18 @@ func (m *Master) Name() string {
// Master asset
func (m *Master) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
&machine.Master{},
}
}

// Generate generates the Master asset.
func (m *Master) Generate(dependencies asset.Parents) error {
clusterID := &installconfig.ClusterID{}
installconfig := &installconfig.InstallConfig{}
mign := &machine.Master{}
dependencies.Get(installconfig, mign)
dependencies.Get(clusterID, installconfig, mign)

var err error
userDataMap := map[string][]byte{"master-user-data": mign.File.Data}
Expand Down Expand Up @@ -84,7 +86,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
mpool.Zones = azs
}
pool.Platform.AWS = &mpool
machines, err := aws.Machines(ic, &pool, "master", "master-user-data")
machines, err := aws.Machines(clusterID.ClusterID, ic, &pool, "master", "master-user-data")
if err != nil {
return errors.Wrap(err, "failed to create master machine objects")
}
Expand All @@ -97,7 +99,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
}
m.MachinesRaw = raw
case libvirttypes.Name:
machines, err := libvirt.Machines(ic, &pool, "master", "master-user-data")
machines, err := libvirt.Machines(clusterID.ClusterID, ic, &pool, "master", "master-user-data")
if err != nil {
return errors.Wrap(err, "failed to create master machine objects")
}
Expand Down Expand Up @@ -128,7 +130,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
}

tags := map[string]string{
"openshiftClusterID": ic.ClusterID,
"openshiftClusterID": clusterID.ClusterID,
}
config.Tags = tags

Expand Down
14 changes: 8 additions & 6 deletions pkg/asset/machines/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,18 @@ func (w *Worker) Name() string {
// Worker asset
func (w *Worker) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
&machine.Worker{},
}
}

// Generate generates the Worker asset.
func (w *Worker) Generate(dependencies asset.Parents) error {
installconfig := &installconfig.InstallConfig{}
clusterID := &installconfig.ClusterID{}
installConfig := &installconfig.InstallConfig{}
wign := &machine.Worker{}
dependencies.Get(installconfig, wign)
dependencies.Get(clusterID, installConfig, wign)

var err error
userDataMap := map[string][]byte{"worker-user-data": wign.File.Data}
Expand All @@ -83,7 +85,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
return errors.Wrap(err, "failed to create user-data secret for worker machines")
}

ic := installconfig.Config
ic := installConfig.Config
pool := workerPool(ic.Machines)
switch ic.Platform.Name() {
case awstypes.Name:
Expand All @@ -107,7 +109,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
mpool.Zones = azs
}
pool.Platform.AWS = &mpool
sets, err := aws.MachineSets(ic, &pool, "worker", "worker-user-data")
sets, err := aws.MachineSets(clusterID.ClusterID, ic, &pool, "worker", "worker-user-data")
if err != nil {
return errors.Wrap(err, "failed to create worker machine objects")
}
Expand All @@ -119,7 +121,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
}
w.MachineSetRaw = raw
case libvirttypes.Name:
sets, err := libvirt.MachineSets(ic, &pool, "worker", "worker-user-data")
sets, err := libvirt.MachineSets(clusterID.ClusterID, ic, &pool, "worker", "worker-user-data")
if err != nil {
return errors.Wrap(err, "failed to create worker machine objects")
}
Expand All @@ -146,7 +148,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
}

tags := map[string]string{
"openshiftClusterID": ic.ClusterID,
"openshiftClusterID": clusterID.ClusterID,
}
config.Tags = tags

Expand Down
5 changes: 4 additions & 1 deletion pkg/asset/manifests/operators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func (m *Manifests) Name() string {
// Manifests asset.
func (m *Manifests) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
&Ingress{},
&DNS{},
Expand Down Expand Up @@ -121,6 +122,7 @@ func (m *Manifests) Files() []*asset.File {
}

func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*asset.File {
clusterID := &installconfig.ClusterID{}
installConfig := &installconfig.InstallConfig{}
etcdCA := &tls.EtcdCA{}
kubeCA := &tls.KubeCA{}
Expand All @@ -129,6 +131,7 @@ func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*ass
rootCA := &tls.RootCA{}
serviceServingCA := &tls.ServiceServingCA{}
dependencies.Get(
clusterID,
installConfig,
etcdCA,
etcdClientCertKey,
Expand Down Expand Up @@ -156,7 +159,7 @@ func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*ass
RootCaCert: string(rootCA.Cert()),
ServiceServingCaCert: base64.StdEncoding.EncodeToString(serviceServingCA.Cert()),
ServiceServingCaKey: base64.StdEncoding.EncodeToString(serviceServingCA.Key()),
CVOClusterID: installConfig.Config.ClusterID,
CVOClusterID: clusterID.ClusterID,
EtcdEndpointHostnames: etcdEndpointHostnames,
EtcdEndpointDNSSuffix: installConfig.Config.BaseDomain,
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/tfvars/tfvars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ type config struct {

// TFVars converts the InstallConfig and Ignition content to
// terraform.tfvar JSON.
func TFVars(cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, error) {
func TFVars(clusterID string, cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, error) {
config := &config{
ClusterID: cfg.ClusterID,
ClusterID: clusterID,
Name: cfg.ObjectMeta.Name,
BaseDomain: cfg.BaseDomain,
MachineCIDR: cfg.Networking.MachineCIDR.String(),
Expand Down
1 change: 1 addition & 0 deletions pkg/types/clustermetadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
// regarding the cluster that was created by installer.
type ClusterMetadata struct {
ClusterName string `json:"clusterName"`
ClusterID string `json:"clusterID"`
ClusterPlatformMetadata `json:",inline"`
}

Expand Down
3 changes: 0 additions & 3 deletions pkg/types/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ type InstallConfig struct {

metav1.ObjectMeta `json:"metadata"`

// ClusterID is the ID of the cluster.
ClusterID string `json:"clusterID"`

// SSHKey is the public ssh key to provide access to instances.
SSHKey string `json:"sshKey"`

Expand Down
3 changes: 0 additions & 3 deletions pkg/types/validation/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ func ValidateInstallConfig(c *types.InstallConfig, openStackValidValuesFetcher o
if c.ObjectMeta.Name == "" {
allErrs = append(allErrs, field.Required(field.NewPath("metadata", "name"), "cluster name required"))
}
if c.ClusterID == "" {
allErrs = append(allErrs, field.Required(field.NewPath("clusterID"), "cluster ID required"))
}
if c.SSHKey != "" {
if err := validate.SSHPublicKey(c.SSHKey); err != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("sshKey"), c.SSHKey, err.Error()))
Expand Down