[WIP] MGMT-18013: Add kube api support for adding mirror registry in agent cluster install #6646
Conversation
|
@eliorerz: This pull request references MGMT-18013 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.17.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-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@eliorerz: GitHub didn't allow me to request PR reviews from the following users: eliorerz. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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-sigs/prow repository. |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
|
/test ? |
|
@eliorerz: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
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-sigs/prow repository. |
|
/test edge-subsystem-kubeapi-aws |
2 similar comments
|
/test edge-subsystem-kubeapi-aws |
|
/test edge-subsystem-kubeapi-aws |
eliorerz
force-pushed
the
MGMT-18013-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall
branch
2 times, most recently
from
dda73a0
to
64ac54d
Compare
|
/test edge-subsystem-kubeapi-aws |
|
/test edge-subsystem-aws |
eliorerz
force-pushed
the
MGMT-18013-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall
branch
from
64ac54d
to
588df6b
Compare
|
/test edge-subsystem-aws edge-subsystem-kubeapi-aws edge-unit-test |
|
/test edge-lint |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6646 +/- ##
==========================================
- Coverage 68.69% 66.76% -1.94%
==========================================
Files 249 220 -29
Lines 37313 27406 -9907
==========================================
- Hits 25633 18298 -7335
+ Misses 9387 7519 -1868
+ Partials 2293 1589 -704
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| // data | ||
| // Set per cluster mirror registry | ||
| // +optional | ||
| MirrorRegistryRef *v1beta1.MirrorRegistryConfigMapReference `json:"mirrorRegistryRef,omitempty"` |
There was a problem hiding this comment.
What do you think about using the corev1.ObjectReference type? Similar to https://github.com/openshift-assisted/cluster-api-agent/blob/9abff0c472c9156bde1cbc936fcc5bb03db7700f/bootstrap/api/v1alpha1/agentbootstrapconfig_types.go#L85
There was a problem hiding this comment.
I though so too but after reading corev1.ObjectReference documentation I decided to create a new reference type. LMK if still you think we should use corev1.ObjectReference
// Instead of using this type, create a locally provided and used type that is well-focused on your reference.
// For example, ServiceReferences for admission registration: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L533 .
| @@ -144,12 +145,12 @@ type OCPClusterAPI interface { | |||
|
|
|||
| //go:generate mockgen --build_flags=--mod=mod -package bminventory -destination mock_installer_internal.go . InstallerInternals | |||
| type InstallerInternals interface { | |||
| RegisterClusterInternal(ctx context.Context, kubeKey *types.NamespacedName, params installer.V2RegisterClusterParams) (*common.Cluster, error) | |||
| RegisterClusterInternal(ctx context.Context, kubeKey *types.NamespacedName, mirrorRegistryConfiguration *v1beta1.MirrorRegistryConfiguration, params installer.V2RegisterClusterParams) (*common.Cluster, error) | |||
There was a problem hiding this comment.
Have you considered adding it to the params? This way we can avoid changing the APIs here, which seems like an overkill...
There was a problem hiding this comment.
Yeah, One of the definitions in this task is to not affect the REST API. Updating the ClusterCreateParams can only be done with adding it to the swagger definition which will affect the REST API.
There was a problem hiding this comment.
I don't like that implementation also, from my perspective there are few solution for passing the mirrorRegistryConfiguration:
- Change the internal API (as done here) and pass
mirrorRegistryConfigurationif needed - Change
ClusterCreateParamsand includemirrorRegistryConfigurationwhich affect the REST API - Create a new structs that holds kubeapi parameters (e.g.
kubeKey,mirrorRegistryConfiguration), this struct will replacekubeKeyand will prevent us from adding new parameters to the function declaration in the future. The main problem with this method is that it will require us to refactor the code and it a bit out of this PR scope
There was a problem hiding this comment.
Oh, I see. But I don't really think it's an issue to change the API for this. I mean, it won't add any mandatory properties the the parameters, so there's no risk for breaking backwards compatibility, etc.
It should be fine to simply add it as an optional property in the swagger, as we do for many properties - it would only affect the swagger model as we won't require it in the API. So seems like it makes sense to move forward with this direction(?)
There was a problem hiding this comment.
You basically right, but the moment we expose the mirror registry configuration on swagger we will need to support some logic on the REST side (even if this is just to raise an error when the user supplying mirror registry configuration on non kube api mode).
@avishayt WDYT?
| GetClusterInternal(ctx context.Context, params installer.V2GetClusterParams) (*common.Cluster, error) | ||
| UpdateClusterNonInteractive(ctx context.Context, params installer.V2UpdateClusterParams) (*common.Cluster, error) | ||
| GetClusterByKubeKey(key types.NamespacedName) (*common.Cluster, error) | ||
| GetHostByKubeKey(key types.NamespacedName) (*common.Host, error) | ||
| InstallClusterInternal(ctx context.Context, params installer.V2InstallClusterParams) (*common.Cluster, error) | ||
| InstallClusterInternal(ctx context.Context, params installer.V2InstallClusterParams, mirrorRegistryConfiguration *v1beta1.MirrorRegistryConfiguration) (*common.Cluster, error) |
| if clusterInstall.Spec.MirrorRegistryRef != nil { | ||
| mirrorRegistryConfiguration, err = r.processMirrorRegistryConfig(ctx, log, clusterInstall) | ||
| if err != nil { | ||
| return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, nil, newInputError(err.Error())) |
There was a problem hiding this comment.
maybe worth adding some log here
There was a problem hiding this comment.
The log is printed inside processMirrorRegistryConfig
| userTomlConfigMap := &corev1.ConfigMap{} | ||
| err := c.Get(ctx, types.NamespacedName{Name: ref.Name, Namespace: ref.Namespace}, userTomlConfigMap) | ||
| if err != nil { | ||
| log.Error(err, "Failed to get ConfigMap", "ConfigMapName", ref.Name, "ConfigMapNamespace", ref.Namespace) |
There was a problem hiding this comment.
do we have a validation for a missing CM? probably need some condition error in status for that.
There was a problem hiding this comment.
We update the status here. Is that enough?
| @@ -499,7 +505,12 @@ func (r *InfraEnvReconciler) createInfraEnv(ctx context.Context, log logrus.Fiel | |||
| createParams.InfraenvCreateParams.StaticNetworkConfig = staticNetworkConfig | |||
| } | |||
|
|
|||
| return r.Installer.RegisterInfraEnvInternal(ctx, key, createParams) | |||
| mirrorRegistryConfiguration, err := mirrorregistries.ProcessMirrorRegistryConfig(ctx, log, r.Client, infraEnv.Spec.MirrorRegistryRef) | |||
There was a problem hiding this comment.
Do we need to handle somewhere a conflict between ACI and InfraEnv? I.e. do we want to settle a potential mismatch in config?
There was a problem hiding this comment.
Nope, by @avishayt definition the user is able to set different mirror registry for InfraEnv and ACI
There was a problem hiding this comment.
I forgot to click submit on my comment yesterday 😆 but I had the same thought!
Do we handle if user sets different mirror registry for both? Do we merge the mirror registries when they start install?
There was a problem hiding this comment.
So maybe worth adding some warning/info logs? I mean in case the user did it by mistake - as I suppose the common use-case would be to set smilier registries.
There was a problem hiding this comment.
Basically, We don't want to check if the mirror registries are different, as long as they are exists. On failure the user will see the status update.
|
Just a head's up that Nick is putting in a change for the operator to always include the hub's OCP cluster's CA certificate bundle #6649 Another consideration is if the user supplies an overall Hub Cluster mirror registry CA certificate (that will now be merged with the Hub Cluster's CA bundle from the above PR). We should handle the case where they provide both a per spoke cluster mirror registry CA and a mirror registry CA in the hub cluster. |
eliorerz
force-pushed
the
MGMT-18013-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall
branch
from
588df6b
to
c296dbc
Compare
|
/test edge-subsystem-aws edge-subsystem-kubeapi-aws edge-unit-test |
# Conflicts: # api/go.mod
…MirrorRegistryConfig
…rorRegistryConfig # Conflicts: # internal/ignition/discovery_test.go
eliorerz
force-pushed
the
MGMT-18013-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall
branch
from
c296dbc
to
109fc2a
Compare
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-sigs/prow repository. |
|
@eliorerz: 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-sigs/prow repository. I understand the commands that are listed here. |
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
/cc @eliorerz