Skip to content
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.

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

✨ Adding support for multiple registry mirrors in bottlerocket #28

Merged

Conversation

pokearu
Copy link
Collaborator

@pokearu pokearu commented Jan 22, 2024

What this PR does / why we need it:
Currently the registry mirror configuration for Bottlerocket was hardcoded to setup mirrors for only public.ecr.aws. With these changes we introduce new API fields for RegistryMirrorConfiguration that enable listing multiple registries to be mirrored to a given private registry.

Example

spec:
  kubeadmConfigSpec:
    clusterConfiguration:
      registryMirror:
        caCert: |
          -----BEGIN CERTIFICATE-----
          MIIF5DCCA8ygAwIBAgIUXgwAZqssgRQ/Kd3HKNJC2sTVsWQwDQYJKoZIhvcNAQEL
          ...
          -----END CERTIFICATE-----
        mirrors:
          - registry: "public.ecr.aws"
            endpoint: "1.2.3.4:443/eks-a"
          - registry: "docker.io"
            endpoint: "1.2.3.4:443"

The above configuration would setup bottlerocket settings through the user data and setup mirrors as below

{
  "settings": {
    "container-registry": {
      "mirrors": [
        {
          "endpoint": [
            "1.2.3.4:443/eks-a"
          ],
          "registry": "public.ecr.aws"
        },
        {
          "endpoint": [
            "1.2.3.4:443"
          ],
          "registry": "docker.io"
        }
      ]
    }
  }
}

If the RegistryMirrorConfiguration.Endpoint field is set and no mirrors are found, we still default to setting up public.ecr.aws as the registry.

@@ -88,16 +91,19 @@ trusted=true
// to "public.ecr.aws" rather than the mirror's endpoint
// TODO: Once the bottlerocket fixes are in we need to remove the "public.ecr.aws" creds
Copy link
Collaborator

Choose a reason for hiding this comment

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

// We need to assign creds for "public.ecr.aws" because host-ctr expects credentials to be assigned
// to "public.ecr.aws" rather than the mirror's endpoint
// TODO: Once the bottlerocket fixes are in we need to remove the "public.ecr.aws" creds

Do we validate that this is fixed by BR? is it still required to assign creds to the public.ecr.aws registry?
Also looks like you are adding credentials for all the mirrors here, is it necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I discussed this with the team. Looks like the issue is still open bottlerocket-os/bottlerocket#2677

Regarding setting creds for all reg, I am not sure if its actually necessary but we suspected it might need it. I can check again 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just confirmed. It only needs public ecr as an extra entry for creds. I can change that up.

}

if len(config.ProxyConfiguration.NoProxy) > 0 {
for _, noProxy := range config.ProxyConfiguration.NoProxy {
bottlerocketInput.NoProxyEndpoints = append(bottlerocketInput.NoProxyEndpoints, strconv.Quote(noProxy))
}
}

// When RegistryMirrorConfiguration.Endpoint is specified, we default the mirror to public.ecr.aws
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind adding detailed explanation about why we are doing this? e.g. for backward compatibility, not trigger rollout recreate of the machine etc.

bottlerocketInput.RegistryMirrorMap = make(map[string][]string)
if config.RegistryMirrorConfiguration.Endpoint != "" {
bottlerocketInput.RegistryMirrorMap["public.ecr.aws"] = []string{strconv.Quote(config.RegistryMirrorConfiguration.Endpoint)}
bottlerocketInput.RegistryMirrorEndpoint = config.RegistryMirrorConfiguration.Endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need bottlerocketInput.RegistryMirrorEndpoint at all? won't it always be config.RegistryMirrorConfiguration.Mirrors[0].Endpoints[0]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it wont be. So in this case we are using the older api and not setting mirrors at all. This was mainly done for backward compatibility and not rolling.
Example

spec:
  kubeadmConfigSpec:
    clusterConfiguration:
      registryMirror:
        caCert: |
          -----BEGIN CERTIFICATE-----
          MIIF5DCCA8ygAwIBAgIUXgwAZqssgRQ/Kd3HKNJC2sTVsWQwDQYJKoZIhvcNAQEL
          ...
          -----END CERTIFICATE-----
          endpoint: 1.2.3.4:443

Is still a valid way of setting it up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think i got what you were asking now. You meant bottlerocketInput.RegistryMirrorEndpoint not the config.RegistryMirrorConfiguration.Endpoint.

I just used it so its easier for go template parsing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea i mean you don't need to define bottlerocketInput.RegistryMirrorEndpoint, the registry map is enough to build the template.

}
}

// Right now we support only one private registry. Hence defaulting to first entry.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we need to call this out in API definition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure!

endpointRegex := regexp.MustCompile(`^(https?:\/\/)?[\w\.\:\-]+`)
if config.RegistryMirrorConfiguration.Endpoint != "" {
bottlerocketInput.RegistryMirrorMap["public.ecr.aws"] = []string{strconv.Quote(config.RegistryMirrorConfiguration.Endpoint)}
bottlerocketInput.RegistryMirrorEndpoint = endpointRegex.FindStringSubmatch(config.RegistryMirrorConfiguration.Endpoint)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can return nil, can we handle it?

@abhinavmpandey08 abhinavmpandey08 merged commit f2c51df into abhay-krishna:eks-a-release-1.6 Jan 25, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants