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

[WIP/Feedback needed] Add a duplicate strategy to deploy ephemeral environment #274

Closed
wants to merge 3 commits into from

Conversation

GregoireW
Copy link

This has to be used with the PR in the image-reflector-controller : fluxcd/image-reflector-controller#211

This PR add a duplicate strategy.

    update:
      strategy: Duplicates

If this strategy find a kubernetes object referencing an image policy with image distribution, then it will duplicate the object in a new file which name is suffixed with __xxx where xxx is the discriminator. (it means a single file cannot contains multiple policy.).

for instance, a test.yaml file with

apiVersion: v1
kind: Service
metadata:
  name: none # {"$imagepolicy": "flux-system:pr", "template": "none{{.optionalSeparator}}{{.discriminator}}" }
  namespace: default
  labels:
    app: none # {"$imagepolicy": "flux-system:pr", "template": "none{{.optionalSeparator}}{{.discriminator}}" }
    sha1: xxx # {"$imagepolicy": "flux-system:pr", "template": "{{.sha1}}" }

then a policy like:

status:
  distribution:
    "123":
      attributes:
        pr: "123"
        sha1: e15de12a
        ts: "202112041743"
      image: img-demo/demo
      tag: pr-123-202112041743-e15de12a
    "124":
      attributes:
        pr: "124"
        sha1: 5fde2b34
        ts: "202112051625"
      image: img-demo/demo
      tag: pr-124-202112051625-5fde2b34
  latestDiscriminator: "123"
  latestImage: img-demo/demo:pr-123-202112041743-e15de12a
  nbDistribution: 2

will update the test.yaml file to the latest image, but will not use the discriminator:

apiVersion: v1
kind: Service
metadata:
  name: none # {"$imagepolicy": "flux-system:pr", "template": "none{{.optionalSeparator}}{{.discriminator}}" }
  namespace: default
  labels:
    app: none # {"$imagepolicy": "flux-system:pr", "template": "none{{.optionalSeparator}}{{.discriminator}}" }
    sha1: e15de12a # {"$imagepolicy": "flux-system:pr", "template": "{{.attributes.sha1}}" }

and will create 2 files test__123.yaml and test__124.yaml

which will contains (the 123 exemple) (comment has been removed to prevent loop creation)

apiVersion: v1
kind: Service
metadata:
  name: none-123
  namespace: default
  labels:
    app: none-123
    sha1: e15de12a

The comment has been extended to get a template attribute. This attribute is a simple template which can use the attributes, image and name.

Signed-off-by: GregoireW <24318548+GregoireW@users.noreply.github.com>
Signed-off-by: GregoireW <24318548+GregoireW@users.noreply.github.com>
Signed-off-by: GregoireW <24318548+GregoireW@users.noreply.github.com>
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

Nice work so far. 🙌
Left some initial quick comments here.

limitations under the License.
*/

package v1beta2
Copy link
Member

Choose a reason for hiding this comment

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

I see no API changes here, other than adding another value for the enum which doesn't really count. The API version bump is not justified.

Copy link
Author

Choose a reason for hiding this comment

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

OK, then good news... Wasn't sure of this. I will revert.

// uses kyaml setters. NB the value in the enum annotation for the
// type, above.
UpdateStrategySetters UpdateStrategyName = "Setters"
UpdateStrategyDuplicates UpdateStrategyName = "Duplicates"
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't say this is really a new strategy as your implementation here still relies on the kyaml setters. This is more of an extension to it.

Copy link
Author

Choose a reason for hiding this comment

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

Setters will just change value on a given object, Duplicates will duplicates an object (well Create additional object, modify them or delete them). Kyaml is just used to manage this duplication. So big difference.

Comment on lines +264 to +265
withoutDiscriminator := func (p imagev1_reflect.ImagePolicy) bool{
return p.Spec.FilterTags==nil || p.Spec.FilterTags.Discriminator == ""
Copy link
Member

Choose a reason for hiding this comment

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

There are formatting inconsistencies here. Please use gofmt, running make locally before committing would do.

Suggested change
withoutDiscriminator := func (p imagev1_reflect.ImagePolicy) bool{
return p.Spec.FilterTags==nil || p.Spec.FilterTags.Discriminator == ""
withoutDiscriminator := func (p imagev1_reflect.ImagePolicy) bool {
return p.Spec.FilterTags == nil || p.Spec.FilterTags.Discriminator == ""

Copy link
Author

Choose a reason for hiding this comment

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

will do.

@GregoireW
Copy link
Author

@relu Now you check both PR on image-reflector and here on image-automation, what do you (well the fluxcd team) think about that ?
Is this something that make sense ? does this fit in the global picture ?

The json I use to apply the policy is something like # {"$imagepolicy": "flux-system:pr", "template": "{{.sha1}}" } Is it ok ? should we add some other options like 'property' which would return the single property and not a template ?

if so I can finish this :

  • test, fmt ...
  • on the policy side, accept additional attributes even on policy without discriminator (today there is a test, if discriminator, go to my code, if not go to the original fluxcd code)
  • on the automation side the result is not correctly done (list of changes) will need to fix that
  • the setters policy is really linked to some internal kustomize object. If we want to be able to set additional template, I'm afraid this will need to be refactored a little bit like I did on the duplicate policy

@GregoireW
Copy link
Author

will open a discussion about this

@GregoireW GregoireW closed this Dec 13, 2021
@GregoireW GregoireW deleted the feature/duplicator branch December 13, 2021 15:37
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.

None yet

2 participants