-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enxebre looks awesome. Some comments.
hypershift-operator/controllers/externalinfracluster_controller.go
Outdated
Show resolved
Hide resolved
UncompressedUserData: k8sutilspointer.BoolPtr(true), | ||
CloudInit: capiaws.CloudInit{ | ||
InsecureSkipSecretsManager: true, | ||
SecureSecretsBackend: "secrets-manager", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what does this do?
hypershift-operator/controllers/hosted_controlplane_controller.go
Outdated
Show resolved
Hide resolved
README.md
Outdated
@@ -27,7 +27,8 @@ $ make uninstall | |||
First, create the following files containing secrets used by the example cluster: | |||
|
|||
- `config/example-cluster/pull-secret` a valid pull secret for image pulls. | |||
- `config/example-cluster/ssh-key` an SSH public key for guest node access | |||
- `config/example-cluster/ssh-key` an SSH public key for guest node access. | |||
- `config/example-cluster/aws-creds` a base64 encoded [aws credentials file](https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right place for this secret. It's a secret required by the capa controller deployment, but these secrets are created along with an example cluster, so you won't get a running capa controller until you create the example cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't think it needs to be base64 encoded. Kustomize should base64 encode it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the capa controller down to the operator control. Also the secret is copied now into the target ns similar to what we do with the pull-secret.
This drops openshift machine API in favour of capi machine management. It moves guestInfraCluster to externalInfraCluster to satisfy kubernetes-sigs/cluster-api-provider-aws#2124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enxebre looking good... gave it another pass. Just thinking about the name ExternalInfraCluster ... should we not name it something more specific to AWS ? like 'HostedAWSInfra' or something like that?
Also, since you already made the change upstream to decouple things, and we get the node linking for free with this, I don't see a reason not to merge this when ready. @ironcladlou do you agree?
@@ -0,0 +1,2158 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not needed anymore
} | ||
providerCredsData, hasProviderCredsData := providerCredsSecret.Data["credentials"] | ||
if !hasProviderCredsData { | ||
return fmt.Errorf("pull secret %s is missing the .dockerconfigjson key", hcp.Spec.PullSecret.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong error message
@@ -5,21 +5,20 @@ import ( | |||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event recorder in line 56 should be for "nodepool-controller" https://github.com/openshift-hive/hypershift/pull/31/files#diff-804e9a40d4fe9658dc8326383ec33b1cc577ab536c2b8b6ec58fcbff122a1718L56
@@ -24,18 +24,18 @@ import ( | |||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | |||
) | |||
|
|||
type GuestClusterReconciler struct { | |||
type ExternalInfraClusterReconciler struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event recorder in line 53 should be for "external-infra-cluster-controller" ... https://github.com/openshift-hive/hypershift/pull/31/files#diff-2234353920a980ab201b2fc06e4ef30d221189c9655e2f8922e5877cff1b5325L53
just capturing follow-up from our discussion this morning:
|
The version field is purely an option to override the bootstrap provider value if any https://github.com/kubernetes-sigs/cluster-api/blob/f5939fd5246e7f1682399e8004e5fd[…]65240/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go We don't use it atm, since we generate static ignition which is then injected as a secret into the machineSpec. If we ever have a bootstrap provider for this we might want to use it. |
@enxebre thanks for all this work! |
This drops openshift machine API in favour of capi machine management.
Move guestInfraCluster to externalInfraCluster to satisfy kubernetes-sigs/cluster-api-provider-aws#2124
Run capa controller manager
Enable automation for creating a secret with aws credentials for the guest cluster/machine controller.