Skip to content

Conversation

@liulinC
Copy link
Collaborator

@liulinC liulinC commented Nov 27, 2025

  • Introduce https_only argument for Host.create
  • Set https_only from configuration for installation
  • Keep https_only from joining host during pool join

@liulinC
Copy link
Collaborator Author

liulinC commented Nov 27, 2025

The configuration file is added in the spec repo.

}
; {
param_type= Bool
; param_name= "https_only"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we already have an option like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has been introduced in XAPI 22.27.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm there is some duplication between create_params and the fields, looks like this was missing from create_params before.

It'd be good if the list of fields in create_params wouldn't have to be repeated when creating the object itself, otherwise we have to declare some fields twice, as here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doesn't they means two different things? One is DB field, and one is API argument?
I presume not all field needs to be exposed during create API?

Copy link
Member

Choose a reason for hiding this comment

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

The host.create API call does not include all fields indeed. It is not a public call and only used by xapi internally, for pool join.

@lindig
Copy link
Contributor

lindig commented Nov 27, 2025

What is the connection with

let set_https_only =
  call ~name:"set_https_only"
    ~doc:
      "updates the host firewall to open or close port 80 depending on the \
       value"
    ~lifecycle:[]
    ~params:
      [
        (Ref _host, "self", "The Host")
      ; ( Bool
        , "value"
        , "true - http port 80 will be blocked, false - http port 80 will be \
           open"
        )
      ]
    ~allowed_roles:_R_POOL_OP ()

This appears to serve the same purpose and this field will have a default. So why is it not enough to change the default? I looked at the ticket and it is not obvious. Also, if this is a policy, should this not be tied to the pool?

@robhoes
Copy link
Member

robhoes commented Nov 27, 2025

What is the connection with (...)

That is an API call to change firewall. This PR is about making the installation default configurable (not obvious from the PR title "Make HTTP/80 configurable"). It also preserves the configuration on pool join.

@lindig
Copy link
Contributor

lindig commented Nov 27, 2025

What is the problem being solved by this PR compared to the existing solution and could it not be extended? Is the difference not rather small and thus risks confusion?

@liulinC
Copy link
Collaborator Author

liulinC commented Nov 28, 2025

What is the problem being solved by this PR compared to the existing solution and could it not be extended? Is the difference not rather small and thus risks confusion?

The background here is we want to disable(not expose) http on XS9 while keep it open for XS8 by default.
So what really matters is the default value.
Of-course, user can use set_https_only to set it another value if they want, but what we update here is the default value without the later explicit settings.

Another way is we use some first boot service to set the value after xapi startup with set_https_only, which is not as clean as current solution.

@lindig
Copy link
Contributor

lindig commented Nov 28, 2025

Another way is we use some first boot service to set the value after xapi startup with set_https_only, which is not as clean as current solution.

This changes 8 files whereas that would re-use an existing mechanism. Anyway, thanks for providing some background.

- Introduce https_only argument for Host.create
- Set https_only from configuration for installation
- Keep https_only from joining host during pool join

Signed-off-by: Lin Liu <[email protected]>
@liulinC
Copy link
Collaborator Author

liulinC commented Dec 2, 2025

push -f just update the release version number.

@liulinC liulinC added this pull request to the merge queue Dec 2, 2025
Copy link
Contributor

@changlei-li changlei-li left a comment

Choose a reason for hiding this comment

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

When host joins pool, the host will use pool master's https_only value, am I right?

create_or_get_sr_on_master __context rpc session_id
(my_local_cache_sr, my_local_cache_sr_rec)
in

Copy link
Contributor

Choose a reason for hiding this comment

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

strange blank line

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 added a line of code here and then remove it, which cause this empty line.
make format does not care about this.

Merged via the queue into xapi-project:master with commit 62f962b Dec 2, 2025
16 checks passed
@liulinC
Copy link
Collaborator Author

liulinC commented Dec 2, 2025

When host joins pool, the host will use pool master's https_only value, am I right?

Before joining the pool, the check would make sure the https_only field is the same with pool master,
here we use the local host value, (which is the same as master)

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.

6 participants