CORS-3220: Prepare GCP CAPI Installation #7978
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-3220 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/label platform/google |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
barbacbd
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
openshift-merge-robot
added
the
needs-rebase
** Added dns resource creation including: - privateManagedZone - Private DNS Records - Public DNS Record (when appropriate)
** Labels are added with the function to add the owned tag ** Network Resources including compute Address for the load balancers and health checks
barbacbd
force-pushed
the
CORS-3220-network-resources
branch
from
460f451 to
0f0965c
Compare
openshift-merge-robot
removed
the
needs-rebase
…r ignition. ** Add creation of Bucket, BucketObject, and Signed URL. ** Add ability to update/set the contents of the Bucket Object after the signed url has been added to the bootstrap machine. This allows us to create the storage object then update its contents after the ignition data can be edited.
barbacbd
force-pushed
the
CORS-3220-network-resources
branch
2 times, most recently
from
dae648f to
4a857a1
Compare
| gcptypes "github.com/openshift/installer/pkg/types/gcp" | ||
| ) | ||
|
|
||
| type Provider struct { |
There was a problem hiding this comment.
We want a build-time check that the Provider type is implementing any interfaces that it intends to implement. E.g. if it implements the Ignition interface, we want to check that at build time:
var _ clusterapi.IgnitionProvider = (*Provider)(nil)| privateZoneName := fmt.Sprintf("%s-private-zone", clusterID.InfraID) | ||
| potentialPrivateZoneName, err := getDNSZoneName(ctx, ic, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if potentialPrivateZoneName != "" { | ||
| privateZoneName = potentialPrivateZoneName | ||
| } |
There was a problem hiding this comment.
Can you add a comment explaining why we need the potentialPrivateZoneName? I believe that is if we are using an existing private zone... and is currently only supported for shared VPC? A quick comment clarifying this might make it more understandable. I think existing or preexisting or user may be better prefix than potential
| apiIntIPAddress, err := createLoadBalancerAddress(ctx, in.InstallConfig, in.InfraID, "", internalLoadBalancer) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| apiIPAddress, err := createLoadBalancerAddress(ctx, in.InstallConfig, in.InfraID, "", externalLoadBalancer) | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
CAPI should be creating one of the load balancers already, and we should create the other one (if applicable). It looks like CAPG does not support "private clusters": kubernetes-sigs/cluster-api-provider-gcp#903, which means it can't create the internal LB.
So at the simplest level, we should just create the internal LB and we will have the split-horizon for a normal install.
But ideally what we want to do is implement private cluster support in CAPG, then add the external LB when it is a public cluster.
| return nil, nil | ||
| } | ||
|
|
||
| func (p Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput) error { |
There was a problem hiding this comment.
I haven't had a chance to review how nodes are added to LBs in GCP, but this InfraReady hook is executed prior to machine creation. I suspect we want to run LB creation in the PostProvision hook so that machines can be joined to the LBs.
| apiIntIPAddress, err := createLoadBalancerAddress(ctx, in.InstallConfig, in.InfraID, "", internalLoadBalancer) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| apiIPAddress, err := createLoadBalancerAddress(ctx, in.InstallConfig, in.InfraID, "", externalLoadBalancer) | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Hm. This is just creating the LB addresses.
- for the capi-provisioned LB, CAPI will publish the ControlPlaneEndpoint value that we can use
- for the non-capi-provisioned LB we will need to create the complete LB
|
we should break apart the ignition and LB implementations into separate prs |
| var credsMap map[string]interface{} | ||
| if err := json.Unmarshal(session.Credentials.JSON, &credsMap); err != nil { | ||
| return "", err | ||
| } | ||
| opts.GoogleAccessID = credsMap["client_email"].(string) | ||
| opts.PrivateKey = []byte(credsMap["private_key"].(string)) |
There was a problem hiding this comment.
We could use a typed struct here instead of map[string]interface? Something like:
type gcpCredentials struct {
ClientEmail string `json:"client_email"`
PrivateKey []byte `json:"private_key"`
}| Expires: time.Now().Add(time.Minute * 60), | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Should we pass the context in?
| Labels: mergeLabels(ic, clusterID), | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, time.Second*60) |
There was a problem hiding this comment.
Should we pass the context in?
|
|
||
| // CreateBucketHandle will create the bucket handle that can be used as a reference for other storage resources. | ||
| func CreateBucketHandle(ctx context.Context, bucketName string) (*storage.BucketHandle, error) { | ||
| ctx, cancel := context.WithTimeout(ctx, time.Second*60) |
There was a problem hiding this comment.
| ctx, cancel := context.WithTimeout(ctx, time.Second*60) | |
| ctx, cancel := context.WithTimeout(ctx, time.Minute*1) |
To be consistent with the timeout below
** Add the ignition stage. The ignition stage will create a GCP storage bucket and a signed url. The Bucket is filled with the original data intended for ignition, and the shim will contain the url.
barbacbd
force-pushed
the
CORS-3220-network-resources
branch
from
4a857a1 to
d2faaf4
Compare
** using the backend service (public load balancer) that is created by CAPG ** Adding the backend service for the private cluster install. This will be removed when capg supports internal/private clusters.
barbacbd
force-pushed
the
CORS-3220-network-resources
branch
from
717037e to
a830807
Compare
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/close |
|
@barbacbd: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
** Added dns resource creation including:
- privateManagedZone
- Private DNS Records
- Public DNS Record (when appropriate)
** Labels are added with the function to add the owned tag
** Network Resources including compute Address for the load balancers and health checks
** Create the resources when control plane nodes are available.