MGMT-18418: Use Image Service HTTP IP for live iso URL

[CrystalChun]

Ironic is unable to pull using assisted-service's CA certificate from changes in openshift/assisted-service#6564 https://issues.redhat.com/browse/MGMT-18418

Adds the option to use the internal IP of the service as the URL to provision the BMH so Ironic can pull the image without TLS verification.

[openshift-ci-robot]

@CrystalChun: This pull request references MGMT-18418 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 bug to target the "4.17.0" version, but no target version was set.

Details

In response to this:

Ironic is unable to pull using assisted-service's CA certificate from changes in openshift/assisted-service#6564 https://issues.redhat.com/browse/MGMT-18418

Adds the option to use the internal IP of the service as the URL to provision the BMH so Ironic can pull the image without TLS verification.

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CrystalChun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [CrystalChun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[carbonin]

Maybe "Internal" instead of "Insecure"?

[CrystalChun]

That sounds much better, thanks!

[CrystalChun]

Modified!

[carbonin]

You should do all of this with the standard library net/url

[CrystalChun]

Thank you!!

[CrystalChun]

Modified to use the std lib net/url package!

[CrystalChun]

/retest

[carbonin]

@jianzzha can you put a review here? I want to make sure whatever we're building will work for how you want to deploy this.

[carbonin]

I thought this was meant to allow someone deploying this to change the namespace for assisted. How does this do that?

[CrystalChun]

This stanza replaces the first environment variable listed in the base deployment yaml with the value specified here.

So the result of $ kustomize build bootstrap/config/manager/overlays/ocp
is this:

...
apiVersion: apps/v1
kind: Deployment
metadata:
...
  name: controller-manager
  namespace: system
spec:
  replicas: 1
  selector:
    matchLabels:
      control-plane: controller-manager
  template:
    metadata:
      annotations:
        kubectl.kubernetes.io/default-container: manager
      labels:
        control-plane: controller-manager
    spec:
      containers:
      - args:
        - --leader-elect
        command:
        - /manager
        env:
        - name: USE_INTERNAL_IMAGE_URL
          value: "false"
        - name: IMAGE_SERVICE_NAME
          value: assisted-image-service
        - name: IMAGE_SERVICE_NAMESPACE
          value: assisted-installer
        image: quay.io/edge-infrastructure/openshift-capi-agent-bootstrap:latest
        imagePullPolicy: Always
        livenessProbe:
...

Whereas just doing $ kustomize build bootstrap/config/manager/base
produces this:

...
apiVersion: apps/v1
kind: Deployment
metadata:
...
  name: controller-manager
  namespace: system
spec:
  replicas: 1
  selector:
    matchLabels:
      control-plane: controller-manager
  template:
    metadata:
      annotations:
        kubectl.kubernetes.io/default-container: manager
      labels:
        control-plane: controller-manager
    spec:
      containers:
      - args:
        - --leader-elect
        command:
        - /manager
        env:
        - name: USE_INTERNAL_IMAGE_URL
          value: "true"
        - name: IMAGE_SERVICE_NAME
          value: assisted-image-service
        - name: IMAGE_SERVICE_NAMESPACE
          value: assisted-installer
        image: quay.io/edge-infrastructure/openshift-capi-agent-bootstrap:latest
        imagePullPolicy: Always
        livenessProbe:
...

[jianzzha]

For the purpose of the sylva integration, the released manifests are used. For example if release 0.1.5 will have this fix, it's the github.com/openshift-assisted/cluster-api-agent/releases/download/v0.1.5/{bootstrap-components.yaml,controlplane-components.yaml} will be used by Sylva. Since bootstrap-components.yaml is already updated, so I think we are good. We can always overwrite USE_INTERNAL_IMAGE_URL from a sylva unit via kustomization.

@CrystalChun This base/ and overlays/ocp/ folder are meant for internal use only, right? maybe they can be re-organized for some e2e CI test and used to deploy all manifests required for the CAPI provider to emulate how an end user will deploy the manifests on a k8s or OCP cluster. But that's outside of the scope of this PR and we can discuss that later.

[jianzzha]

I think the intention of kustomize folder is for users who has cloned the source repo to be able to use kubectl apply -k to deploy the capi pods and resources in one step. The root level README.md does mention kubectl apply -k config/samples/. So I guess the purpose of the base/ and overlays/ folder here will be eventually used for that purpose? @CrystalChun @rccrdpccl

[rccrdpccl]

Wouldn't it be easier if we have the base layer without these envs, and just add them in the overlays? This way if we add other env vars the order would not matter.
Also, we should probably disable this behaviour by default and assume public addresses. If that's not the case the user should know and configure the provider accordingly (I know the main use case is with this feature enabled, but I feel we should enable it when we declare what address we have, as by default we would need to fail the controller if it's enabled but not internal namespace/service it's defined)

[jianzzha]

Regardless what default value to choose, the usage of the env var needs to be documented in the README.md.

[CrystalChun]

I did initially try that (no env vars in the base and add them in overlay) but kustomize acted weird with it :/ I might've been patching it incorrectly though

[rccrdpccl]

Should this have a default from within the code? Normally we deploy this way, but can we really assume it from here? Wouldn't it be safer to force the use specify this?

[carbonin]

This is part of the reason I asked @jianzzha to review.

I don't know what we will "normally" be doing.
When assisted is deployed through MCE it won't be in the assisted-installer namespace, but when we deploy it for dev, it is. I'm also not sure what namespace whatever MCE distribution we use for sylva will use. I don't know what to consider "normal" in this case.

[jianzzha]

Sylva is referencing https://github.com/openshift/assisted-service/tree/master/config/default kustomize folder at this moment, so 'assisted-installer' namespace is set by that kustomize.

I'm assuming that MCE can kustomize the IMAGE_SERVICE_NAMESPACE if it needs to?

[carbonin]

I'm assuming that MCE can kustomize the IMAGE_SERVICE_NAMESPACE if it needs to?

This is what I'm trying to ensure. MCE does not use kustomize when deploying assisted. I assume it also won't use kustomize when deploying this CAPI provider (if it is even going to be responsible for that). MCE pulls the files from the rendered operator bundle and applies those.

[carbonin]

But the open question (for me at least) is still if MCE is going to deploy the CAPI providers or if they're being installed through some other means.

[carbonin]

I suggested looking in the same namespace as a more sensible default than hardcoding assisted-installer.

I think we should still expose the env var as overrideable, but it was my understanding that we didn't have a good way to do that from kustomize without hardcoding other namespaces.

As for where these components are running we only have three cases:

1. Everything installed from MCE -> everything runs in the same namespace
2. Assisted installed from MCE, CAPI provider installed by user -> doc to run CAPI provider in assisted namespace
3. User installs everything -> doc to run everything in the same namespace.

From what I could tell from the discussion around kustomize overrides for the namespace this seems more reliable.

[rccrdpccl]

I see and agree that running everything in the same namespace might be more sensible than hardcoding assisted's namespace. What about the following:

• Default to USE_INTERNAL_IMAGE_URL to false: this will use public route, where all of this is a non-issue and it would be the desired behaviour
• Default IMAGE_SERVICE_NAMESPACE to empty: when empty it will mean same namespace (if override is turned on), however it can be overridden if need be.

In Sylva's "upstream" case, we'd only need to add one env var through kustomization (USE_INTERNAL_IMAGE_URL=true) and deploy in the same namespace.
In MCE case, everything would be deployed in the same namespace, but no overrides needed as we'd go through the advertised public address.

Basically my point is that the "workaround" (USE_INTERNAL_IMAGE_URL) should not be default behaviour. Would you agree?

[carbonin]

In MCE case, everything would be deployed in the same namespace, but no overrides needed

You're still assuming whenever we run with MCE we're going to be in OCP and I'm not sure yet that this will always be true.
But that said, I beleive MCE's installer can set env vars when it deploys operators so we'd have to get them to set this in the OCP/kube case no matter what we choose so I don't think it's an issue.

Plus we're still a bit away from having MCE install this provider so we can probably deal with this when it's actually an issue.

[rccrdpccl]

Wouldn't it be easier to just remove any "workaround" kustomization rather than having to change the upstream manifest, when/if we'll finally release that?
I feel we are making a workaround the default behaviour making some assumptions (what namespace we run what component) on the way. Shouldn't we allow the user to have this workaround and make the assumptions/decisions themselves?

[carbonin]

Had a discussion offline, the decision was to remove the default value for the image service namespace. If provided we will use it to look up the service, if not we will look for the service in the namespace the provider is running in.

We will also default USE_INTERNAL_IMAGE_URL to false and the user will need to change that if they want/need to.

[carbonin]

Super minor but we probably don't need the default at all here, right?

[CrystalChun]

Oh yep good point! Removing it

[carbonin]

Looks good to me. I'll let @rccrdpccl give it the final lgtm though

[rccrdpccl]

Could we please add some documentation about how would we use this feature and why?

[CrystalChun]

Could we please add some documentation about how would we use this feature and why?

Sorry missed it originally, just added it!

[rccrdpccl]

We already have some instructions in the readme about installing the providers, can we integrate those docs with these new instructions?

[CrystalChun]

Gotcha, yes tried to do so but since we're suggesting a kustomize with a patch, these instructions/prerequisites mostly stayed the same 😅 do you know how to do it with clusterctl?

Also would you like assisted service as a prerequisite for the general deployment instructions? I was assuming we're targeting OCP so assisted service would already be deployed and need not be listed as a general prerequisite. Only listed if it's vanilla kube. Let me know your thoughts!

[rccrdpccl]

AFAIK we cannot do this with clusterctl, but I meant maybe we can have one section explaining how to install, with all the variants.

[rccrdpccl]

/lgtm